Re: [Qemu-devel] [PATCH v2 1/2] vmdk: Fix error for JSON descriptor file names

[Fam Zheng <famz@redhat.com>]

On Wed, 12/03 14:57, Max Reitz wrote:
> If vmdk blindly tries to use path_combine() using bs->file->filename as
> the base file name, this will result in a bad error message for JSON
> file names when calling bdrv_open(). It is better to only try
> bs->file->exact_filename; if that is empty, bs->file->filename will be
> useless for path_combine() and an error should be emitted (containing
> bs->file->filename because desc_file_path (which is
> bs->file->exact_filename) is empty).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c               |  2 +-
>  block/vmdk.c          | 10 +++++++++-
>  include/block/block.h |  1 +
>  3 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 591fbe4..239ae5f 100644
> --- a/block.c
> +++ b/block.c
> @@ -229,7 +229,7 @@ size_t bdrv_opt_mem_align(BlockDriverState *bs)
>  }
>  
>  /* check if the path starts with "<protocol>:" */
> -static int path_has_protocol(const char *path)
> +int path_has_protocol(const char *path)
>  {
>      const char *p;
>  
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2cbfd3e..65413a1 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -818,6 +818,14 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
>              goto next_line;
>          }
>  
> +        if (!path_is_absolute(fname) && !path_has_protocol(fname) &&

If I read the VMDK spec correctly, fname can't have protocol (as a format spec
it hasn't a single word on protocol :). But it's not wrong to check. I continue
reading thinking it as "true".

fname, on the other hand, is in most cases a relative path. In this case, this
condition falls to desc_file_path, which is bs->file->exact_filename. This will
be non-empty for local files, so the path_combine below will work.

If fname is absolute, path_combine will also work.

So it is correct.

> +            !desc_file_path[0])
> +        {
> +            error_setg(errp, "Cannot use relative extent paths with VMDK "
> +                       "descriptor file '%s'", bs->file->filename);
> +            return -EINVAL;
> +        }
> +
>          path_combine(extent_path, sizeof(extent_path),
>                  desc_file_path, fname);
>          extent_file = NULL;
> @@ -894,7 +902,7 @@ static int vmdk_open_desc_file(BlockDriverState *bs, int flags, char *buf,
>      }
>      s->create_type = g_strdup(ct);
>      s->desc_offset = 0;
> -    ret = vmdk_parse_extents(buf, bs, bs->file->filename, errp);
> +    ret = vmdk_parse_extents(buf, bs, bs->file->exact_filename, errp);
>  exit:
>      return ret;
>  }
> diff --git a/include/block/block.h b/include/block/block.h
> index 610be9f..919c8f5 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -401,6 +401,7 @@ void bdrv_get_full_backing_filename(BlockDriverState *bs,
>                                      char *dest, size_t sz);
>  int bdrv_is_snapshot(BlockDriverState *bs);
>  
> +int path_has_protocol(const char *path);

Exporting path_has_protocol could be a separate patch but I don't have a strong
objection as this patch is still short.

Reviewed-by: Fam Zheng <famz@redhat.com>

>  int path_is_absolute(const char *path);
>  void path_combine(char *dest, int dest_size,
>                    const char *base_path,
> -- 
> 1.9.3
>