[Qemu-devel] [PATCH v3 0/3] block: commits of snapshots larger than backing files

[Qemu-devel] [PATCH v3 0/3] block: commits of snapshots larger than backing files

[Jeff Cody]

Changes, v2->v3:

Patch 1/3: None
Patch 2/3: Set errp for both bdrv_getlength() failure cases (Fam)
           Remove "." from error message (Eric Blake)
Patch 3/3: Removed spurious double space (Eric Blake)


Changes, v1->v2:

Patch 1/3: Added error check for bdrv_getlength() return (Stefan)
Patch 2/3: Restore flags in commit_active_start() on failures after
           the bdrv_reopen() (Fam)
Patch 3/3: New patch, adds documentation clarification for the behavior
           of both offline commit, and live block commit, as it pertains
           to image truncation. (Stefan)

If a snapshot is larger than a backing file, then the offline bdrv_commit and
the live active layer commit will fail with an i/o error (usually).  A live
commit of a non-active layer will complete successfully, as it runs
bdrv_truncate() on the backing image to resize it to the larger size.

For both bdrv_commit() and commit_active_start(), this series will resize
the underlying base image if needed.  If the resize fails, an error will
be returned.


Jeff Cody (3):
  block: resize backing file image during offline commit, if necessary
  block: resize backing image during active layer commit, if needed
  block: update block commit documentation regarding image truncation

 block.c          | 23 ++++++++++++++++++++---
 block/mirror.c   | 41 +++++++++++++++++++++++++++++++++++++++++
 hmp-commands.hx  |  5 +++++
 qapi-schema.json |  7 +++++++
 qemu-img.texi    |  7 ++++++-
 qmp-commands.hx  | 39 +++++++++++++++++++++++++++++++++++++++
 6 files changed, 118 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/3] block: resize backing file image during offline commit, if necessary

[Jeff Cody]

Currently, if an image file is logically larger than its backing file,
commiting it via 'qemu-img commit' will fail.

For instance, if we have a base image with a virtual size 10G, and a
snapshot image of size 20G, then committing the snapshot offline with
'qemu-img commit' will likely fail.

This will automatically attempt to resize the base image, if the
snapshot image to be committed is larger.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 64e7d22..dea7591 100644
--- a/block.c
+++ b/block.c
@@ -1876,10 +1876,10 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
 int bdrv_commit(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
-    int64_t sector, total_sectors;
+    int64_t sector, total_sectors, length, backing_length;
     int n, ro, open_flags;
     int ret = 0;
-    uint8_t *buf;
+    uint8_t *buf = NULL;
     char filename[PATH_MAX];
 
     if (!drv)
@@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
         }
     }
 
-    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+    length = bdrv_getlength(bs);
+    backing_length = bdrv_getlength(bs->backing_hd);
+
+    if (length < 0 || backing_length < 0) {
+        goto ro_cleanup;
+    }
+
+    /* If our top snapshot is larger than the backing file image,
+     * grow the backing file image if possible.  If not possible,
+     * we must return an error */
+    if (length > backing_length) {
+        ret = bdrv_truncate(bs->backing_hd, length);
+        if (ret < 0) {
+            goto ro_cleanup;
+        }
+    }
+
+    total_sectors = length >> BDRV_SECTOR_BITS;
     buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
 
     for (sector = 0; sector < total_sectors; sector += n) {
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/3] block: resize backing image during active layer commit, if needed

[Jeff Cody]

If the top image to commit is the active layer, and also larger than
the base image, then an I/O error will likely be returned during
block-commit.

For instance, if we have a base image with a virtual size 10G, and a
active layer image of size 20G, then committing the snapshot via
'block-commit' will likely fail.

