From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=50099 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PhPeN-00041E-NT for qemu-devel@nongnu.org; Mon, 24 Jan 2011 11:52:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PhPeL-0000cw-Rb for qemu-devel@nongnu.org; Mon, 24 Jan 2011 11:52:19 -0500 Received: from e2.ny.us.ibm.com ([32.97.182.142]:52800) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PhPeL-0000cM-P9 for qemu-devel@nongnu.org; Mon, 24 Jan 2011 11:52:17 -0500 Received: from d01dlp02.pok.ibm.com (d01dlp02.pok.ibm.com [9.56.224.85]) by e2.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id p0OGY7p7008079 for ; Mon, 24 Jan 2011 11:34:25 -0500 Received: from d01relay06.pok.ibm.com (d01relay06.pok.ibm.com [9.56.227.116]) by d01dlp02.pok.ibm.com (Postfix) with ESMTP id EB9304DE804A for ; Mon, 24 Jan 2011 11:48:22 -0500 (EST) Received: from d01av04.pok.ibm.com (d01av04.pok.ibm.com [9.56.224.64]) by d01relay06.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id p0OGpohn1560600 for ; Mon, 24 Jan 2011 11:51:50 -0500 Received: from d01av04.pok.ibm.com (loopback [127.0.0.1]) by d01av04.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id p0OGpniu000456 for ; Mon, 24 Jan 2011 11:51:49 -0500 Message-ID: <4D3DAE10.1090000@linux.vnet.ibm.com> Date: Mon, 24 Jan 2011 10:51:28 -0600 From: Michael Roth MIME-Version: 1.0 References: <1295270117-24760-1-git-send-email-mdroth@linux.vnet.ibm.com> <1295270117-24760-8-git-send-email-mdroth@linux.vnet.ibm.com> <4D39B668.4040806@redhat.com> <4D39C8A6.7000404@linux.vnet.ibm.com> <4D3D5185.40900@redhat.com> In-Reply-To: <4D3D5185.40900@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [RFC][PATCH v6 07/23] virtagent: base server definitions List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jes Sorensen Cc: agl@linux.vnet.ibm.com, stefanha@linux.vnet.ibm.com, abeekhof@redhat.com, marcel.mittelstaedt@de.ibm.com, qemu-devel@nongnu.org, aliguori@linux.vnet.ibm.com, ryanh@us.ibm.com, markus_mueller@de.ibm.com On 01/24/2011 04:16 AM, Jes Sorensen wrote: > On 01/21/11 18:55, Michael Roth wrote: >> On 01/21/2011 10:38 AM, Jes Sorensen wrote: >>>> +#include >>>> +#include >>>> + >>>> +#define GUEST_AGENT_SERVICE_ID "virtagent" >>>> +#define GUEST_AGENT_PATH "/tmp/virtagent-guest.sock" >>>> +#define HOST_AGENT_SERVICE_ID "virtagent-host" >>>> +#define HOST_AGENT_PATH "/tmp/virtagent-host.sock" >>>> +#define VA_GETFILE_MAX 1<< 30 >>>> +#define VA_FILEBUF_LEN 16384 >>>> +#define VA_DMESG_LEN 16384 >>> >>> I really don't like these hard coded constants - you you have a command >>> line interface allowing for the change of the sockets and file names? >>> Otherwise you'll hit problems on the host side with concurrent runs of >>> qemu. >> >> Yup, that's one of the TODOs. In terms of configuration we can add >> parameters to the chardev to override these, but the goal here is sane >> defaults to avoid unnecessarily complicated invocations. > > As a sane default, using.pid or something along those lines is > better. It is very common to run more than one qemu instance at a time. > Sorry, wasn't clear here. Using a pid by default to differentiate per-qemu instances of virtagent is a specific TODO, but it is currently configurable via the commandline as well: qemu -chardev virtagent,path=/tmp/qemu-guest1-virtagent.sock,... >>> I really would like to see the dmesg stuff removed too for now as we >>> discussed earlier. >> >> I think as a development/support tool it has a recently strong use case, >> even given it's limitations (which are not so bad....we retrieve up to a >> max of 16KB, possibly less depending on guest configuration, so it's not >> entirely predictable, but it's not dangerous. It's platform-specific, >> but that's handled by capabilities negotiation). > > There is plenty of good ways to do the same thing, copy file to host, > then view is just as easy and can be scripted, without the security > implications of having it inline. > >> I just don't really see the downside to keeping it in. > > It's obviously contentious, and it is not core functionality. In order > to get the patches adapted upstream it would easy the process to remove > it and keep it as a separate patch. Fair enough, the proposed copyfile replacement would be suitable as well. My main concern is stripping away too much functionality for the initial merge, since guest-initiated shutdown is all we'd really have left lacking viewdmesg/viewfile. Would it be better to get copyfile in for the initial set of patches, or as a subsequent set? > > Cheers, > Jes