Re: [Qemu-devel] [RFC PATCH v2 10/12] mc: expose tunable parameter for checkpointing frequency

[Michael R. Hines]

On 03/12/2014 05:49 AM, Eric Blake wrote:
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index f3fc514..2066c76 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -888,7 +888,7 @@ ETEXI
>                       "\n\t\t\t -b for migration without shared storage with"
>                       " full copy of disk\n\t\t\t -i for migration without "
>                       "shared storage with incremental copy of disk "
> -                     "(base image shared between src and destination)",
> +                     "(base image shared between src and destination)",
> Spurious hunk.  Oh, I see - you managed to take TAB damage and make it
> worse with a space-TAB (I guess this file isn't tab-clean, like the
> .json file is).  Eww.

Ooops. =)

> >           .mhandler.cmd = hmp_migrate,
> >       },
> >   
> > @@ -965,6 +965,20 @@ Set maximum tolerated downtime (in seconds) for migration.
> >   ETEXI
> >   
> >       {
> > +        .name       = "migrate-set-mc-delay",
> We're building up a LOT of migrate- tunable commands.  Maybe it's time
> to think about building a more generic migrate-set-parameter, which
> takes both the name of the parameter to set and its value, so that a
> single command serves all parameters, instead of needing a proliferation
> of commands.  Of course, for that to be useful, we also need a way to
> introspect which parameters can be tuned; whereas with the current
> approach of one command per parameter (well, 2 for set vs. get) the
> introspection is based on whether the command exists.

Well, unless there's anymore strong objection, I didn't find it too
difficult to add a new command in QEMU, although I did find it quite
painful to expose this command in Libvirt - I had to modify something
like 5 or 6 (IIRC) different files in libvirt to accomplish the same goal.

Could we "merge" the commands into a single command at the
libvirt level instead of the QEMU level?

Is there any other "pressing" reason to merge them at the QEMU
level?


> > +++ b/qapi-schema.json
> > @@ -2160,6 +2160,19 @@
> >   { 'command': 'migrate_set_downtime', 'data': {'value': 'number'} }
> >   
> >   ##
> > +# @migrate-set-mc-delay
> > +#
> > +# Set delay (in milliseconds) between micro checkpoints.
> > +#
> > +# @value: maximum delay in milliseconds
> > +#
> > +# Returns: nothing on success
> > +#
> > +# Since: 2.x
> > +##
> > +{ 'command': 'migrate-set-mc-delay', 'data': {'value': 'int'} }
> > +
> > +##
> I hate write-only interfaces.  If I can set the parameter, I _also_ need
> a way to query the current value of the parameter.  Either an existing
> migration statistics output should be modified to include this new
> information, or you need to add a counterpart migrate-get-mc-delay command.

Totally forgot about that - will get a 'get' command in there ASAP.

- Michael