From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:40455) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RbAWQ-0003op-TS for qemu-devel@nongnu.org; Thu, 15 Dec 2011 07:34:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RbAWL-0000x7-2U for qemu-devel@nongnu.org; Thu, 15 Dec 2011 07:34:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51683) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RbAWK-0000wt-Ng for qemu-devel@nongnu.org; Thu, 15 Dec 2011 07:34:45 -0500 Message-ID: <4EE9EA1F.6020206@redhat.com> Date: Thu, 15 Dec 2011 13:37:51 +0100 From: Kevin Wolf MIME-Version: 1.0 References: <1323784351-25531-1-git-send-email-stefanha@linux.vnet.ibm.com> <1323784351-25531-9-git-send-email-stefanha@linux.vnet.ibm.com> <4EE8B8BC.2040804@redhat.com> <20111215082758.GD26425@stefanha-thinkpad.localdomain> <4EE9CD1F.4010107@redhat.com> <20111215093018.79bc6814@doriath> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 8/9] qmp: add query-block-jobs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org, Marcelo Tosatti , Stefan Hajnoczi , Luiz Capitulino Am 15.12.2011 13:00, schrieb Stefan Hajnoczi: > On Thu, Dec 15, 2011 at 11:30 AM, Luiz Capitulino > wrote: >> On Thu, 15 Dec 2011 11:34:07 +0100 >> Kevin Wolf wrote: >> >>> Am 15.12.2011 09:27, schrieb Stefan Hajnoczi: >>>> On Wed, Dec 14, 2011 at 03:54:52PM +0100, Kevin Wolf wrote: >>>>> Am 13.12.2011 14:52, schrieb Stefan Hajnoczi: >>>>>> diff --git a/hmp.c b/hmp.c >>>>>> index 66d9d0f..c16d6a1 100644 >>>>>> --- a/hmp.c >>>>>> +++ b/hmp.c >>>>>> @@ -499,6 +499,46 @@ void hmp_info_pci(Monitor *mon) >>>>>> qapi_free_PciInfoList(info); >>>>>> } >>>>>> >>>>>> +void hmp_info_block_jobs(Monitor *mon) >>>>>> +{ >>>>>> + BlockJobInfoList *list; >>>>>> + Error *err = NULL; >>>>>> + >>>>>> + list = qmp_query_block_jobs(&err); >>>>>> + assert(!err); >>>>>> + >>>>>> + if (!list) { >>>>>> + monitor_printf(mon, "No active jobs\n"); >>>>>> + return; >>>>>> + } >>>>>> + >>>>>> + while (list) { >>>>>> + /* The HMP output for streaming jobs is special because historically it >>>>>> + * was different from other job types so applications may depend on the >>>>>> + * exact string. >>>>>> + */ >>>>> >>>>> Er, what? This is new code. What HMP clients use this string? I know >>>>> that libvirt already got support for this before we implemented it, but >>>>> shouldn't that be QMP only? >>>> >>>> Libvirt HMP uses this particular string, which turned out to be >>>> sub-optimal once I realized we might support other types of block jobs >>>> in the future. >>>> >>>> You can still build libvirt HMP-only by disabling the yajl library >>>> dependency. The approach I've taken is to make the interfaces available >>>> over both HMP and QMP (and so has the libvirt-side code). >>>> >>>> In any case, we have defined both HMP and QMP. Libvirt implements both >>>> and I don't think there's a reason to provide only QMP. >>>> >>>> Luiz: For future features, are we supposed to provide only QMP >>>> interfaces, not HMP? >>> >>> Of course, qemu should provide them as HMP command. But libvirt >>> shouldn't use HMP commands. HMP is intended for human users, not as an >>> API for management. >> >> That's correct. >> >> What defines if you're going to have a HMP version of a command is if >> you expect it to be used by humans and if you do all its output and >> arguments should be user friendly. You should never expect nor assume >> it's going to be used by a management interface. > > Okay, thanks Kevin and Luiz for explaining. In this case I know it is > used by a management interface because libvirt has code to use it. > > I was my mistake to include the HMP side as part of the "API" but it's > here now and I think we can live with this. We probably can, but I would prefer fixing it in libvirt. Possibly the right fix there would be to remove it entirely from the HMP code - if I understand right, the HMP code is only meant to support older qemu versions anyway. I also don't quite understand why there even is an option to disable QMP in libvirt, we have always told that HMP will become unstable. Kevin