Re: [Resend PATCH V3 1/4] usb: add struct usb_hub_port to store port related members.

[Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>]

On Wed, 13 Jun 2012, Greg KH wrote:

> > > > +struct usb_hub_port {
> > > > +	void			*port_owner;
> > > 
> > > What?  Why a void pointer?  You should almost _never_ use a void pointer
> > > in kernel code, it's bad form, and it means you are trying to hide
> > > something here, when it shouldn't be hidden, as we should know the real
> > > type, right?
> > 
> > That's my fault; Lan is simply moving code that I wrote.
> > 
> > The void * values are simply cookies as far as the hub driver is
> > concerned -- the meaning is _intended_ to be hidden and the pointers
> > are never dereferenced.  All that matters about them is whether two
> > values are the same or different.  The actual pointers stored in
> > port_owner are created by usbfs; they are pointers to the internal data
> > structures defined in devio.c.
> > 
> > If you really want, we could change void * to struct dev_state *.  I'm 
> > not sure this would make things any better or clearer.
> 
> Yes, it would make it clearer when we read the code, seing dev_state *,
> makes me know that we know what is going on.  A void * makes me know we
> have no clue as to what is going on :)

Hmmm.  The thing is, these values really aren't pointers -- or at
least, they aren't used as pointers.

What's the right type to represent an arbitrary cookie?  Something
large enough to hold a pointer but which will never be dereferenced?  
Unsigned long?  Or maybe a typedef:

	typedef void *hub_cookie_t;

What do you suggest?

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html