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

[Jan Kiszka]

On 2012-02-15 18:49, Luiz Capitulino wrote:
> On Wed, 15 Feb 2012 18:39:31 +0100
> Jan Kiszka <address@hidden> wrote:
> 
>> On 2012-02-15 18:23, Luiz Capitulino wrote:
>>> 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 :)
>>
>> (see the other thread)
> 
> Yeah, I saw it and will fix the problems you've pointed out.
> 
>>>>>  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.
>>
>> What object represent a QMP session now? 
> 
> We don't exactly have the notion of a QMP session today, but all QMP state
> is currently stored in the Monitor object.
> 
>> That object will once hold the
>> reference to the FDs. So some qmp_get_fd will take that session and
>> return the requested fd - so, it does make sense, long-term at least.
> 
> Yes. Actually most of the code has already been written by Anthony:
> 
>  git://repo.or.cz/qemu/aliguori.git glib
> 
>  (look at qmp-core.c)
> 
> What I'm doing is to rebase it, do some integration work & fix ups.
> 
>> In any case, as long as everyone can mess with cur_mon, you don't need
>> to introduce wrappers that just link a normal monitor service with that
>> variable.
> 
> So, you're suggestion to just use monitor_get_fd(), right?

As qemu_get_fd is obviously not to stay, yes. That reduces the risk of
proliferation (or pattern replication like qemu_monitor_suspend/resume).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux