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: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?

-- 
Regards,
Corey