This will automatically attempt to resize the base image, if the
active layer image to be committed is larger.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block/mirror.c | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 2932bab..2a33aa6 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -630,11 +630,52 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
                          BlockDriverCompletionFunc *cb,
                          void *opaque, Error **errp)
 {
+    int64_t length, base_length;
+    int orig_base_flags;
+
+    orig_base_flags = bdrv_get_flags(base);
+
     if (bdrv_reopen(base, bs->open_flags, errp)) {
         return;
     }
+
+    length = bdrv_getlength(bs);
+    if (length < 0) {
+        error_setg(errp, "Unable to determine length of %s", bs->filename);
+        goto error_restore_flags;
+    }
+
+    base_length = bdrv_getlength(base);
+    if (base_length < 0) {
+        error_setg(errp, "Unable to determine length of %s", base->filename);
+        goto error_restore_flags;
+    }
+
+    if (length > base_length) {
+        if (bdrv_truncate(base, length) < 0) {
+            error_setg(errp, "Top image %s is larger than base image %s, and "
+                             "resize of base image failed",
+                             bs->filename, base->filename);
+            goto error_restore_flags;
+        }
+    } else if (length < 0) {
+        goto error_restore_flags;
+    }
+
+
     bdrv_ref(base);
     mirror_start_job(bs, base, speed, 0, 0,
                      on_error, on_error, cb, opaque, errp,
                      &commit_active_job_driver, false, base);
+    if (error_is_set(errp)) {
+        goto error_restore_flags;
+    }
+
+    return;
+
+error_restore_flags:
+    /* ignore error and errp for bdrv_reopen, because we want to propagate
+     * the original error */
+    bdrv_reopen(base, orig_base_flags, NULL);
+    return;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 3/3] block: update block commit documentation regarding image truncation

[Jeff Cody]

This updates the documentation for commiting snapshot images.
Specifically, this highlights what happens when the base image
is either smaller or larger than the snapshot image being committed.

In the case of the base image being smaller, it is resized to the
larger size of the snapshot image.  In the case of the base image
being larger, it is not resized automatically, but once the commit
has completed it is safe for the user to truncate the base image.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hmp-commands.hx  |  5 +++++
 qapi-schema.json |  7 +++++++
 qemu-img.texi    |  7 ++++++-
 qmp-commands.hx  | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index feca084..f3fc514 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -35,6 +35,11 @@ STEXI
 @item commit
 @findex commit
 Commit changes to the disk images (if -snapshot is used) or backing files.
+If the backing file is smaller than the snapshot, then the backing file will be
+resized to be the same size as the snapshot.  If the snapshot is smaller than
+the backing file, the backing file will not be truncated.  If you want the
+backing file to match the size of the smaller snapshot, you can safely truncate
+it yourself once the commit operation successfully completes.
 ETEXI
 
     {
diff --git a/qapi-schema.json b/qapi-schema.json
index f27c48a..64a108b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1973,6 +1973,13 @@
 #                    user needs to complete the job with the block-job-complete
 #                    command after getting the ready event. (Since 2.0)
 #
+#                    If the base image is smaller than top, then the base image
+#                    will be resized to be the same size as top.  If top is
+#                    smaller than the base image, the base will not be
+#                    truncated.  If you want the base image size to match the
+#                    size of the smaller top, you can safely truncate it
+#                    yourself once the commit operation successfully completes.
+#
 #
 # @speed:  #optional the maximum speed, in bytes per second
 #
diff --git a/qemu-img.texi b/qemu-img.texi
index 1bba91e..22556df 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -140,7 +140,12 @@ it doesn't need to be specified separately in this case.
 
 @item commit [-f @var{fmt}] [-t @var{cache}] @var{filename}
 
-Commit the changes recorded in @var{filename} in its base image.
+Commit the changes recorded in @var{filename} in its base image or backing file.
+If the backing file is smaller than the snapshot, then the backing file will be
+resized to be the same size as the snapshot.  If the snapshot is smaller than
+the backing file, the backing file will not be truncated.  If you want the
+backing file to match the size of the smaller snapshot, you can safely truncate
+it yourself once the commit operation successfully completes.
 
 @item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2}
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 02cc815..d7ed5ba 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -965,6 +965,45 @@ EQMP
         .mhandler.cmd_new = qmp_marshal_input_block_commit,
     },
 
+SQMP
+block-commit
+------------
+
+Live commit of data from overlay image nodes into backing nodes - i.e., writes
+data between 'top' and 'base' into 'base'.
+
+Arguments:
+
+- "device": The device's ID, must be unique (json-string)
+- "base": The file name of the backing image to write data into.
+          If not specified, this is the deepest backing image
+          (json-string, optional)
+- "top":  The file name of the backing image within the image chain,
+          which contains the topmost data to be committed down.
+
+          If top == base, that is an error.
+          If top == active, the job will not be completed by itself,
+          user needs to complete the job with the block-job-complete
+          command after getting the ready event. (Since 2.0)
+
+          If the base image is smaller than top, then the base image
+          will be resized to be the same size as top.  If top is
+          smaller than the base image, the base will not be
+          truncated.  If you want the base image size to match the
+          size of the smaller top, you can safely truncate it
+          yourself once the commit operation successfully completes.
+          (json-string)
+- "speed":  the maximum speed, in bytes per second (json-int, optional)
+
+
+Example:
+
+-> { "execute": "block-commit", "arguments": { "device": "virtio0",
+                                              "top": "/tmp/snap1.qcow2" } }
+<- { "return": {} }
+
+EQMP
+
     {
         .name       = "drive-backup",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/3] block: resize backing file image during offline commit, if necessary

