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

[Paolo Bonzini <pbonzini@redhat.com>]

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

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