[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 09/25] block: Fix bdrv_find_backing_image()
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v6 09/25] block: Fix bdrv_find_backing_image() |
Date: |
Thu, 2 Nov 2017 16:59:07 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
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