From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:35045) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzSjW-0007xu-CJ for qemu-devel@nongnu.org; Thu, 09 Aug 2012 09:25:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SzSjS-0007bg-T0 for qemu-devel@nongnu.org; Thu, 09 Aug 2012 09:25:02 -0400 Received: from e32.co.us.ibm.com ([32.97.110.150]:33283) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SzSjS-0007bS-M9 for qemu-devel@nongnu.org; Thu, 09 Aug 2012 09:24:58 -0400 Received: from /spool/local by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 9 Aug 2012 07:24:56 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id E86423E4007B for ; Thu, 9 Aug 2012 13:23:49 +0000 (WET) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q79DNeOH092798 for ; Thu, 9 Aug 2012 07:23:44 -0600 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q79DOuNc022953 for ; Thu, 9 Aug 2012 07:24:56 -0600 Message-ID: <5023B9D9.5050105@linux.vnet.ibm.com> Date: Thu, 09 Aug 2012 09:23:37 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344355108-14786-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344355108-14786-3-git-send-email-coreyb@linux.vnet.ibm.com> <5023B5B8.8040604@redhat.com> In-Reply-To: <5023B5B8.8040604@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [libvirt] [PATCH v7 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, Stefan Hajnoczi , qemu-devel@nongnu.org On 08/09/2012 09:06 AM, Eric Blake wrote: > On 08/09/2012 03:04 AM, Stefan Hajnoczi wrote: >> On Tue, Aug 7, 2012 at 4:58 PM, Corey Bryant wrote: >>> +## >>> +# @FdsetFdInfo: >>> +# >>> +# Information about a file descriptor that belongs to an fd set. >>> +# >>> +# @fd: The file descriptor value. >>> +# >>> +# @removed: If true, the remove-fd command has been issued for this fd. >>> +# >>> +# Since: 1.2.0 >>> +## >>> +{ 'type': 'FdsetFdInfo', 'data': {'fd': 'int', 'removed': 'bool'} } >> >> I'm not sure if the removed field is useful, especially since >> remove-fd is idempotent (you can keep querying fds and then marking >> them removed again until they finally close). The reason I suggest >> minimizing the schema is so that we can change implementation details >> later without having to synthesize this state. > > Pretty much agreed - rather than listing 'removed', omitting the fd by > default will match the monitors expectations ("why are you telling me > about an fd I told you to remove?"). The only reason I could see for > keeping it would be for debug purposes, but that would be debugging of > qemu, not the application connected to the monitor, at which point gdb > debugging is probably better. > Thanks for the input! I'll go ahead and drop removed fd's from the query-fdsets output. >>> +{ 'type': 'FdsetInfo', >>> + 'data': {'fdset_id': 'int', 'refcount': 'int', 'in_use': 'bool', >>> + 'fds': ['FdsetFdInfo']} } >> >> Why are refcount and in_use exposed? How will applications use them? >> This seems like internal state to me. > > refcount _might_ be useful for knowing whether qemu is actively using > the fdset. For example, in the sequence: > > add-fd nnn > drive-add > > if libvirtd crashes after sending drive-add but before getting a > response, then on restart it has to figure out if the drive-add took or > failed. A non-zero refcount means it took. But then again, so does a > query-block. I like the approach of being minimal until we prove we > need it, and can't right now think of anything where having a refcount > would tell libvirt anything new that it can't already learn from > somewhere else. > I'll also drop both in_use and refcount. I was already planning on dropping in_use because at this point it's always true anyway. >> >> Should add-fd allow the application to associate an opaque string with >> the fdset? This could be used to recover after crash. Otherwise the >> application needs to store the fdset_id -> filename mapping in a file >> on disk and hope it was safely stored before crash. It seems like the >> best place to keep this info is with the fdset. > > Very nice idea! In fact, even nicer than returning a markup of O_RDONLY > - if the management app cares about knowing details on an fd, such as > whether it is read-only, then an opaque string tied to the fd in the > fdset becomes very useful. The string needs to be optional on add-fd. > (Libvirt might even use an xml-like string like " file='/path/to/file' mode='O_RDONLY'/>", but of course, being an opaque > string, qemu doesn't have to care what the string contains.) > Yes this makes a lot of sense. I'll add the opaque string support. Since the client can put the access mode in the opaque string then I won't add support to QEMU to return the access-mode for each fd on query-fdsets. -- Regards, Corey