[libvirt] [Qemu-devel] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets

[Corey Bryant]

On 08/06/2012 09:51 AM, Kevin Wolf wrote:
> Am 06.08.2012 15:32, schrieb Corey Bryant:
>> On 08/06/2012 05:15 AM, Kevin Wolf wrote:
>>> Am 03.08.2012 00:21, schrieb Corey Bryant:
>>>>>> @@ -84,6 +158,36 @@ int qemu_open(const char *name, int flags, ...)
>>>>>>         int ret;
>>>>>>         int mode = 0;
>>>>>>
>>>>>> +#ifndef _WIN32
>>>>>> +    const char *fdset_id_str;
>>>>>> +
>>>>>> +    /* Attempt dup of fd from fd set */
>>>>>> +    if (strstart(name, "/dev/fdset/", &fdset_id_str)) {
>>>>>> +        int64_t fdset_id;
>>>>>> +        int fd, dupfd;
>>>>>> +
>>>>>> +        fdset_id = qemu_parse_fdset(fdset_id_str);
>>>>>> +        if (fdset_id == -1) {
>>>>>> +            errno = EINVAL;
>>>>>> +            return -1;
>>>>>> +        }
>>>>>> +
>>>>>> +        fd = monitor_fdset_get_fd(default_mon, fdset_id, flags);
>>>>>
>>>>> I know that use of default_mon in this patch is not correct, but I
>>>>> wanted to get these patches out for review. I used default_mon for
>>>>> testing because cur_mon wasn't pointing to the monitor I'd added fd sets
>>>>> to.  I need to figure out why.
>>>>>
>>>>
>>>> Does it make sense to use default_mon here?  After digging into this
>>>> some more, I'm thinking it makes sense, and I'll explain why.
>>>>
>>>> It looks like cur_mon can't be used.  cur_mon will point to the monitor
>>>> object for the duration of a command, and be reset to old_mon (NULL in
>>>> my case) after the command completes.
>>>>
>>>> qemu_open() and qemu_close() are frequently called long after a monitor
>>>> command has come and gone, so cur_mon won't work.  For example,
>>>> drive_add will cause qemu_open() to be called, but after the command has
>>>> completed, the file will keep getting opened/closed during normal QEMU
>>>> operation.  I'm not sure why, I've just noticed this behavior.
>>>>
>>>> Does anyone have any thoughts on this?  It would require fd sets to be
>>>> added to the default monitor only.
>>>
>>> I think we have two design options that would make sense:
>>>
>>> 1. Make the file descriptors global instead of per-monitor. Is there a
>>>      reason why each monitor has its own set of fds? (Also I'm wondering
>>>      if they survive a monitor disconnect this way?)
>>
>> I'd prefer to have them associated with a monitor object so that we can
>> more easily keep track of whether or not they're in use by a monitor
>> connection.
>
> Hm, I see.
>
>>> 2. Save a monitor reference with the fdset information.
>>>
>>
>> Are you saying that each monitor would have the same copy of fdset
>> information?
>
> This one doesn't really make sense indeed...
>
>>
>>> Allowing to send file descriptors on every monitor, but making only
>>> those of the default monitor actually usable, sounds like a bad choice
>>> to me.
>>
>> What if we also allow them to be added only to the default monitor?
>
> Would get you some kind of consistency at least, even though I don't
> like that secondary monitors can't use the functionality.
>
> Can't we make the fdset information global, so that a qemu_open/close()
> searches all of them, but let it have a Monitor* owner for keeping track
> whether it's in use?

I think global fdsets might work (sorry I didn't think it through enough 
on my first reply).  I think I'll need to drop the "in_use" flag and tie 
monitor references into the refcount.  (I know I know, you suggested 
that a while back.. :).  I'll give it a shot and see how it goes.
-- 
Regards,
Corey