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

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

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 <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.
>

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 "<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.)
>

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