[Benoît Canet]

Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
> Currently, if an image file is logically larger than its backing file,
> commiting it via 'qemu-img commit' will fail.
> 
> For instance, if we have a base image with a virtual size 10G, and a
> snapshot image of size 20G, then committing the snapshot offline with
> 'qemu-img commit' will likely fail.
> 
> This will automatically attempt to resize the base image, if the
> snapshot image to be committed is larger.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Reviewed-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 64e7d22..dea7591 100644
> --- a/block.c
> +++ b/block.c
> @@ -1876,10 +1876,10 @@ int bdrv_check(BlockDriverState *bs, BdrvCheckResult *res, BdrvCheckMode fix)
>  int bdrv_commit(BlockDriverState *bs)
>  {
>      BlockDriver *drv = bs->drv;
> -    int64_t sector, total_sectors;
> +    int64_t sector, total_sectors, length, backing_length;
>      int n, ro, open_flags;
>      int ret = 0;
> -    uint8_t *buf;
> +    uint8_t *buf = NULL;

Why assign NULL to buf ? Is it related to the rest of the patch ?

Reviewed-by: Benoit Canet <benoit@irqsave.net>

>      char filename[PATH_MAX];
>  
>      if (!drv)
> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
>          }
>      }
>  
> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +    length = bdrv_getlength(bs);
> +    backing_length = bdrv_getlength(bs->backing_hd);
> +
> +    if (length < 0 || backing_length < 0) {
> +        goto ro_cleanup;
> +    }
> +
> +    /* If our top snapshot is larger than the backing file image,
> +     * grow the backing file image if possible.  If not possible,
> +     * we must return an error */
> +    if (length > backing_length) {
> +        ret = bdrv_truncate(bs->backing_hd, length);
> +        if (ret < 0) {
> +            goto ro_cleanup;
> +        }
> +    }
> +
> +    total_sectors = length >> BDRV_SECTOR_BITS;
>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
>  
>      for (sector = 0; sector < total_sectors; sector += n) {
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH v3 2/3] block: resize backing image during active layer commit, if needed

[Benoît Canet]

Le Thursday 23 Jan 2014 à 16:48:56 (-0500), Jeff Cody a écrit :
> If the top image to commit is the active layer, and also larger than
> the base image, then an I/O error will likely be returned during
> block-commit.
> 
> For instance, if we have a base image with a virtual size 10G, and a
> active layer image of size 20G, then committing the snapshot via
> 'block-commit' will likely fail.
> 
> This will automatically attempt to resize the base image, if the
> active layer image to be committed is larger.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/mirror.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 2932bab..2a33aa6 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -630,11 +630,52 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
>                           BlockDriverCompletionFunc *cb,
>                           void *opaque, Error **errp)
>  {
> +    int64_t length, base_length;
> +    int orig_base_flags;
> +
> +    orig_base_flags = bdrv_get_flags(base);
> +
>      if (bdrv_reopen(base, bs->open_flags, errp)) {
>          return;
>      }
> +
> +    length = bdrv_getlength(bs);
> +    if (length < 0) {
> +        error_setg(errp, "Unable to determine length of %s", bs->filename);
> +        goto error_restore_flags;
> +    }
> +
> +    base_length = bdrv_getlength(base);
> +    if (base_length < 0) {
> +        error_setg(errp, "Unable to determine length of %s", base->filename);
> +        goto error_restore_flags;
> +    }
> +
> +    if (length > base_length) {
> +        if (bdrv_truncate(base, length) < 0) {
> +            error_setg(errp, "Top image %s is larger than base image %s, and "
> +                             "resize of base image failed",
> +                             bs->filename, base->filename);
> +            goto error_restore_flags;
> +        }


> +    } else if (length < 0) {
> +        goto error_restore_flags;
> +    }
The code already test for (length < 0) ten lines above. Is this branch really
needed ?

Best regards

Benoît

> +
> +
>      bdrv_ref(base);
>      mirror_start_job(bs, base, speed, 0, 0,
>                       on_error, on_error, cb, opaque, errp,
>                       &commit_active_job_driver, false, base);
> +    if (error_is_set(errp)) {
> +        goto error_restore_flags;
> +    }
> +
> +    return;
> +
> +error_restore_flags:
> +    /* ignore error and errp for bdrv_reopen, because we want to propagate
> +     * the original error */
> +    bdrv_reopen(base, orig_base_flags, NULL);
> +    return;
>  }
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH v3 2/3] block: resize backing image during active layer commit, if needed

