Re: [Qemu-devel] [PATCH 6/6] qapi: Convert migrate

[Luiz Capitulino <lcapitulino@redhat.com>]

On Wed, 15 Feb 2012 10:01:22 -0600
Michael Roth <mdroth@linux.vnet.ibm.com> wrote:

> On Wed, Feb 15, 2012 at 02:25:49PM +0100, Juan Quintela wrote:
> > Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > > The migrate command is one of those commands where HMP and QMP completely
> > > mix up together. This made the conversion to the QAPI (which separates the
> > > command into QMP and HMP parts) a bit difficult.
> > >
> > > The first important change to be noticed is that this commit completes the
> > > removal of the Monitor object from migration code, started by the previous
> > > commit.
> > >
> > > Another important and tricky change is about supporting the non-detached
> > > mode. That's, if the user doesn't pass '-d' the migrate command will lock
> > > the monitor and will only release it when migration is finished.
> > >
> > > To support that in the new HMP command (hmp_migrate()), it was necessary
> > > to create a timer which runs every second and checks if the migration is
> > > still active. If it's, the timer callback will re-schedule itself to run
> > > one second in the future. If the migration has already finished, the
> > > monitor lock is relased and the user can use it normally.
> > >
> > > All these changes should be transparent to the user.
> > 
> > > +static void hmp_migrate_status_cb(void *opaque)
> > > +{
> > > +    MigrationStatus *status = opaque;
> > > +    MigrationInfo *info;
> > > +
> > > +    info = qmp_query_migrate(NULL);
> > > +    if (!info->has_status || strcmp(info->status, "active") == 0) {
> > > +        qemu_mod_timer(status->timer, qemu_get_clock_ms(rt_clock) +
> > > 1000);
> > 
> > 1 second is one eternity.  I will ask for something smaller (100ms or
> > 200ms range).

Well, I see two issues in reducing the timeout. The first one is that, we'll
be polling more aggressively for something that doesn't finish quickly. The
one is that, this is HMP, so I'm not sure humans will perceive the difference
most of the time.

> > > +    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
> > 
> > This is the only user of qmp_migrate, why do we need to pass both
> > has_blk & blk (and inc and !!inc?)  Both parameters are bool, I am
> > really confused here.
> 
> QAPI-generated qmp_marshal_input_migrate() is another user, and we don't
> want to decide the defaults for incoming/blk there, so we need some way
> to tell qmp_migrate() whether or not the bools we pass should be
> ignored, or whether they were explicitly defined by the qmp user.
> 
> It looks weird in this particular case,

For all qapi functions that take optionals parameters, yes.

> but other than having multiple
> variants of the qmp_migrate() command it's the only way to avoid having
> randomly/un- initialized values clobber the defaults expected by the user.

This question always comes from time to time, I'm wondering if we could
generate a wrapper that assumes optionals are always passed or have good
default values (like checking if a pointer argument is NULL, and if it's
setting the has_ bool to false).

I don't think this is a must have, but is a nice feature.