From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56804) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUB2M-0005Xg-MZ for qemu-devel@nongnu.org; Mon, 22 Apr 2013 03:19:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UUB2I-0008VW-Ui for qemu-devel@nongnu.org; Mon, 22 Apr 2013 03:19:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9659) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UUB2I-0008Uq-I1 for qemu-devel@nongnu.org; Mon, 22 Apr 2013 03:19:38 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3M7JbjK027299 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 22 Apr 2013 03:19:37 -0400 Message-ID: <5174E487.6000200@redhat.com> Date: Mon, 22 Apr 2013 09:19:35 +0200 From: Gerd Hoffmann MIME-Version: 1.0 References: <1366275680-15416-1-git-send-email-kraxel@redhat.com> <516FE2AB.5060304@redhat.com> <20130418112106.36ca6c51@redhat.com> <5170129A.8010807@redhat.com> In-Reply-To: <5170129A.8010807@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RfC PATCH 0/5] console: qom-ify & extent screendump monitor command List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Luiz Capitulino On 04/18/13 17:34, Eric Blake wrote: > On 04/18/2013 09:21 AM, Luiz Capitulino wrote: >>>> Question for the libvirt guys: Is it ok for libvirt to just extend the >>>> existing screendump command? Can libvirt figure there is a new >>>> (optional) parameter? See patch #5. >>> > >>> So yes, I think libvirt will be able to drive the new command by knowing >>> how many heads appear per device, then passing in the appropriate named >>> device to the QMP command. And yes, I'll review patch 5 regarding >>> interface design. >> >> We can extend screendump on HMP, but the general rule for QMP is to add a >> new command instead so that clients don't have to play tricks like Eric is >> suggesting :) Ahem. It didn't sound like libvirt plays tricks there, to me it simply looked like an description of how libvirt manages devices + heads and the confirmation that libvirt indeed know which device it wants a screenshot from. We also have to care to not mix up two things: #1 is whenever we'll go extent screendump or add screendump2 (see sub-thread started by markus reply). #2 the actual design of the new/extended command. > The problem at hand is that your proposal in patch 5: > > -{ 'command': 'screendump', 'data': {'filename': 'str'} } > +{ 'command': 'screendump', 'data': {'filename': 'str', > + '*device' : 'str'} } > > still doesn't support the case of dumping just one head of a multi-head > device. qxl is the only device with multihead support, and it works by defining a large framebuffer and defining the heads as rectangles within this framebuffer: +-------------------------------------------+ | framebuffer | | +-------------+ +----------+ | | | head 1 | | head 2 | | | | | | | | | +-------------+ +----------+ | +-------------------------------------------+ (in practice you wouldn't have unused space around the heads of course, but it's easier to draw this way ;) So a recent spice client will get the head configuration info, then open two windows, one for each head. A old spice client without multihead support will create a single window covering the whole framebuffer instead. Asking qemu to screendump a multihead qxl device will likewise write out a dump of the whole framebuffer, i.e. the dump will have *all* heads. > device selection. Libvirt uses query-commands to determine whether new So query-commands doesn't list the arguments supported by a command, only the commands themself. > Option 2 is probably slightly nicer for guaranteeing a sane error > message back to the user, but option 1 (the approach of this series) > still seems workable. Sane error messaging is good, I'd still prefer Option 1 though, to get both we'll need a new query-arguments command I guess ... cheers, Gerd