| Message ID | 1398784072-3319-9-git-send-email-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/29/2014 09:07 AM, Max Reitz wrote: > After the top image has been committed, it should be emptied unless > specified otherwise. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > qemu-img-cmds.hx | 4 ++-- > qemu-img.c | 34 +++++++++++++++++++++++++++++++--- > qemu-img.texi | 6 +++++- > 3 files changed, 38 insertions(+), 6 deletions(-) > > > + /* The block job will swap base_bs and bs (which is not what we really want > + * here, but okay) and unref bs (and subsequently all intermediate block > + * devices). In order to be able to empty these images afterwards, increment > + * the reference counter here preemptively. */ Comment is stale, since we aren't going to "empty these images", only the top image. > > +The image @var{filename} is emptied after the operation has been successful. If s/has been successful/has succeeded/ > +you do not need @var{filename} afterwards anymore and intend to drop it, you may s/ anymore// > +skip this operation by specifying the @code{-d} flag. s/this operation/emptying @var{filename}/ The code looks okay, but as my tweaks include user-visible documentation, you may want to post another revision just to make sure it reads well. But we're close enough that if you only need to make the changes I suggested, I'm fine with: Reviewed-by: Eric Blake <eblake@redhat.com>
On 29.04.2014 17:41, Eric Blake wrote: > On 04/29/2014 09:07 AM, Max Reitz wrote: >> After the top image has been committed, it should be emptied unless >> specified otherwise. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> qemu-img-cmds.hx | 4 ++-- >> qemu-img.c | 34 +++++++++++++++++++++++++++++++--- >> qemu-img.texi | 6 +++++- >> 3 files changed, 38 insertions(+), 6 deletions(-) >> >> >> + /* The block job will swap base_bs and bs (which is not what we really want >> + * here, but okay) and unref bs (and subsequently all intermediate block >> + * devices). In order to be able to empty these images afterwards, increment >> + * the reference counter here preemptively. */ > Comment is stale, since we aren't going to "empty these images", only > the top image. Ah, right. I worked so long on this version that I simply forgot to adjust this in the end. >> >> +The image @var{filename} is emptied after the operation has been successful. If > s/has been successful/has succeeded/ > >> +you do not need @var{filename} afterwards anymore and intend to drop it, you may > s/ anymore// > >> +skip this operation by specifying the @code{-d} flag. > s/this operation/emptying @var{filename}/ > > The code looks okay, but as my tweaks include user-visible > documentation, you may want to post another revision just to make sure > it reads well. But we're close enough that if you only need to make the > changes I suggested, I'm fine with: > > Reviewed-by: Eric Blake <eblake@redhat.com> Thank you, I will most certainly adjust this as I do expect some comments about patch 3. ;-) Max
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx index d029609..b31d81c 100644 --- a/qemu-img-cmds.hx +++ b/qemu-img-cmds.hx @@ -22,9 +22,9 @@ STEXI ETEXI DEF("commit", img_commit, - "commit [-q] [-f fmt] [-t cache] filename") + "commit [-q] [-f fmt] [-t cache] [-d] filename") STEXI -@item commit [-q] [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-q] [-f @var{fmt}] [-t @var{cache}] [-d] @var{filename} ETEXI DEF("compare", img_compare, diff --git a/qemu-img.c b/qemu-img.c index f80b42c..2ffc054 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -737,14 +737,14 @@ static int img_commit(int argc, char **argv) int c, ret, flags; const char *filename, *fmt, *cache; BlockDriverState *bs, *base_bs; - bool quiet = false; + bool quiet = false, drop = false; Error *local_err = NULL; CommonBlockJobCBInfo cbi; fmt = NULL; cache = BDRV_DEFAULT_CACHE; for(;;) { - c = getopt(argc, argv, "f:ht:q"); + c = getopt(argc, argv, "f:ht:dq"); if (c == -1) { break; } @@ -759,6 +759,9 @@ static int img_commit(int argc, char **argv) case 't': cache = optarg; break; + case 'd': + drop = true; + break; case 'q': quiet = true; break; @@ -769,7 +772,7 @@ static int img_commit(int argc, char **argv) } filename = argv[optind++]; - flags = BDRV_O_RDWR; + flags = BDRV_O_RDWR | BDRV_O_UNMAP; ret = bdrv_parse_cache_flags(cache, &flags); if (ret < 0) { error_report("Invalid cache option: %s", cache); @@ -801,7 +804,32 @@ static int img_commit(int argc, char **argv) goto done; } + /* The block job will swap base_bs and bs (which is not what we really want + * here, but okay) and unref bs (and subsequently all intermediate block + * devices). In order to be able to empty these images afterwards, increment + * the reference counter here preemptively. */ + if (!drop) { + bdrv_ref(base_bs); + } + run_block_job(bs->job, &local_err); + if (local_err) { + goto unref_backing; + } + + if (!drop && base_bs->drv->bdrv_make_empty) { + ret = base_bs->drv->bdrv_make_empty(base_bs); + if (ret) { + error_setg_errno(&local_err, -ret, "Could not empty %s", + filename); + goto unref_backing; + } + } + +unref_backing: + if (!drop) { + bdrv_unref(base_bs); + } done: bdrv_unref(bs); diff --git a/qemu-img.texi b/qemu-img.texi index f84590e..40f0088 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -140,7 +140,7 @@ this case. @var{backing_file} will never be modified unless you use the The size can also be specified using the @var{size} option with @code{-o}, it doesn't need to be specified separately in this case. -@item commit [-f @var{fmt}] [-t @var{cache}] @var{filename} +@item commit [-f @var{fmt}] [-t @var{cache}] [-d] @var{filename} 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 @@ -149,6 +149,10 @@ 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. +The image @var{filename} is emptied after the operation has been successful. If +you do not need @var{filename} afterwards anymore and intend to drop it, you may +skip this operation by specifying the @code{-d} flag. + @item compare [-f @var{fmt}] [-F @var{fmt}] [-p] [-s] [-q] @var{filename1} @var{filename2} Check if two images have the same content. You can compare images with
After the top image has been committed, it should be emptied unless specified otherwise. Signed-off-by: Max Reitz <mreitz@redhat.com> --- qemu-img-cmds.hx | 4 ++-- qemu-img.c | 34 +++++++++++++++++++++++++++++++--- qemu-img.texi | 6 +++++- 3 files changed, 38 insertions(+), 6 deletions(-)