Re: [Qemu-devel] [PATCH for 2.10 26/35] linux-user: use is_error() to avoid warnings and make the code clearer

[Laurent Vivier]

Le 04/06/2018 à 16:20, Philippe Mathieu-Daudé a écrit :
> On 06/03/2018 08:33 PM, Laurent Vivier wrote:
>> Le 29/05/2018 à 17:19, Laurent Vivier a écrit :
>>> Le 29/05/2018 à 16:25, Philippe Mathieu-Daudé a écrit :
>>>> Hi Laurent,
>>>
>>> Hi Philippe,
>>>
>>>> On 07/24/2017 04:16 PM, Laurent Vivier wrote:
>>>>> Le 24/07/2017 à 20:27, Philippe Mathieu-Daudé a écrit :
>>>>>> linux-user/flatload.c:740:9: warning: Loss of sign in implicit conversion
>>>>>>     if (res > (unsigned long)-4096)
>>>>>>         ^~~
>>>>>>
>>>>>> Reported-by: Clang Static Analyzer
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <address@hidden>
>>>>>
>>>>> Reviewed-by: Laurent Vivier <address@hidden>
>>>>>
>>>>>> ---
>>>>>>  linux-user/flatload.c | 15 +++++++++------
>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>
>>>>>> diff --git a/linux-user/flatload.c b/linux-user/flatload.c
>>>>>> index a35a560904..10c529910f 100644
>>>>>> --- a/linux-user/flatload.c
>>>>>> +++ b/linux-user/flatload.c
>>>>>> @@ -224,8 +224,9 @@ static int decompress_exec(
>>>>>>                  ret = bprm->file->f_op->read(bprm->file, buf, LBUFSIZE, 
>>>>>> &fpos);
>>>>>>                  if (ret <= 0)
>>>>>>                          break;
>>>>>> -                if (ret >= (unsigned long) -4096)
>>>>>> +                if (is_error(ret)) {
>>>>>>                          break;
>>>>>> +                }
>>>>>>                  len -= ret;
>>>>>>  
>>>>>>                  strm.next_in = buf;
>>>>>> @@ -283,8 +284,7 @@ calc_reloc(abi_ulong r, struct lib_info *p, int 
>>>>>> curid, int internalp)
>>>>>>                      "in same module (%d != %d)\n",
>>>>>>                      (unsigned) r, curid, id);
>>>>>>              goto failed;
>>>>>> -        } else if ( ! p[id].loaded &&
>>>>>> -                    load_flat_shared_library(id, p) > (unsigned long) 
>>>>>> -4096) {
>>>>>> +        } else if (!p[id].loaded && 
>>>>>> is_error(load_flat_shared_library(id, p))) {
>>>>>>              fprintf(stderr, "BINFMT_FLAT: failed to load library %d\n", 
>>>>>> id);
>>>>>>              goto failed;
>>>>>>          }
>>>>>> @@ -523,9 +523,10 @@ static int load_flat_file(struct linux_binprm * 
>>>>>> bprm,
>>>>>>                  fpos = 0;
>>>>>>                  result = bprm->file->f_op->read(bprm->file,
>>>>>>                                  (char *) textpos, text_len, &fpos);
>>>>>> -                if (result < (unsigned long) -4096)
>>>>>> +                if (!is_error(result)) {
>>>>>>                          result = decompress_exec(bprm, text_len, (char 
>>>>>> *) datapos,
>>>>>>                                           data_len + (relocs * 
>>>>>> sizeof(unsigned long)), 0);
>>>>>> +                }
>>>>>>          }
>>>>>>          else
>>>>>>  #endif
>>>>>> @@ -693,8 +694,9 @@ static int load_flat_shared_library(int id, struct 
>>>>>> lib_info *libs)
>>>>>>  
>>>>>>          res = prepare_binprm(&bprm);
>>>>>>  
>>>>>> -        if (res <= (unsigned long)-4096)
>>>>>> +        if (!is_error(res)) {
>>>>>>                  res = load_flat_file(&bprm, libs, id, NULL);
>>>>>> +        }
>>>>>>          if (bprm.file) {
>>>>>>                  allow_write_access(bprm.file);
>>>>>>                  fput(bprm.file);
>>>>>> @@ -737,8 +739,9 @@ int load_flt_binary(struct linux_binprm *bprm, 
>>>>>> struct image_info *info)
>>>>>>  
>>>>>>  
>>>>>>      res = load_flat_file(bprm, libinfo, 0, &stack_len);
>>>>>> -    if (res > (unsigned long)-4096)
>>>>>> +    if (is_error(res)) {
>>>>>>              return res;
>>>>>> +    }
>>>>>>  
>>>>>>      /* Update data segment pointers for all libraries */
>>>>>>      for (i=0; i<MAX_SHARED_LIBS; i++) {
>>>>>>
>>>>
>>>> Can you take this via your linux-user tree?
>>>>
>>>
>>> Applied, thanks.
>>
>> Unapplied, it needs a rebase:
> 
> No rebase required, it just need the previous patch applied too :)
> But I didn't think of explicit it :/
> Can you take both of them directly or do you prefer I RESEND?
> 
>>
>> qemu/linux-user/flatload.c: In function 'load_flt_binary':
>> qemu/linux-user/flatload.c:742:9: error: implicit declaration of
>> function 'is_error'; did you mean 'g_error'?
>> [-Werror=implicit-function-declaration]
>>      if (is_error(res)) {
>>          ^~~~~~~~
>>          g_error
>> qemu/linux-user/flatload.c:742:9: error: nested extern declaration of
>> 'is_error' [-Werror=nested-externs]
>>

I have already my next pull-request in test phase without this one, so
the better is to resend a series with the patches you want to be merged.

Thanks,
Laurent