Re: [Qemu-block] [PATCH v6 09/25] block: Fix bdrv_find_backing_image()

[Max Reitz]

On 2017-10-30 15:47, Alberto Garcia wrote:
> On Fri 29 Sep 2017 06:53:31 PM CEST, Max Reitz wrote:
>> @@ -4096,22 +4086,31 @@ BlockDriverState 
>> *bdrv_find_backing_image(BlockDriverState *bs,
>>          } else {
>>              /* If not an absolute filename path, make it relative to the 
>> current
>>               * image's filename path */
>> -            path_combine_deprecated(filename_tmp, PATH_MAX, 
>> curr_bs->filename,
>> -                                    backing_file);
>> +            filename_tmp = bdrv_make_absolute_filename(curr_bs, 
>> backing_file,
>> +                                                       NULL);
>> +            if (!filename_tmp) {
>> +                continue;
>> +            }
>>  
>>              /* We are going to compare absolute pathnames */
>>              if (!realpath(filename_tmp, filename_full)) {
>> +                g_free(filename_tmp);
>>                  continue;
>>              }
>> +            g_free(filename_tmp);
> 
> This feels a bit too verbose, doesn't it? (especially because you're
> doing the same thing twice, see below). It could be made a bit shorter,
> something like:
> 
>     bool have_filename = filename_tmp && realpath(filename_tmp, 
> filename_full);
>     g_free(filename_tmp);
>     if (!have_filename) {
>          continue;
>     }

Well, but then again

if (filename_tmp && realpath(filename_tmp, filename_full)) {
    g_free(filename_tmp);
    continue;
}
g_free(filename_tmp);

has the same length. :-)

(Actually, overall it'd be one line shorter because I'd have to use an
own line for the "bool have_filename" declaration (to put it at the
start of the block).)

So I'll do that, yes.

Thanks for reviewing (and this suggestion)!

Max

> 
>> -            path_combine_deprecated(filename_tmp, PATH_MAX, 
>> curr_bs->filename,
>> -                                    curr_bs->backing_file);
>> +            filename_tmp = bdrv_get_full_backing_filename(curr_bs, NULL);
>> +            if (!filename_tmp) {
>> +                continue;
>> +            }
>>  
>>              if (!realpath(filename_tmp, backing_file_full)) {
>> +                g_free(filename_tmp);
>>                  continue;
>>              }
>> +            g_free(filename_tmp);
> 
> Berto
>

signature.asc
Description: OpenPGP digital signature