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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

On 08/11/2012 07:14 AM, Corey Bryant wrote:
> 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.
> 

> v9:
>  -Drop fdset refcount and check dup_fds instead. (eblake@redhat.com)
>  -Fix dupfd leak in qemu_dup(). (eblake@redhat.com)
>  -Always set O_CLOEXEC in qemu_dup(). (kwolf@redhat.com)
>  -Change name of qemu_dup() to qemu_dup_flags(). (kwolf@redhat.com)
> 

> @@ -87,6 +146,40 @@ 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(fdset_id, flags);
> +        if (fd == -1) {
> +            return -1;
> +        }
> +
> +        dupfd = qemu_dup_flags(fd, flags);
> +        if (fd == -1) {

Checking the wrong condition:
s/fd/dupfd/

> +            return -1;
> +        }
> +
> +        ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
> +        if (ret == -1) {
> +            close(dupfd);
> +            return -1;

This function appears to promise a reasonable errno on failure.
However, I don't think monitor_fdset_dup_fd_add guarantees a reasonable
errno, and even if it does, close() can corrupt errno.  I think that
prior to returning here, you either need an explicit errno=ENOMEM, or
fix monitor_fdset_dup_fd to guarantee a nice errno, plus a save and
restore of errno here.  Unless no one cares about errno on failure, in
which case your earlier errno=EINVAL can be dropped.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]