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

[Markus Armbruster]

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.

[...]