| Message ID | 20210127085752.120571-9-aik@ozlabs.ru |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Compile with -Wextra | expand |
On 27/01/2021 09.57, Alexey Kardashevskiy wrote: > -Wextra enables a bunch of rather useful checks which this fixes. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > lib/libnet/netapps.h | 4 ++-- > lib/libnet/netload.c | 4 ++-- > lib/libnet/ping.c | 6 +++--- > lib/libnet/pxelinux.c | 5 +++-- > slof/ppc64.c | 12 +++++++++--- > 5 files changed, 19 insertions(+), 12 deletions(-) > > diff --git a/lib/libnet/netapps.h b/lib/libnet/netapps.h > index 6e00466de5a9..722ca7ff7ab4 100644 > --- a/lib/libnet/netapps.h > +++ b/lib/libnet/netapps.h > @@ -18,8 +18,8 @@ > > struct filename_ip; > > -extern int netload(char *buffer, int len, char *args_fs, int alen); > -extern int ping(char *args_fs, int alen); > +extern int netload(char *buffer, int len, char *args_fs, unsigned alen); > +extern int ping(char *args_fs, unsigned alen); > extern int dhcp(char *ret_buffer, struct filename_ip *fn_ip, > unsigned int retries, int flags); > > diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c > index 2dc00f00097d..ae169f3e015d 100644 > --- a/lib/libnet/netload.c > +++ b/lib/libnet/netload.c > @@ -528,7 +528,7 @@ static void encode_response(char *pkt_buffer, size_t size, int ip_init) > } > } > > -int netload(char *buffer, int len, char *args_fs, int alen) > +int netload(char *buffer, int len, char *args_fs, unsigned alen) > { > int rc, filename_len; > filename_ip_t fn_ip; > @@ -566,7 +566,7 @@ int netload(char *buffer, int len, char *args_fs, int alen) > set_timer(TICKS_SEC); > while (get_timer() > 0); > } > - fd_device = socket(0, 0, 0, (char*) own_mac); > + fd_device = socket(AF_INET, SOCK_DGRAM, 0, (char*) own_mac); > if(fd_device != -2) > break; > if(getchar() == 27) { > diff --git a/lib/libnet/ping.c b/lib/libnet/ping.c > index 51db061ecaff..476c4fe44ba7 100644 > --- a/lib/libnet/ping.c > +++ b/lib/libnet/ping.c > @@ -106,7 +106,7 @@ parse_args(const char *args, struct ping_args *ping_args) > return 0; > } > > -int ping(char *args_fs, int alen) > +int ping(char *args_fs, unsigned alen) > { > short arp_failed = 0; > filename_ip_t fn_ip; > @@ -119,7 +119,7 @@ int ping(char *args_fs, int alen) > > memset(&ping_args, 0, sizeof(struct ping_args)); > > - if (alen <= 0 || alen >= sizeof(args) - 1) { > + if (alen >= sizeof(args) - 1) { What about alen == 0 ? > usage(); > return -1; > } > @@ -137,7 +137,7 @@ int ping(char *args_fs, int alen) > > /* Get mac_addr from device */ > printf("\n Reading MAC address from device: "); > - fd_device = socket(0, 0, 0, (char *) own_mac); > + fd_device = socket(AF_INET, SOCK_DGRAM, 0, (char *) own_mac); > if (fd_device == -1) { > printf("\nE3000: Could not read MAC address\n"); > return -100; > diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c > index c4ac5d5a078a..be1939f24d8b 100644 > --- a/lib/libnet/pxelinux.c > +++ b/lib/libnet/pxelinux.c > @@ -55,7 +55,8 @@ static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len, > static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid, > int retries, char *cfgbuf, int cfgbufsize) > { > - int rc, idx; > + int rc; > + unsigned idx; > char *baseptr; > > /* Did we get a usable base directory via DHCP? */ > @@ -77,7 +78,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > ++baseptr; > /* Check that we've got enough space to store "pxelinux.cfg/" > * and the UUID (which is the longest file name) there */ > - if (baseptr - fn_ip->filename > sizeof(fn_ip->filename) - 50) { > + if (baseptr - fn_ip->filename > (int)(sizeof(fn_ip->filename) - 50)) { > puts("Error: The bootfile string is too long for " > "deriving the pxelinux.cfg file name from it."); > return -1; > diff --git a/slof/ppc64.c b/slof/ppc64.c > index 83a8e82cfb42..ca6cafffc35d 100644 > --- a/slof/ppc64.c > +++ b/slof/ppc64.c > @@ -144,6 +144,12 @@ int socket(int domain, int type, int proto, char *mac_addr) > int prop_len; > int fd; > > + if (!(domain == AF_INET || domain == AF_INET6)) Better: if (domain != AF_INET && domain != AF_INET6)) > + return -1; > + > + if (type != SOCK_DGRAM || proto != 0) > + return -1; I think these changes are not necessary anymore since you're compiling with -Wno-unused-parameter ... so either drop these or put them into a separate patch? Thomas
On Thu, Jan 28, 2021 at 04:19:22PM +0100, Thomas Huth wrote: > >+ if (!(domain == AF_INET || domain == AF_INET6)) > > Better: > > if (domain != AF_INET && domain != AF_INET6)) That is exactly the same for the compiler, so the only thing that matters is what is easier to read and understand. I find the latter much worse usually (and that would be even more obvious if C had an "unless" keyword), but of course opinions differ. Ages ago people did De Morgan's rules manually because the compilers of that time were not up to snuff. That time is long past, but old habits are hard to break. You can go back and forth on this infinitely :-) Segher
On 29/01/2021 02:19, Thomas Huth wrote: > On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >> -Wextra enables a bunch of rather useful checks which this fixes. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> lib/libnet/netapps.h | 4 ++-- >> lib/libnet/netload.c | 4 ++-- >> lib/libnet/ping.c | 6 +++--- >> lib/libnet/pxelinux.c | 5 +++-- >> slof/ppc64.c | 12 +++++++++--- >> 5 files changed, 19 insertions(+), 12 deletions(-) >> >> diff --git a/lib/libnet/netapps.h b/lib/libnet/netapps.h >> index 6e00466de5a9..722ca7ff7ab4 100644 >> --- a/lib/libnet/netapps.h >> +++ b/lib/libnet/netapps.h >> @@ -18,8 +18,8 @@ >> struct filename_ip; >> -extern int netload(char *buffer, int len, char *args_fs, int alen); >> -extern int ping(char *args_fs, int alen); >> +extern int netload(char *buffer, int len, char *args_fs, unsigned alen); >> +extern int ping(char *args_fs, unsigned alen); >> extern int dhcp(char *ret_buffer, struct filename_ip *fn_ip, >> unsigned int retries, int flags); >> diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c >> index 2dc00f00097d..ae169f3e015d 100644 >> --- a/lib/libnet/netload.c >> +++ b/lib/libnet/netload.c >> @@ -528,7 +528,7 @@ static void encode_response(char *pkt_buffer, >> size_t size, int ip_init) >> } >> } >> -int netload(char *buffer, int len, char *args_fs, int alen) >> +int netload(char *buffer, int len, char *args_fs, unsigned alen) >> { >> int rc, filename_len; >> filename_ip_t fn_ip; >> @@ -566,7 +566,7 @@ int netload(char *buffer, int len, char *args_fs, >> int alen) >> set_timer(TICKS_SEC); >> while (get_timer() > 0); >> } >> - fd_device = socket(0, 0, 0, (char*) own_mac); >> + fd_device = socket(AF_INET, SOCK_DGRAM, 0, (char*) own_mac); >> if(fd_device != -2) >> break; >> if(getchar() == 27) { >> diff --git a/lib/libnet/ping.c b/lib/libnet/ping.c >> index 51db061ecaff..476c4fe44ba7 100644 >> --- a/lib/libnet/ping.c >> +++ b/lib/libnet/ping.c >> @@ -106,7 +106,7 @@ parse_args(const char *args, struct ping_args >> *ping_args) >> return 0; >> } >> -int ping(char *args_fs, int alen) >> +int ping(char *args_fs, unsigned alen) >> { >> short arp_failed = 0; >> filename_ip_t fn_ip; >> @@ -119,7 +119,7 @@ int ping(char *args_fs, int alen) >> memset(&ping_args, 0, sizeof(struct ping_args)); >> - if (alen <= 0 || alen >= sizeof(args) - 1) { >> + if (alen >= sizeof(args) - 1) { > > What about alen == 0 ? Ah, missed. > >> usage(); >> return -1; >> } >> @@ -137,7 +137,7 @@ int ping(char *args_fs, int alen) >> /* Get mac_addr from device */ >> printf("\n Reading MAC address from device: "); >> - fd_device = socket(0, 0, 0, (char *) own_mac); >> + fd_device = socket(AF_INET, SOCK_DGRAM, 0, (char *) own_mac); >> > if (fd_device == -1) { >> printf("\nE3000: Could not read MAC address\n"); >> return -100; >> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c >> index c4ac5d5a078a..be1939f24d8b 100644 >> --- a/lib/libnet/pxelinux.c >> +++ b/lib/libnet/pxelinux.c >> @@ -55,7 +55,8 @@ static int pxelinux_tftp_load(filename_ip_t *fnip, >> void *buffer, int len, >> static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, >> const char *uuid, >> int retries, char *cfgbuf, int cfgbufsize) >> { >> - int rc, idx; >> + int rc; >> + unsigned idx; >> char *baseptr; >> /* Did we get a usable base directory via DHCP? */ >> @@ -77,7 +78,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, >> uint8_t *mac, const char *uui >> ++baseptr; >> /* Check that we've got enough space to store "pxelinux.cfg/" >> * and the UUID (which is the longest file name) there */ >> - if (baseptr - fn_ip->filename > sizeof(fn_ip->filename) - 50) { >> + if (baseptr - fn_ip->filename > (int)(sizeof(fn_ip->filename) >> - 50)) { >> puts("Error: The bootfile string is too long for " >> "deriving the pxelinux.cfg file name from it."); >> return -1; >> diff --git a/slof/ppc64.c b/slof/ppc64.c >> index 83a8e82cfb42..ca6cafffc35d 100644 >> --- a/slof/ppc64.c >> +++ b/slof/ppc64.c >> @@ -144,6 +144,12 @@ int socket(int domain, int type, int proto, char >> *mac_addr) >> int prop_len; >> int fd; >> + if (!(domain == AF_INET || domain == AF_INET6)) > > Better: > > if (domain != AF_INET && domain != AF_INET6)) No it is not :) > >> + return -1; >> + >> + if (type != SOCK_DGRAM || proto != 0) >> + return -1; > > I think these changes are not necessary anymore since you're compiling > with -Wno-unused-parameter ... so either drop these or put them into a > separate patch? Well, it is also self documenting what we do implement in slof. And a little bit less work when I remove -Wno-unused-parameter later. I'll make it a separate patch. Thanks, > > Thomas > >
On 29/01/2021 02.59, Alexey Kardashevskiy wrote: > > > On 29/01/2021 02:19, Thomas Huth wrote: >> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>> -Wextra enables a bunch of rather useful checks which this fixes. [...] >>> diff --git a/slof/ppc64.c b/slof/ppc64.c >>> index 83a8e82cfb42..ca6cafffc35d 100644 >>> --- a/slof/ppc64.c >>> +++ b/slof/ppc64.c >>> @@ -144,6 +144,12 @@ int socket(int domain, int type, int proto, char >>> *mac_addr) >>> int prop_len; >>> int fd; >>> + if (!(domain == AF_INET || domain == AF_INET6)) >> >> Better: >> >> if (domain != AF_INET && domain != AF_INET6)) > > No it is not :) > >> >>> + return -1; >>> + >>> + if (type != SOCK_DGRAM || proto != 0) >>> + return -1; >> >> I think these changes are not necessary anymore since you're compiling >> with -Wno-unused-parameter ... so either drop these or put them into a >> separate patch? > > > Well, it is also self documenting what we do implement in slof. And a little > bit less work when I remove -Wno-unused-parameter later. I'll make it a > separate patch. Thanks, I'd rather vote for keeping -Wno-unused-parameter. Otherwise you're always forced to write silly code or use __attribute__((unused)) when a function has to match a certain parameter list, e.g. because it is used as a function pointer later, and that's rather annoying. Thomas
On 29/01/2021 17:42, Thomas Huth wrote: > On 29/01/2021 02.59, Alexey Kardashevskiy wrote: >> >> >> On 29/01/2021 02:19, Thomas Huth wrote: >>> On 27/01/2021 09.57, Alexey Kardashevskiy wrote: >>>> -Wextra enables a bunch of rather useful checks which this fixes. > [...] >>>> diff --git a/slof/ppc64.c b/slof/ppc64.c >>>> index 83a8e82cfb42..ca6cafffc35d 100644 >>>> --- a/slof/ppc64.c >>>> +++ b/slof/ppc64.c >>>> @@ -144,6 +144,12 @@ int socket(int domain, int type, int proto, >>>> char *mac_addr) >>>> int prop_len; >>>> int fd; >>>> + if (!(domain == AF_INET || domain == AF_INET6)) >>> >>> Better: >>> >>> if (domain != AF_INET && domain != AF_INET6)) >> >> No it is not :) >> >>> >>>> + return -1; >>>> + >>>> + if (type != SOCK_DGRAM || proto != 0) >>>> + return -1; >>> >>> I think these changes are not necessary anymore since you're >>> compiling with -Wno-unused-parameter ... so either drop these or put >>> them into a separate patch? >> >> >> Well, it is also self documenting what we do implement in slof. And a >> little bit less work when I remove -Wno-unused-parameter later. I'll >> make it a separate patch. Thanks, > > I'd rather vote for keeping -Wno-unused-parameter. Otherwise you're > always forced to write silly code or use __attribute__((unused)) when a > function has to match a certain parameter list, e.g. because it is used > as a function pointer later, and that's rather annoying. Then why pass them around? I understand if it is a callback of some sort and not all instances may need additional parameters but this is not the case here. And passing around 3 zeroes - domain/type/proto is as stupid, I am basically commenting on what is supported in socket(). Thanks,
Hi! On Sun, Jan 31, 2021 at 02:54:08PM +1100, Alexey Kardashevskiy wrote: > >I'd rather vote for keeping -Wno-unused-parameter. Otherwise you're > >always forced to write silly code or use __attribute__((unused)) when a > >function has to match a certain parameter list, e.g. because it is used > >as a function pointer later, and that's rather annoying. > > Then why pass them around? I understand if it is a callback of some sort > and not all instances may need additional parameters but this is not the > case here. > > And passing around 3 zeroes - domain/type/proto is as stupid, I am > basically commenting on what is supported in socket(). Thanks, Fwiw, from GCC 11 on (so not too useful for you yet) you can leave out the name of unused parameters, just like in C++. (Older compilers say "error: parameter name omitted"). If you want to name it anyway (as documentation perhaps), you can comment that, like void f(int a, int /*b*/) { return 42*a; } but in cases like yours you can often just say void f(int a, int) { return 42*a; } Segher
diff --git a/lib/libnet/netapps.h b/lib/libnet/netapps.h index 6e00466de5a9..722ca7ff7ab4 100644 --- a/lib/libnet/netapps.h +++ b/lib/libnet/netapps.h @@ -18,8 +18,8 @@ struct filename_ip; -extern int netload(char *buffer, int len, char *args_fs, int alen); -extern int ping(char *args_fs, int alen); +extern int netload(char *buffer, int len, char *args_fs, unsigned alen); +extern int ping(char *args_fs, unsigned alen); extern int dhcp(char *ret_buffer, struct filename_ip *fn_ip, unsigned int retries, int flags); diff --git a/lib/libnet/netload.c b/lib/libnet/netload.c index 2dc00f00097d..ae169f3e015d 100644 --- a/lib/libnet/netload.c +++ b/lib/libnet/netload.c @@ -528,7 +528,7 @@ static void encode_response(char *pkt_buffer, size_t size, int ip_init) } } -int netload(char *buffer, int len, char *args_fs, int alen) +int netload(char *buffer, int len, char *args_fs, unsigned alen) { int rc, filename_len; filename_ip_t fn_ip; @@ -566,7 +566,7 @@ int netload(char *buffer, int len, char *args_fs, int alen) set_timer(TICKS_SEC); while (get_timer() > 0); } - fd_device = socket(0, 0, 0, (char*) own_mac); + fd_device = socket(AF_INET, SOCK_DGRAM, 0, (char*) own_mac); if(fd_device != -2) break; if(getchar() == 27) { diff --git a/lib/libnet/ping.c b/lib/libnet/ping.c index 51db061ecaff..476c4fe44ba7 100644 --- a/lib/libnet/ping.c +++ b/lib/libnet/ping.c @@ -106,7 +106,7 @@ parse_args(const char *args, struct ping_args *ping_args) return 0; } -int ping(char *args_fs, int alen) +int ping(char *args_fs, unsigned alen) { short arp_failed = 0; filename_ip_t fn_ip; @@ -119,7 +119,7 @@ int ping(char *args_fs, int alen) memset(&ping_args, 0, sizeof(struct ping_args)); - if (alen <= 0 || alen >= sizeof(args) - 1) { + if (alen >= sizeof(args) - 1) { usage(); return -1; } @@ -137,7 +137,7 @@ int ping(char *args_fs, int alen) /* Get mac_addr from device */ printf("\n Reading MAC address from device: "); - fd_device = socket(0, 0, 0, (char *) own_mac); + fd_device = socket(AF_INET, SOCK_DGRAM, 0, (char *) own_mac); if (fd_device == -1) { printf("\nE3000: Could not read MAC address\n"); return -100; diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c index c4ac5d5a078a..be1939f24d8b 100644 --- a/lib/libnet/pxelinux.c +++ b/lib/libnet/pxelinux.c @@ -55,7 +55,8 @@ static int pxelinux_tftp_load(filename_ip_t *fnip, void *buffer, int len, static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid, int retries, char *cfgbuf, int cfgbufsize) { - int rc, idx; + int rc; + unsigned idx; char *baseptr; /* Did we get a usable base directory via DHCP? */ @@ -77,7 +78,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui ++baseptr; /* Check that we've got enough space to store "pxelinux.cfg/" * and the UUID (which is the longest file name) there */ - if (baseptr - fn_ip->filename > sizeof(fn_ip->filename) - 50) { + if (baseptr - fn_ip->filename > (int)(sizeof(fn_ip->filename) - 50)) { puts("Error: The bootfile string is too long for " "deriving the pxelinux.cfg file name from it."); return -1; diff --git a/slof/ppc64.c b/slof/ppc64.c index 83a8e82cfb42..ca6cafffc35d 100644 --- a/slof/ppc64.c +++ b/slof/ppc64.c @@ -144,6 +144,12 @@ int socket(int domain, int type, int proto, char *mac_addr) int prop_len; int fd; + if (!(domain == AF_INET || domain == AF_INET6)) + return -1; + + if (type != SOCK_DGRAM || proto != 0) + return -1; + /* search free file descriptor (and skip stdio handlers) */ for (fd = 3; fd < FILEIO_MAX; ++fd) { if (fd_array[fd].type == FILEIO_TYPE_EMPTY) { @@ -217,7 +223,7 @@ int close(int fd) */ int recv(int fd, void *buf, int len, int flags) { - if (!is_valid_fd(fd)) + if (!is_valid_fd(fd) || flags) return -1; forth_push((unsigned long)buf); @@ -237,7 +243,7 @@ int recv(int fd, void *buf, int len, int flags) */ int send(int fd, const void *buf, int len, int flags) { - if (!is_valid_fd(fd)) + if (!is_valid_fd(fd) || flags) return -1; forth_push((unsigned long)buf); @@ -258,7 +264,7 @@ int send(int fd, const void *buf, int len, int flags) ssize_t read(int fd, void *buf, size_t len) { char *ptr = (char *)buf; - int cnt = 0; + unsigned cnt = 0; char code; if (fd == 0 || fd == 2) {
-Wextra enables a bunch of rather useful checks which this fixes. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- lib/libnet/netapps.h | 4 ++-- lib/libnet/netload.c | 4 ++-- lib/libnet/ping.c | 6 +++--- lib/libnet/pxelinux.c | 5 +++-- slof/ppc64.c | 12 +++++++++--- 5 files changed, 19 insertions(+), 12 deletions(-)