[libvirt] [Qemu-devel] [PATCH v4 3/7] qapi: Add pass-fd QMP command

[Corey Bryant]

On 06/22/2012 04:24 PM, Eric Blake wrote:
> On 06/22/2012 12:36 PM, Corey Bryant wrote:
>> This patch adds the pass-fd QMP command using the QAPI framework.
>> Like the getfd command, it is used to pass a file descriptor via
>> SCM_RIGHTS and associate it with a name.  However, the pass-fd
>> command also returns the received file descriptor, which is a
>> difference in behavior from the getfd command, which returns
>> nothing.  pass-fd also supports a boolean "force" argument that
>> can be used to specify that an existing file descriptor is
>> associated with the specified name, and that it should become a
>> copy of the newly passed file descriptor.
>
>> +        if (replace) {
>> +            /* the existing fd is replaced with the new fd */
>> +            close(monfd->fd);
>> +            monfd->fd = fd;
>> +            return fd;
>> +        }
>> +
>> +        if (copy) {
>
> Since 'replace' and 'copy' are never both true, should this instead be a
> tri-state enum argument instead of two independent bool arguments?
>

Sure, that makes sense.  I'll do that in v5.

>> +            /* the existing fd becomes a copy of the new fd */
>> +            if (dup2(fd, monfd->fd) == -1) {
>
> Broken - you want to use dup3(fd, monfd->fd, O_CLOEXEC) (and fall back
> to dup2()+fcntl(F_GETFD/F_SETFD) on platforms where dup3 is not
> available; it has been proposed for the next revision of POSIX but right
> now is pretty much limited to Linux and Cygwin).  Otherwise, you are
> undoing the fact that patch 1/7 just changed the 'getfd' list to always
> keep all its fds marked cloexec.
>

Thanks for catching this.  I'll fix this in v5.  In terms of platforms 
that support dup3 vs dup2, I'm assuming the following preprocessor 
checks will do what we need:

#if defined(__linux__) || defined(__CYGWIN__)
dup3(fd, monfd->fd, O_CLOEXEC)
#else
dup2()+fcntl(F_GETFD/F_SETFD)
#endif

-- 
Regards,
Corey