Re: [Qemu-block] [PATCH 16/22] qmp: add persistent flag to block-dirty-bitmap-add

[Vladimir Sementsov-Ogievskiy]

10.10.2016 19:08, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:

[...]

> > +    }
> >   
> >    out:
> >       aio_context_release(aio_context);
> > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > index 31f9990..2bf56cd 100644
> > --- a/qapi/block-core.json
> > +++ b/qapi/block-core.json
> > @@ -1235,10 +1235,15 @@
> >   # @granularity: #optional the bitmap granularity, default is 64k for
> >   #               block-dirty-bitmap-add
> >   #
> > +# @persistent: #optional the bitmap is persistent, i.e. it will be saved to
> > +#              corresponding block device on it's close. Default is false.
> > +#              For block-dirty-bitmap-add. (Since 2.8)
> I'm not sure what the "For block-dirty-bitmap-add." is supposed to mean,
> because this whole struct is for block-dirty-bitmap-add (and for
> block-dirty-bitmap-add transactions, to be exact, but @persistent will
> surely work there, too, won't it?).
>
> Also, I'd say "will be saved to the corresponding block device image
> file" instead of just "block device", because in my understanding a
> block device and its image file are two separate things.

Hmm.. But 'its close' is block device close, not file close. And we call 
common bdrv_* function to save it, so we actually save it to device, and 
then the device puzzles out, how to actually save it.

> > +#
> >   # Since 2.4
> >   ##
> >   { 'struct': 'BlockDirtyBitmapAdd',
> > -  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32' } }
> > +  'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
> > +  '*persistent': 'bool' } }
> I think normally we'd align the new line so that the opening ' of
> '*persistent' is under the opening ' of 'node'.
>
> >   
> >   ##
> >   # @block-dirty-bitmap-add
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index ba2a916..434b418 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -1441,7 +1441,7 @@ EQMP
> >   
> >       {
> >           .name       = "block-dirty-bitmap-add",
> > -        .args_type  = "node:B,name:s,granularity:i?",
> > +        .args_type  = "node:B,name:s,granularity:i?,persistent:b?",
> >           .mhandler.cmd_new = qmp_marshal_block_dirty_bitmap_add,
> >       },
> >   
> > @@ -1458,6 +1458,9 @@ Arguments:
> >   - "node": device/node on which to create dirty bitmap (json-string)
> >   - "name": name of the new dirty bitmap (json-string)
> >   - "granularity": granularity to track writes with (int, optional)
> > +- "persistent": bitmap will be saved to corresponding block device
> > +                on it's close. Block driver should maintain persistent bitmaps
> > +                (json-bool, optional, default false) (Since 2.8)
> And I don't know what the user is supposed to make of the information
> that block drivers will take care of maintaining persistent bitmaps. All
> they care about is that it will be stored in the corresponding image
> file, so in my opinion it would be better to just omit the last sentence
> here.

Users shoud know, that only qcow2 supports persistent bitmaps. Instead 
of last sentence I can write "For now only Qcow2 disks supports 
persistent bitmaps".


--
Best regards,
Vladimir