Messages in this thread | | | Subject | RE: [PATCH] incorrect use of sizeof() in ioctl definitions | Date | Wed, 8 Oct 2003 16:40:46 +0800 | From | "Tian, Kevin" <> |
| |
> Andries Brouwer [mailto:aebr@win.tue.nl] wrote: > On Tue, Sep 30, 2003 at 11:25:56PM +0100, Matthew Wilcox wrote: > > On Tue, Sep 30, 2003 at 02:08:05PM -0700, Andrew Morton wrote: > > > Arun Sharma <arun.sharma@intel.com> wrote: > > > > > > > > Some drivers seem to use macros such as _IOR/_IOW in a way that ends > up > > > > calling the sizeof() operator twice. For eg: > > > > > > > > -#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, sizeof(__u32*)) > > > > +#define FBIO_ATY128_GET_MIRROR _IOR('@', 1, __u32*) > > But this changes the define. You want > > #define FBIO_ATY128_GET_MIRROR _IOR_BAD('@', 1, __u32*)
Maybe I got something wrong, but could someone please help me to understand why introduce _IOR_BAD here? Thanks first! :)
IMO, the birth of new ioctl definition convention considers "size" of argument as a part, so we should conform to this rule. _IOR_BAD is no different as old one <_IOR('@', 1, sizeof(__u32*))>, which expands as sizeof(sizeof(__32*)) and actually assume temp result of internal sizeof as the argument. Of course this didn't reflect the true point and we should change the definition. :)
Also I'm confused about the modification about using size_t to replace sizeof(). Take MATROXFB_SET_OUTPUT_MODE for example:
(old) #define MATROXFB_SET_OUTPUT_MODE _IOW('n',0xFA,sizeof(struct matroxioc_output_mode)) (now) #define MATROXFB_SET_OUTPUT_MODE _IOW('n',0xFA,size_t)
The size of matroxioc_output_mode is 8 bytes on all platforms, however size_t will be calculated as 4 bytes on 32bit arch and 8 bytes on 64bit arch. So this is also like using sizeof(), which imposes the difference between 32bit ioctl number and 64bit ioctl number. However in standard manner, I mean: #define MATROXFB_SET_OUTPUT_MODE _IOW('n',0xFA,struct matroxioc_output_mode) The value should be identical on all platforms, which save our efforts to do useless conversion when running 32bit apps on 64bit platform.
The most important is: to use sizeof() or size_t here both messed the ioctl definition, which violate the initial motivation of _IO**, is it?
> Something else we should do is to change all occurrences of 'size' > here into 'argtype'. All this nonsense came because of the bad choice > of identifier. Agree. The inaccurate name here confused us... :) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |