From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:59099) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rxj5U-00043f-Bm for qemu-devel@nongnu.org; Wed, 15 Feb 2012 12:56:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Rxj5P-0003Rf-22 for qemu-devel@nongnu.org; Wed, 15 Feb 2012 12:56:16 -0500 Received: from thoth.sbs.de ([192.35.17.2]:21346) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Rxj5O-0003R7-NT for qemu-devel@nongnu.org; Wed, 15 Feb 2012 12:56:11 -0500 Message-ID: <4F3BF1B5.5050805@siemens.com> Date: Wed, 15 Feb 2012 18:56:05 +0100 From: Jan Kiszka MIME-Version: 1.0 References: <1328902266-25308-1-git-send-email-lcapitulino@redhat.com> <4F3B73DB.4010105@siemens.com> <20120215104954.1bce66da@doriath.home> <4F3BB47C.5090304@siemens.com> <20120215152333.1beb96b1@doriath.home> <4F3BEDD3.5070307@siemens.com> <20120215154931.5253ffae@doriath.home> In-Reply-To: <20120215154931.5253ffae@doriath.home> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 0/6] qapi: Convert migrate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: "aliguori@us.ibm.com" , "qemu-devel@nongnu.org" , "quintela@redhat.com" On 2012-02-15 18:49, Luiz Capitulino wrote: > On Wed, 15 Feb 2012 18:39:31 +0100 > Jan Kiszka wrote: > >> On 2012-02-15 18:23, Luiz Capitulino wrote: >>> On Wed, 15 Feb 2012 14:34:52 +0100 >>> Jan Kiszka wrote: >>> >>>> On 2012-02-15 13:49, Luiz Capitulino wrote: >>>>> On Wed, 15 Feb 2012 09:59:07 +0100 >>>>> Jan Kiszka 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