why not choose another way to define the _IOC_xxxMASK related to the ioctl

why not choose another way to define the _IOC_xxxMASK related to the ioctl

[RS]

it defines in the kernel:  #define _IOC_NRMASK	((1 << _IOC_NRBITS)-1)   //define  ...  #define _IOC_NRSHIFT	0  ...  #define _IOC_DIR(nr)		(((nr) >> _IOC_DIRSHIFT) & _IOC_DIRMASK)  //when decode

why not define it like this:
  #define _IOC_NRSHIFT	0
  ...
  #define _IOC_NRMASK ((_IOC_NRSHIFT >> _IOC_NRBITS) - _IOC_NRSHIFT)   //define
  ...
  #define _IOC_DIR(nr)        ((nr &  _IOC_DIRMASK) >> _IOC_DIRSHIFT)  // when decode


I think it is better for the word "mask" . 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20130330/d0df0713/attachment.html 

why not choose another way to define the _IOC_xxxMASK related to the ioctl

[Tobias Boege]

On Sat, 30 Mar 2013, RS wrote:
> it defines in the kernel:  #define _IOC_NRMASK	((1 << _IOC_NRBITS)-1)   //define  ...  #define _IOC_NRSHIFT	0  ...  #define _IOC_DIR(nr)		(((nr) >> _IOC_DIRSHIFT) & _IOC_DIRMASK)  //when decode
> 
> why not define it like this:
>   #define _IOC_NRSHIFT	0
>   ...
>   #define _IOC_NRMASK ((_IOC_NRSHIFT >> _IOC_NRBITS) - _IOC_NRSHIFT)   //define
>   ...
>   #define _IOC_DIR(nr)        ((nr &  _IOC_DIRMASK) >> _IOC_DIRSHIFT)  // when decode
> 
> 
> I think it is better for the word "mask" . 
> 

It can't be better this way because it's wrong. Let's compute a bit:

You are referring include/uapi/asm-generic/ioctl.h AFAICS:

	#define _IOC_NRBITS     8
	#define _IOC_NRSHIFT    0
	#define _IOC_NRMASK     ((1 << _IOC_NRBITS)-1)

This gives _IOC_NRMASK = ((1 << 8) - 1) = 0xff. But supposing your

	_IOC_NRMASK	((_IOC_NRSHIFT >> _IOC_NRBITS) - _IOC_NRSHIFT)

it yields _IOC_NRMASK = ((0 >> 8) - 0) = 0x00.

You see? With your mask you'll never get any bits out of the ioctl number
because it's just wrong.

The same applies to _IOC_DIR(nr). You can't just exchange bitwise ands and
shifts - you'd have to change the constants for this... and there is really
no point in it.

Regards,
Tobi

why not choose another way to define the _IOC_xxxMASK related to the ioctl

[RS]

Think you !
Oh , I was wrong . How careless I was.
I have fixed it :
#define _IOC_NRMASK     (((1 << _IOC_NRBITS)-1) << _IOC_NRSHIFT)  // if the  _IOC_NRBITS is 8 ,then the result is 0XFF
#define _IOC_DIR(nr)        ((nr &  _IOC_DIRMASK) >> _IOC_DIRSHIFT)  // when decode , keep the same.
Now I think this will spend more time than the kernel code when executed. It performs an extra operation ">>", but I think it better to understand. 
     _IOC_DIRMASK         0xC0000000
     _IOC_TYPEMASK      0x0000FF00
     _IOC_NRMASK          0x000000FF
     _IOC_SIZEMASK        0x3FFF0000





At 2013-03-30 16:41:55,"Tobias Boege" <tobias@gambas-buch.de> wrote:
>On Sat, 30 Mar 2013, RS wrote:
>> it defines in the kernel:  #define _IOC_NRMASK	((1 << _IOC_NRBITS)-1)   //define  ...  #define _IOC_NRSHIFT	0  ...  #define _IOC_DIR(nr)		(((nr) >> _IOC_DIRSHIFT) & _IOC_DIRMASK)  //when decode
>> 
>> why not define it like this:
>>   #define _IOC_NRSHIFT	0
>>   ...
>>   #define _IOC_NRMASK ((_IOC_NRSHIFT >> _IOC_NRBITS) - _IOC_NRSHIFT)   //define
>>   ...
>>   #define _IOC_DIR(nr)        ((nr &  _IOC_DIRMASK) >> _IOC_DIRSHIFT)  // when decode
>> 
>> 
>> I think it is better for the word "mask" . 
>> 
>
>It can't be better this way because it's wrong. Let's compute a bit:
>
>You are referring include/uapi/asm-generic/ioctl.h AFAICS:
>
>	#define _IOC_NRBITS     8
>	#define _IOC_NRSHIFT    0
>	#define _IOC_NRMASK     ((1 << _IOC_NRBITS)-1)
>
>This gives _IOC_NRMASK = ((1 << 8) - 1) = 0xff. But supposing your
>
>	_IOC_NRMASK	((_IOC_NRSHIFT >> _IOC_NRBITS) - _IOC_NRSHIFT)
>
>it yields _IOC_NRMASK = ((0 >> 8) - 0) = 0x00.
>
>You see? With your mask you'll never get any bits out of the ioctl number
>because it's just wrong.
>
>The same applies to _IOC_DIR(nr). You can't just exchange bitwise ands and
>shifts - you'd have to change the constants for this... and there is really
>no point in it.
>
>Regards,
>Tobi
>
>
>_______________________________________________
>Kernelnewbies mailing list
>Kernelnewbies at kernelnewbies.org
>http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20130330/8c1f7b89/attachment.html 

why not choose another way to define the _IOC_xxxMASK related to the ioctl

[Valdis.Kletnieks at vt.edu]

On Sat, 30 Mar 2013 18:01:32 +0800, RS said:
>
> Now I think this will spend more time than the kernel code when executed.

Have you actually examined the generated code on several popular
architectures to see what gcc actually does?

(hint - many things can constant-folded at compile time.  So if
the 3 values are #defined to constants, the expression

    (_IOC_NRSHIFT >> _IOC_NRBITS) - _IOC_NRSHIFT)

will generate no actual shift or subtract instructions, merely another
constant.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 865 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20130330/ba725e47/attachment.bin