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

["Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com>]

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/