Re: [libvirt] [PATCH v7 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets

On 08/09/2012 03:04 AM, Stefan Hajnoczi wrote: > On Tue, Aug 7, 2012 at 4:58 PM, Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; 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.

>> +{ '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.

> > 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 "<fd file='/path/to/file' mode='O_RDONLY'/>", but of course, being an opaque string, qemu doesn't have to care what the string contains.)