Re: [Qemu-devel] [PATCH 1/1] block: check full backing filename when searching protocol filenames

[Max Reitz]

On 25.01.2017 19:24, Eric Blake wrote:
> On 01/25/2017 11:22 AM, Jeff Cody wrote:
>> In bdrv_find_backing_image(), if we are searching an image for a backing
>> file that contains a protocol, we currently only compare unmodified
>> paths.
>>
>> However, some management software will change the backing filename to be
>> a relative filename in a path.  QEMU is able to handle this fine,
>> because internally it will use path_combine to put together the full
>> protocol URI.
>>
>> However, this can lead to an inability to match an image during a QAPI
>> command that needs to use bdrv_find_backing_image() to find the image,
>> when it is searched by the full URI.
>>
>> When searching for a protocol filename, if the straight comparison
>> fails, this patch will also compare against the full backing filename to
>> see if that is a match.
>>
>> Signed-off-by: Jeff Cody <address@hidden>
>> ---
>>  block.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
> 
> Does this overlap with Max' work on 'block: Fix some filename generation
> issues'?  But in isolation, it looks okay to me,

Yes, it does, but it's not too hard to fix up in my series. Better get
this in first and then do the trivial change on my side.

> Reviewed-by: Eric Blake <address@hidden>

Noted :-)

Max

>>
>> diff --git a/block.c b/block.c
>> index 39ddea3..a173afc 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3145,6 +3145,7 @@ BlockDriverState 
>> *bdrv_find_backing_image(BlockDriverState *bs,
>>      int is_protocol = 0;
>>      BlockDriverState *curr_bs = NULL;
>>      BlockDriverState *retval = NULL;
>> +    Error *local_error = NULL;
> 
> I might have scoped this...
> 
>>  
>>      if (!bs || !bs->drv || !backing_file) {
>>          return NULL;
>> @@ -3165,6 +3166,17 @@ BlockDriverState 
>> *bdrv_find_backing_image(BlockDriverState *bs,
>>                  retval = curr_bs->backing->bs;
>>                  break;
>>              }
> 
> ...as the first line within the if that uses it. But no big deal.
> 
>> +            /* Also check against the full backing filename for the image */
>> +            bdrv_get_full_backing_filename(curr_bs, backing_file_full, 
>> PATH_MAX,
>> +                                           &local_error);
>> +            if (local_error == NULL) {
>> +                if (strcmp(backing_file, backing_file_full) == 0) {
>> +                    retval = curr_bs->backing->bs;
>> +                    break;
>> +                }
>> +            } else {
>> +                error_free(local_error);
>> +            }
>>          } else {
>>              /* If not an absolute filename path, make it relative to the 
>> current
>>               * image's filename path */
>>
>

signature.asc
Description: OpenPGP digital signature