Message ID | 20200925073140.173394-9-colomar.6.4.3@gmail.com |
---|---|
State | New |
Headers | show |
Series | Add types, and some fixes | expand |
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
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, };
Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com> --- man2/seccomp.2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)