[Eric Blake]

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

On 01/23/2014 02:48 PM, Jeff Cody wrote:
> If the top image to commit is the active layer, and also larger than
> the base image, then an I/O error will likely be returned during
> block-commit.
> 
> For instance, if we have a base image with a virtual size 10G, and a
> active layer image of size 20G, then committing the snapshot via
> 'block-commit' will likely fail.
> 
> This will automatically attempt to resize the base image, if the
> active layer image to be committed is larger.
> 
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

> +    if (length > base_length) {
> +        if (bdrv_truncate(base, length) < 0) {
> +            error_setg(errp, "Top image %s is larger than base image %s, and "
> +                             "resize of base image failed",
> +                             bs->filename, base->filename);
> +            goto error_restore_flags;
> +        }
> +    } else if (length < 0) {
> +        goto error_restore_flags;
> +    }
> +
> +
>      bdrv_ref(base);
>      mirror_start_job(bs, base, speed, 0, 0,
>                       on_error, on_error, cb, opaque, errp,
>                       &commit_active_job_driver, false, base);
> +    if (error_is_set(errp)) {
> +        goto error_restore_flags;

If we get here, bdrv_truncate() succeeded at enlarging the base file.  A
full rollback would imply adding a second bdrv_truncate() in the error
handling path to undo the growth back to the original size (of course,
ignoring failure to shrink).  But that's more complex, and probably not
worth adding in (at any rate, leaving the base file larger doesn't
affect semantics of the bytes seen by the guest, and for file formats
that handle resizing via a sparse tail doesn't even consume much
storage).  So my R-b stands.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/3] block: resize backing file image during offline commit, if necessary

[Eric Blake]

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

On 01/23/2014 03:00 PM, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
>> Currently, if an image file is logically larger than its backing file,
>> commiting it via 'qemu-img commit' will fail.

s/commiting/committing/


>> +    uint8_t *buf = NULL;
> 
> Why assign NULL to buf ? Is it related to the rest of the patch ?
> 
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> 
>>      char filename[PATH_MAX];
>>  
>>      if (!drv)
>> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
>>          }
>>      }
>>  
>> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
>> +    length = bdrv_getlength(bs);
>> +    backing_length = bdrv_getlength(bs->backing_hd);
>> +
>> +    if (length < 0 || backing_length < 0) {
>> +        goto ro_cleanup;

Because this goto now reaches the ro_cleanup label with buf
uninitialized, if we don't assign NULL originally.

>> +    total_sectors = length >> BDRV_SECTOR_BITS;
>>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);

The old code only ever reached ro_cleanup after assigning buf, and
ro_cleanup blindly frees buf.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 1/3] block: resize backing file image during offline commit, if necessary

[Jeff Cody]

On Thu, Jan 23, 2014 at 03:07:44PM -0700, Eric Blake wrote:
> On 01/23/2014 03:00 PM, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
> >> Currently, if an image file is logically larger than its backing file,
> >> commiting it via 'qemu-img commit' will fail.
> 
> s/commiting/committing/
> 

I could respin, or Kevin / Stefan could fix this up whenever it is
applied to their branches.

> 
> >> +    uint8_t *buf = NULL;
> > 
> > Why assign NULL to buf ? Is it related to the rest of the patch ?
> > 
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > 
> >>      char filename[PATH_MAX];
> >>  
> >>      if (!drv)
> >> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
> >>          }
> >>      }
> >>  
> >> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> >> +    length = bdrv_getlength(bs);
> >> +    backing_length = bdrv_getlength(bs->backing_hd);
> >> +
> >> +    if (length < 0 || backing_length < 0) {
> >> +        goto ro_cleanup;
> 
> Because this goto now reaches the ro_cleanup label with buf
> uninitialized, if we don't assign NULL originally.
> 

Yup, exactly.

> >> +    total_sectors = length >> BDRV_SECTOR_BITS;
> >>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> 
> The old code only ever reached ro_cleanup after assigning buf, and
> ro_cleanup blindly frees buf.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v3 2/3] block: resize backing image during active layer commit, if needed

[Jeff Cody]

