Re: [Qemu-devel] [PATCH 1/2] block: make bdrv_find_backing_image compare canonical filenames

[Jeff Cody <jcody@redhat.com>]

On 10/10/2012 03:34 AM, Paolo Bonzini wrote:
> Il 10/10/2012 07:56, Jeff Cody ha scritto:
>> Currently, bdrv_find_backing_image compares bs->backing_file with
>> what is passed in as a backing_file name.  Mismatches may occur,
>> however, when bs->backing_file and backing_file are both not
>> absolute or relative.
>>
>> Use path_combine() to make sure any relative backing filenames are
>> relative to the current image filename being searched, and then use
>> realpath() to make all comparisons based on absolute filenames.
>>
>> This also changes bdrv_find_backing_image to no longer be recursive,
>> but iterative.
>>
>> Signed-off-by: Jeff Cody <jcody@redhat.com>
>> ---
>>  block.c | 38 ++++++++++++++++++++++++++++++++------
>>  1 file changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index e95f613..641b8fa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3123,18 +3123,44 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>      return -ENOTSUP;
>>  }
>>  
>> +/* backing_file can either be relative, or absolute.  If it is
>> + * relative, it must be relative to the chain.  So, passing in
>> + * bs->filename from a BDS as backing_file should not be done,
>> + * as that may be relative to the CWD rather than the chain. */
>>  BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
>>          const char *backing_file)
>>  {
>> -    if (!bs->drv) {
>> +    char filename_full[PATH_MAX];
>> +    char backing_file_full[PATH_MAX];
>> +    char filename_tmp[PATH_MAX];
>> +    BlockDriverState *curr_bs = NULL;
>> +
>> +    if (!bs || !bs->drv) {
>>          return NULL;
>>      }
>>  
>> -    if (bs->backing_hd) {
>> -        if (strcmp(bs->backing_file, backing_file) == 0) {
>> -            return bs->backing_hd;
>> -        } else {
>> -            return bdrv_find_backing_image(bs->backing_hd, backing_file);
>> +    for (curr_bs = bs; curr_bs->backing_hd; curr_bs = curr_bs->backing_hd) {
>> +        /* If not an absolute filename path, make it relative to the current
>> +         * image's filename path */
>> +        path_combine(filename_tmp, sizeof(filename_tmp),
>> +                     curr_bs->filename, backing_file);
>> +
>> +        /* We are going to compare absolute pathnames */
>> +        if (!realpath(filename_tmp, filename_full)) {
>> +            continue;
>> +        }
>> +
>> +        /* We need to make sure the backing filename we are comparing against
>> +         * is relative to the current image filename (or absolute) */
>> +        path_combine(filename_tmp, sizeof(filename_tmp),
>> +                     curr_bs->filename, curr_bs->backing_file);
>> +
>> +        if (!realpath(filename_tmp, backing_file_full)) {
> 
> Unfortunately realpath does not exist on Win32.  But
> http://lists.gnu.org/archive/html/grub-devel/2011-09/msg00034.html
> suggests that there is a _fullpath and in fact already has all you need
> to write a qemu_realpath function.
> 
> Paolo
> 

Doesn't block.c already rely on realpath(), in bdrv_open()?  Why would
this be any different using it here?

>> +            continue;
>> +        }
>> +
>> +        if (strcmp(backing_file_full, filename_full) == 0) {
>> +            return curr_bs->backing_hd;
>>          }
>>      }
>>  
>>
>