Re: [Qemu-devel] [PATCH] block: Fix relative backing file path checking

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2522 bytes --]

On 12/09/2013 11:42 PM, Xu Wang wrote:
> This patch is made for Bug #1257334 (diffuse handling of image
> creation from another path). The cause of it is user could create
> image even though the backing file doesn't exist. Becase backing

s/Becase/, because/

> file checking in the bdrv_img_create() is from the user's current
> path instead of relative path to the image to be created. This patch
> makes qemu check backing file according to the relative path to the
> image to be created. Hence if relative backing file path doesn't
> exist, the backing file checking will fail now.
> 
> Test case:
>   Reproduce process (from bug page):
>     1. $mkdir a/
>     2. $qemu-img create -f qcow2 a/blob.img 10G
>     3. $qemu-img create -f qcow2 -b a/blob.img a/ovl.img
>        Here the actual backing file of ovl.img is a/a/blob.img. But
>        the backing file checking will check from the user's current
>        path and find a/blob.img successfully. But the path saved in
>        the ovl.img is a/a/blob.img. Bug occurred.
> 
>   After patched:
>     The step 3 above an error message will be thrown because backing
>     file checking started after got the full path of backing file
>     intead of relative path.

s/intead/instead/

> 
> Signed-off-by: Xu Wang <gesaint@linux.vnet.ibm.com>
> ---
>  block.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)


> 
> diff --git a/block.c b/block.c
> index 13f001a..20d2b66 100644
> --- a/block.c
> +++ b/block.c
> @@ -4790,18 +4790,31 @@ void bdrv_img_create(const char *filename, const char *fmt,
>              uint64_t size;
>              char buf[32];
>              int back_flags;
> +            char backing_filename_full[PATH_MAX];

I hate code that uses PATH_MAX as an array size.  It is not portable to
systems like GNU Hurd.  But qemu doesn't compile on GNU Hurd, and you're
not the first user of this construct in the codebase (so the problem
already exists and should be independently cleaned up).  So I won't let
it hold up your patch.

> +                error_setg_errno(errp, -ret, "Backing file '%s'"
> +                                 "(actual path is '%s') error: %s",

Outputs "Backing file 'foo'(actual path...", which looks bad.  You need
a space in the error message either before or after the line break.

The rest of the patch looks okay.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]