[SLOF] [PATCH slof 08/13] libnet: Compile with -Wextra

[Alexey Kardashevskiy]

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

-- 
Alexey