qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 3/4] qemu-img: Implement commit like QMP


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 3/4] qemu-img: Implement commit like QMP
Date: Tue, 08 Apr 2014 13:16:39 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.4.0

On 08.04.2014 08:49, Markus Armbruster wrote:
Max Reitz <address@hidden> writes:

On 07.04.2014 21:10, Eric Blake wrote:
On 04/07/2014 11:29 AM, Max Reitz wrote:
qemu-img should use QMP commands whenever possible in order to ensure
feature completeness of both online and offline image operations. As
qemu-img itself has no access to QMP (since this would basically require
just everything being linked into qemu-img), imitate QMP's
implementation of block-commit by using commit_active_start() and then
waiting for the block job to finish.

This new implementation does not empty the snapshot image, as opposed to
the old implementation using bdrv_commit(). However, as QMP's
block-commit apparently never did this and as qcow2 (which is probably
qemu's standard image format) does not even implement the required
function (bdrv_make_empty()), it does not seem necessary.

Signed-off-by: Max Reitz <address@hidden>
---
   block/Makefile.objs |  2 +-
   qemu-img.c          | 68 
++++++++++++++++++++++++++++++++++++++---------------
   2 files changed, 50 insertions(+), 20 deletions(-)

@@ -728,29 +755,32 @@ static int img_commit(int argc, char **argv)
       if (!bs) {
           return 1;
       }
+
+    base_bs = bdrv_find_base(bs);
+    if (!base_bs) {
+        error_set(&local_err, QERR_BASE_NOT_FOUND, "NULL");
+        goto done;
+    }
Is it worth adding an optional '-b base' image to allow qemu-img to
commit across multiple images?  That is, QMP can shorten from 'a <- b <-
c' all the way to 'a'; but qemu-img has to be called twice (once to 'a
<- b' and second to 'a').  Separate commit, of course.
Sounds interesting, I'll have a look.

+
+ commit_active_start(bs, base_bs, 0, COMMIT_BUF_SECTORS <<
BDRV_SECTOR_BITS,
+                        BLOCKDEV_ON_ERROR_REPORT, dummy_block_job_cb, bs,
+                        &local_err);
+    if (error_is_set(&local_err)) {
No new uses of error_is_set if we can help it.  This can be 'if
(local_err)'.
Okay, seems like I missed something.
Yes :)

commit 84d18f065fb041a1c0d78d20320d740ae0673c8a
Author: Markus Armbruster <address@hidden>
Date:   Thu Jan 30 15:07:28 2014 +0100

     Use error_is_set() only when necessary
error_is_set(&var) is the same as var != NULL, but it takes
     whole-program analysis to figure that out.  Unnecessarily hard for
     optimizers, static checkers, and human readers.  Dumb it down to
     obvious.
Gets rid of several dozen Coverity false positives. Note that the obvious form is already used in many places.

[...]

Thank you for the reference. Seems like I did not miss it, but worse, I forgot about it.

Max



reply via email to

[Prev in Thread] Current Thread [Next in Thread]