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

  Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
---
 man2/seccomp.2 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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
  

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

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