On Thu, Jan 23, 2014 at 11:05:26PM +0100, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 16:48:56 (-0500), Jeff Cody a écrit :
> > If the top image to commit is the active layer, and also larger than
> > the base image, then an I/O error will likely be returned during
> > block-commit.
> > 
> > For instance, if we have a base image with a virtual size 10G, and a
> > active layer image of size 20G, then committing the snapshot via
> > 'block-commit' will likely fail.
> > 
> > This will automatically attempt to resize the base image, if the
> > active layer image to be committed is larger.
> > 
> > Signed-off-by: Jeff Cody <jcody@redhat.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block/mirror.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index 2932bab..2a33aa6 100644
> > --- a/block/mirror.c
> > +++ b/block/mirror.c
> > @@ -630,11 +630,52 @@ void commit_active_start(BlockDriverState *bs, BlockDriverState *base,
> >                           BlockDriverCompletionFunc *cb,
> >                           void *opaque, Error **errp)
> >  {
> > +    int64_t length, base_length;
> > +    int orig_base_flags;
> > +
> > +    orig_base_flags = bdrv_get_flags(base);
> > +
> >      if (bdrv_reopen(base, bs->open_flags, errp)) {
> >          return;
> >      }
> > +
> > +    length = bdrv_getlength(bs);
> > +    if (length < 0) {
> > +        error_setg(errp, "Unable to determine length of %s", bs->filename);
> > +        goto error_restore_flags;
> > +    }
> > +
> > +    base_length = bdrv_getlength(base);
> > +    if (base_length < 0) {
> > +        error_setg(errp, "Unable to determine length of %s", base->filename);
> > +        goto error_restore_flags;
> > +    }
> > +
> > +    if (length > base_length) {
> > +        if (bdrv_truncate(base, length) < 0) {
> > +            error_setg(errp, "Top image %s is larger than base image %s, and "
> > +                             "resize of base image failed",
> > +                             bs->filename, base->filename);
> > +            goto error_restore_flags;
> > +        }
> 
> 
> > +    } else if (length < 0) {
> > +        goto error_restore_flags;
> > +    }
> The code already test for (length < 0) ten lines above. Is this branch really
> needed ?
> 
> Best regards
> 
> Benoît

Thanks - no, it is not needed.  I did not mean to have that there -
while harmless, it is essentially dead code and should be culled.
I'll submit a v4 to take care of that.

> 
> > +
> > +
> >      bdrv_ref(base);
> >      mirror_start_job(bs, base, speed, 0, 0,
> >                       on_error, on_error, cb, opaque, errp,
> >                       &commit_active_job_driver, false, base);
> > +    if (error_is_set(errp)) {
> > +        goto error_restore_flags;
> > +    }
> > +
> > +    return;
> > +
> > +error_restore_flags:
> > +    /* ignore error and errp for bdrv_reopen, because we want to propagate
> > +     * the original error */
> > +    bdrv_reopen(base, orig_base_flags, NULL);
> > +    return;
> >  }
> > -- 
> > 1.8.3.1
> > 
> > 

Re: [Qemu-devel] [PATCH v3 1/3] block: resize backing file image during offline commit, if necessary

[Benoît Canet]

Le Thursday 23 Jan 2014 à 15:07:44 (-0700), Eric Blake a écrit :
> On 01/23/2014 03:00 PM, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 16:48:55 (-0500), Jeff Cody a écrit :
> >> Currently, if an image file is logically larger than its backing file,
> >> commiting it via 'qemu-img commit' will fail.
> 
> s/commiting/committing/
> 
> 
> >> +    uint8_t *buf = NULL;
> > 
> > Why assign NULL to buf ? Is it related to the rest of the patch ?
> > 
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > 
> >>      char filename[PATH_MAX];
> >>  
> >>      if (!drv)
> >> @@ -1904,7 +1904,24 @@ int bdrv_commit(BlockDriverState *bs)
> >>          }
> >>      }
> >>  
> >> -    total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> >> +    length = bdrv_getlength(bs);
> >> +    backing_length = bdrv_getlength(bs->backing_hd);
> >> +
> >> +    if (length < 0 || backing_length < 0) {
> >> +        goto ro_cleanup;
> 
> Because this goto now reaches the ro_cleanup label with buf
> uninitialized, if we don't assign NULL originally.

Thanks for the explanation.

Best regards

Benoît
> 
> >> +    total_sectors = length >> BDRV_SECTOR_BITS;
> >>      buf = g_malloc(COMMIT_BUF_SECTORS * BDRV_SECTOR_SIZE);
> 
> The old code only ever reached ro_cleanup after assigning buf, and
> ro_cleanup blindly frees buf.
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>