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

[Max Reitz]

On 2017-11-02 16:45, Max Reitz wrote:
> 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)) {

(err, I mean ||, of course.)

>     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