Re: [Qemu-devel] [PATCH 07/10] linux-user: init_guest_space: Clean up control flow a bit

[Laurent Vivier <laurent@vivier.eu>]

Le 09/03/2018 à 21:37, Laurent Vivier a écrit :
> Le 28/12/2017 à 19:08, Luke Shumaker a écrit :
>> From: Luke Shumaker <lukeshu@parabola.nu>
>>
>> Instead of doing
>>
>>         if (check1) {
>>             if (check2) {
>>                success;
>>             }
>>         }
>>
>>         retry;
>>
>> Do a clearer
>>
>>         if (!check1) {
>>            goto try_again;
>>         }
>>
>>         if (!check2) {
>>            goto try_again;
>>         }
>>
>>         success;
>>
>>     try_again:
>>         retry;
>>
>> Besides being clearer, this makes it easier to insert more checks that
>> need to trigger a retry on check failure, or rearrange them, or anything
>> like that.
>>
>> Because some indentation is changing, "ignore space change" may be useful
>> for viewing this patch.
>>
>> Signed-off-by: Luke Shumaker <lukeshu@parabola.nu>
>> ---
>>  linux-user/elfload.c | 34 +++++++++++++++++++---------------
>>  1 file changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
>> index b560f5d6fe..5c0ad65611 100644
>> --- a/linux-user/elfload.c
>> +++ b/linux-user/elfload.c
>> @@ -1906,24 +1906,28 @@ unsigned long init_guest_space(unsigned long host_start,
>>          }
>>  
>>          /* Check to see if the address is valid.  */
>> -        if (!host_start || aligned_start == current_start) {
>> +        if (host_start && aligned_start != current_start) {
>> +            goto try_again;
>> +        }
>> +
>>  #if defined(TARGET_ARM) && !defined(TARGET_AARCH64)
>> -            /* On 32-bit ARM, we need to also be able to map the commpage.  */
>> -            int valid = init_guest_commpage(aligned_start - guest_start,
>> -                                            aligned_size + guest_start);
>> -            if (valid == 1) {
>> -                break;
>> -            } else if (valid == -1) {
>> -                munmap((void *)real_start, real_size);
>> -                return (unsigned long)-1;
>> -            }
>> -            /* valid == 0, so try again. */
>> -#else
>> -            /* On other architectures, whatever we have here is fine.  */
>> -            break;
>> -#endif
>> +        /* On 32-bit ARM, we need to also be able to map the commpage.  */
>> +        int valid = init_guest_commpage(aligned_start - guest_start,
>> +                                        aligned_size + guest_start);
>> +        if (valid == -1) {
>> +            munmap((void *)real_start, real_size);
>> +            return (unsigned long)-1;
>> +        } else if (valid == -1) {
> 
> I think it should be "if (valid == 0)" here.

Any comment?

Thanks,
Laurent