Re: [Qemu-devel] [PATCH v8 7/7] block: Enable qemu_open/close to work with fd sets

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

On 08/10/2012 12:56 PM, Corey Bryant wrote:
>
>
> On 08/10/2012 12:34 PM, Kevin Wolf wrote:
>> Am 10.08.2012 04:10, schrieb Corey Bryant:
>>> When qemu_open is passed a filename of the "/dev/fdset/nnn"
>>> format (where nnn is the fdset ID), an fd with matching access
>>> mode flags will be searched for within the specified monitor
>>> fd set.  If the fd is found, a dup of the fd will be returned
>>> from qemu_open.
>>>
>>> Each fd set has a reference count.  The purpose of the reference
>>> count is to determine if an fd set contains file descriptors that
>>> have open dup() references that have not yet been closed.  It is
>>> incremented on qemu_open and decremented on qemu_close.  It is
>>> not until the refcount is zero that file desriptors in an fd set
>>> can be closed.  If an fd set has dup() references open, then we
>>> must keep the other fds in the fd set open in case a reopen
>>> of the file occurs that requires an fd with a different access
>>> mode.
>>>
>>> Signed-off-by: Corey Bryant <coreyb@linux.vnet.ibm.com>
>>
>>> @@ -78,6 +79,69 @@ int qemu_madvise(void *addr, size_t len, int advice)
>>>   #endif
>>>   }
>>>
>>> +/*
>>> + * Dups an fd and sets the flags
>>> + */
>>> +static int qemu_dup(int fd, int flags)
>>
>> qemu_dup() is probably not a good name. We'll want to use it when we
>> need to get a wrapper around dup(). And I suspect that we will need it
>> for making bdrv_reopen() compatible with fdset refcounting.
>>
>
> Do you you have a suggestion for a name?
>
>>> +{
>>> +    int ret;
>>> +    int serrno;
>>> +    int dup_flags;
>>> +    int setfl_flags;
>>> +
>>> +    if (flags & O_CLOEXEC) {
>>> +#ifdef F_DUPFD_CLOEXEC
>>> +        ret = fcntl(fd, F_DUPFD_CLOEXEC, 0);
>>> +#else
>>> +        ret = dup(fd);
>>> +        if (ret != -1) {
>>> +            qemu_set_cloexec(ret);
>>> +        }
>>> +#endif
>>> +    } else {
>>> +        ret = dup(fd);
>>> +    }
>>
>> qemu_open() is supposed to add O_CLOEXEC by itself, so I think we should
>> execute the then branch unconditionally (or we would have to change the
>> qemu_dup() call below to add it - but the fact that O_CLOEXEC isn't even
>> necessarily defined doesn't help with that).
>
> Sure I can modify this to always add O_CLOEXEC.  (I know you mentioned
> this before but I think I interpreted the comment as a question to others.)
>
> Do you also want me to modify qemu_open to always add O_CLOEXEC?
>

My mistake.. it's always set in qemu_open already.

-- 
Regards,
Corey