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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2981 bytes --]

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

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]