[08/10] seccomp.2: Remove unneeded cast

Message ID 20200925073140.173394-9-colomar.6.4.3@gmail.com
State New
Headers show
Series
  • Add types, and some fixes
Related show

Commit Message

H.J. Lu via Libc-alpha Sept. 25, 2020, 7:31 a.m.
Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

---
 man2/seccomp.2 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.28.0

Comments

H.J. Lu via Libc-alpha Sept. 25, 2020, 8:34 a.m. | #1
Hi Alex,

On 9/25/20 9:31 AM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

> ---

>  man2/seccomp.2 | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/man2/seccomp.2 b/man2/seccomp.2

> index 58033da1c..d6b856c32 100644

> --- a/man2/seccomp.2

> +++ b/man2/seccomp.2

> @@ -1101,7 +1101,7 @@ install_filter(int syscall_nr, int t_arch, int f_errno)

>      };

>  

>      struct sock_fprog prog = {

> -        .len = (unsigned short) (sizeof(filter) / sizeof(filter[0])),

> +        .len = sizeof(filter) / sizeof(filter[0]),

>          .filter = filter,

>      };


I have a small doubt about this change. With the change,
there are no compilation warnings.

But, if we change the code to something slightly different:

[[
    size_t x = (sizeof(filter) / sizeof(filter[0]));
    struct sock_fprog prog = {
        .len = x,
        .filter = filter,
    };
]]

The "cc -Wconversion" gives us the following warning:

    warning: conversion from ‘size_t’ {aka ‘long unsigned int’}
    to ‘short unsigned int’ may change value

Presumably we don't get a warning for an assignment of the form

    .len = (sizeof(filter) / sizeof(filter[0]))

because the compiler is smart enough to work out that the
value of the constant expression is within the range of
"unsigned short".

Your thoughts?

Thanks,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
H.J. Lu via Libc-alpha Sept. 25, 2020, 8:42 a.m. | #2
On 2020-09-25 10:34, Michael Kerrisk (man-pages) wrote:
> Hi Alex,

> 

> On 9/25/20 9:31 AM, Alejandro Colomar wrote:

>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

>> ---

>>   man2/seccomp.2 | 2 +-

>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>

>> diff --git a/man2/seccomp.2 b/man2/seccomp.2

>> index 58033da1c..d6b856c32 100644

>> --- a/man2/seccomp.2

>> +++ b/man2/seccomp.2

>> @@ -1101,7 +1101,7 @@ install_filter(int syscall_nr, int t_arch, int f_errno)

>>       };

>>   

>>       struct sock_fprog prog = {

>> -        .len = (unsigned short) (sizeof(filter) / sizeof(filter[0])),

>> +        .len = sizeof(filter) / sizeof(filter[0]),

>>           .filter = filter,

>>       };

> 

> I have a small doubt about this change. With the change,

> there are no compilation warnings.

> 

> But, if we change the code to something slightly different:

> 

> [[

>      size_t x = (sizeof(filter) / sizeof(filter[0]));

>      struct sock_fprog prog = {

>          .len = x,

>          .filter = filter,

>      };

> ]]

> 

> The "cc -Wconversion" gives us the following warning:

> 

>      warning: conversion from ‘size_t’ {aka ‘long unsigned int’}

>      to ‘short unsigned int’ may change value

> 

> Presumably we don't get a warning for an assignment of the form

> 

>      .len = (sizeof(filter) / sizeof(filter[0]))

> 

> because the compiler is smart enough to work out that the

> value of the constant expression is within the range of

> "unsigned short".

> 

> Your thoughts?


Hi Michael,

I'd say that the cast doesn't fix any problems at all.  It silences a 
valid warning, and I'd use a pragma for that (to be more explicit about 
the intention of silencing a warning) if I do want -Wconversion enabled 
(which usually I don't want, because it's too noisy) and I'm sure that 
this won't overflow.  I'd limit the use casts to only when I *really* 
need to.

I guess that if you enable -O3, the warning will vanish again because 
the compiler will optimize away 'x' (but I didn't test).

Thanks,

Alex

> 

> Thanks,

> 

> Michael

> 

>
H.J. Lu via Libc-alpha Sept. 25, 2020, 9:23 a.m. | #3
On 9/25/20 10:42 AM, Alejandro Colomar wrote:
> 

> 

> On 2020-09-25 10:34, Michael Kerrisk (man-pages) wrote:

>> Hi Alex,

>>

>> On 9/25/20 9:31 AM, Alejandro Colomar wrote:

>>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>

>>> ---

>>>   man2/seccomp.2 | 2 +-

>>>   1 file changed, 1 insertion(+), 1 deletion(-)

>>>

>>> diff --git a/man2/seccomp.2 b/man2/seccomp.2

>>> index 58033da1c..d6b856c32 100644

>>> --- a/man2/seccomp.2

>>> +++ b/man2/seccomp.2

>>> @@ -1101,7 +1101,7 @@ install_filter(int syscall_nr, int t_arch, int f_errno)

>>>       };

>>>   

>>>       struct sock_fprog prog = {

>>> -        .len = (unsigned short) (sizeof(filter) / sizeof(filter[0])),

>>> +        .len = sizeof(filter) / sizeof(filter[0]),

>>>           .filter = filter,

>>>       };

>>

>> I have a small doubt about this change. With the change,

>> there are no compilation warnings.

>>

>> But, if we change the code to something slightly different:

>>

>> [[

>>      size_t x = (sizeof(filter) / sizeof(filter[0]));

>>      struct sock_fprog prog = {

>>          .len = x,

>>          .filter = filter,

>>      };

>> ]]

>>

>> The "cc -Wconversion" gives us the following warning:

>>

>>      warning: conversion from ‘size_t’ {aka ‘long unsigned int’}

>>      to ‘short unsigned int’ may change value

>>

>> Presumably we don't get a warning for an assignment of the form

>>

>>      .len = (sizeof(filter) / sizeof(filter[0]))

>>

>> because the compiler is smart enough to work out that the

>> value of the constant expression is within the range of

>> "unsigned short".

>>

>> Your thoughts?

> 

> Hi Michael,

> 

> I'd say that the cast doesn't fix any problems at all.  It silences a 

> valid warning, and I'd use a pragma for that (to be more explicit about 

> the intention of silencing a warning) if I do want -Wconversion enabled 

> (which usually I don't want, because it's too noisy) and I'm sure that 

> this won't overflow.  I'd limit the use casts to only when I *really* 

> need to.

> 

> I guess that if you enable -O3, the warning will vanish again because 

> the compiler will optimize away 'x' (but I didn't test).


Fair enough! I've applied the patch. Thanks!

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Patch

diff --git a/man2/seccomp.2 b/man2/seccomp.2
index 58033da1c..d6b856c32 100644
--- a/man2/seccomp.2
+++ b/man2/seccomp.2
@@ -1101,7 +1101,7 @@  install_filter(int syscall_nr, int t_arch, int f_errno)
     };
 
     struct sock_fprog prog = {
-        .len = (unsigned short) (sizeof(filter) / sizeof(filter[0])),
+        .len = sizeof(filter) / sizeof(filter[0]),
         .filter = filter,
     };