RE: [PATCH] incorrect use of sizeof() in ioctl definitions

[Maciej Zenczykowski]

> 	Maybe I got something wrong, but could someone please help me to
> understand why introduce _IOR_BAD here? Thanks first! :)

So as not to break userspace we must still support old values, at the same 
time we want new programs to start using the new correct values - hence 
the introduction of _backward compatibility_ values.

> (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 old was bad since it was sizeof(sizeof(...)) - it so happens that by 
def sizeof(anything) is a size_t - thus replacing sizeof(sizeof(..)) with 
sizeof(size_t) changes nothing - just shortens the code...
Of course what we probably should really have is the above (now) code 
defined as "BAD" and the previous (old) define without the sizeof as the 
current (no BAD prefix).

> 	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.

Precisely - unfortunately due to coding errors (the erroneous double 
sizeof invocation) this isn't the way it is - and for backwards 
compatibility it can't be broken... sad, really...

> 
> 	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?

Yap, both violate.  It is a mess and there is no easy fix due to the need
to retain the old invalid ioctl's as well... and the real 'tough' mess is
on platforms where due to type sizes both the OLD and the NEW defines
result in the same IOCTL define value... that'll probably screw over
switch statements (I'm not sure - but I think you can't have the same
value twice in a case statement).  If this happens on all platforms than 
the conversion OLD==BAD to NEW can be done quietly - otherwise... we've 
got hell.

Cheers,
MaZe.

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