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

[Luiz Capitulino]

On Wed, 15 Feb 2012 14:34:52 +0100
Jan Kiszka <address@hidden> wrote:

> On 2012-02-15 13:49, Luiz Capitulino wrote:
> > On Wed, 15 Feb 2012 09:59:07 +0100
> > Jan Kiszka <address@hidden> wrote:
> > 
> >> On 2012-02-10 20:31, Luiz Capitulino wrote:
> >>> This is a rebase of Anthony's conversion, from his glib branch; and this 
> >>> is
> >>> also the beginning of the conversion of complex commands to the qapi.
> >>>
> >>> There are two important changes that should be observed:
> >>>
> >>>  1. patch 5/6 purges the 'mon' object from migration code. One of the
> >>>     consequences is that we lose the ability to print progress status to
> >>>     the HMP user (esp. in block migration)
> >>
> >> This smells extremely fishy. You have some common "monitor" context in
> >> both cases, means something that decides where suspend/resume takes
> >> effect or where to pick up file descriptors from. If the exiting Monitor
> >> object is not generic enough, introduce some super-class and use that in
> >> common services. Or make sure that a variant of Monitor is also valid
> >> over QMP. But don't remove the dependency from the API, while
> >> reintroducing it via the backdoor of cur_mon.
> > 
> > What we really want to do here is to untangle HMP and QMP. Unfortunately,
> > the migrate command is one of those commands where the two are deeply
> > tangled and the split won't be perfect.
> > 
> > However, the two cases you mention above are solvable:
> > 
> >  1. suspend/resume: this is *really* a HMP feature and shouldn't be in any
> >     QMP code path. This is correctly addressed in this series by moving it
> >     to hmp_migrate()
> 
> Almost correctly. ;)

Well, it was moved to the right place :)

> > 
> >  2. file descriptor passing: the new QMP server will support sessions and
> >     we'll move statefull commands (like getfd) to it. When we do it, we'll
> >     introduce a new API to get fds that won't depend on the monitor. 
> > However,
> >     this requires all commands to be converted to the qapi first. Meanwhile
> >     we use the qemu_get_fd() API.
> > 
> >     Note: qemu_get_fd() is temporary, it shouldn't be a problem to use it
> >     (if it's not incorrect, of course, I honestly haven't fully tested it 
> > yet).
> 
> So there will be a common super-class of Monitor and that new QMP
> session that also manages the file descriptors? That would make sense.

Oh, yes. Now I see that you said exactly that earlier. Sorry for more or less
re-stating it.

> Still, there would be monitor_get_fd and qmp_get_fd then not
> qemu_get_fd. I think that should be done already.

The problem is that monitor_get_fd() already exists and qmp_get_fd()
doesn't make much sense (as this is not related to QMP right now). So,
I could call it monitor_get_fd_cur() or something like this.

> BTW, where do you get
> the FDs from now in QMP mode? Is there currently a Monitor instance
> associated?

Yes. The current QMP server is associated with a Monitor instance and
it supports the getfd command.