diff mbox series

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

Message ID 20200925073140.173394-9-colomar.6.4.3@gmail.com
State Not Applicable
Headers show
Series Add types, and some fixes | expand

Commit Message

Alejandro Colomar Sept. 25, 2020, 7:31 a.m. UTC
Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/seccomp.2 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Kerrisk \(man-pages\) Sept. 25, 2020, 8:34 a.m. UTC | #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
Alejandro Colomar Sept. 25, 2020, 8:42 a.m. UTC | #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
> 
>
Michael Kerrisk \(man-pages\) Sept. 25, 2020, 9:23 a.m. UTC | #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
diff mbox series

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,
     };