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

[Eric Blake <eblake@redhat.com>]

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

On 08/09/2012 08:10 PM, 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.
> 
> 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

s/desriptors/descriptors/

> 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.
> 

> +int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> +{
> +    MonFdset *mon_fdset;
> +    MonFdsetFd *mon_fdset_fd;
> +    int mon_fd_flags;
> +
> +    QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> +        if (mon_fdset->id != fdset_id) {
> +            continue;
> +        }
> +        QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) {
> +            if (mon_fdset_fd->removed) {
> +                continue;
> +            }

Is this right?  According to the commit message, the whole point of
leaving an fd in the set is to allow the fd to be reused as a dup()
target for as long as the fdset is alive, even if the monitor no longer
cares about the existence of the fd.  But this will always skip over an
fd marked for removal.  Maybe this function needs a flag to say whether
this is an initial open driven by an explicit user string (in which
case, honor the removed flag - if the user removed the O_RDWR fd and
then tries a drive_add with the same fdset, the drive_add should fail
because from the user's perspective, there is no O_RDWR fd in the set);
vs. an internal usage due to a reopen (use an fd even if removed is
true, because we may be toggling between O_RDWR and O_RDONLY multiple
times long after the monitor has already removed the fdset, based on
actions that were not drive by an explicit /dev/fdset name.)

> +
> +            mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL);
> +            if (mon_fd_flags == -1) {
> +                return -1;
> +            }
> +
> +            if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> +                return mon_fdset_fd->fd;
> +            }

I still wonder if a request for O_RDONLY should be satisfied by an
existing O_RDWR fd, especially in light of the fact that libvirt would
rather pass in only one RDWR fd but qemu block opening currently opens
twice during probing.  But if it turns out to be a problem in practice,
and if libvirt can't really manage to pass two fds into the set, we can
hack that in later.  Meanwhile, I'm okay with this first round patch
requiring an exact match.

> @@ -87,6 +151,39 @@ 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(fd, flags);
> +        if (fd == -1) {
> +            return -1;
> +        }
> +
> +        ret = monitor_fdset_dup_fd_add(fdset_id, dupfd);
> +        if (ret == -1) {
> +            return -1;

Leaks dupfd (admittedly only on a corner-case failure, but still worth
addressing).

-- 
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 --]