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

[Max Reitz <mreitz@redhat.com>]

On 08.04.2014 08:49, Markus Armbruster wrote:
> Max Reitz <mreitz@redhat.com> 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 <mreitz@redhat.com>
>>>> ---
>>>>    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 <armbru@redhat.com>
> 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