Re: [libvirt] [PATCH v5 6/6] block: Enable qemu_open/close to work with fd sets

On 07/23/2012 07:08 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. > > 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. > > +++ b/monitor.c > @@ -2551,6 +2551,91 @@ static void monitor_fdsets_set_in_use(Monitor *mon, bool in_use) > } > } > > +void monitor_fdset_increment_refcount(Monitor *mon, int64_t fdset_id) > +{ > + mon_fdset_t *mon_fdset; > + > + if (!mon) { > + return; > + } Am I reading this code right by stating that 'if there is no monitor, we don't increment the refcount'? How does a monitor reattach affect things? Or am I missing something fundamental about the cases when 'mon==NULL' will exist?

> +int monitor_fdset_get_fd(Monitor *mon, int64_t fdset_id, int flags) > +{ > + mon_fdset_t *mon_fdset; > + mon_fdset_fd_t *mon_fdset_fd; > + int mon_fd_flags; > + > + if (!mon) { > + errno = ENOENT; > + return -1; > + } > + > + 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; > + } > + > + mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL); > + if (mon_fd_flags == -1) { > + return -1; This says we fail on the first fcntl() failure, instead of trying other fds in the set. Granted, an fcntl() failure is probably the sign of a bigger bug (such as closing an fd at the wrong point in time), so I guess trying to go on doesn't make much sense once we already know we are hosed.

> + } > + > + switch (flags & O_ACCMODE) { > + case O_RDWR: > + if ((mon_fd_flags & O_ACCMODE) == O_RDWR) { > + return mon_fdset_fd->fd; > + } > + break; > + case O_RDONLY: > + if ((mon_fd_flags & O_ACCMODE) == O_RDONLY) { > + return mon_fdset_fd->fd; > + } > + break; Do we want to allow the case where the caller asked for O_RDONLY, but the set only has O_RDWR? After all, the caller is getting a compatible subset of what the set offers.

> + case O_WRONLY: > + if ((mon_fd_flags & O_ACCMODE) == O_WRONLY) { > + return mon_fdset_fd->fd; > + } > + break; Likewise, should we allow a caller asking for O_WRONLY when the set provides only O_RDWR?

> > +/* > + * Dups an fd and sets the flags > + */ > +static int qemu_dup(int fd, int flags) > +{ > + int i; > + int ret; > + int serrno; > + int dup_flags; > + int setfl_flags[] = { O_APPEND, O_ASYNC, O_DIRECT, O_NOATIME, > + O_NONBLOCK, 0 }; > + > + if (flags & O_CLOEXEC) { > + ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); F_DUPFD_CLOEXEC is required by POSIX but not implemented on all modern OS yet; you probably need some #ifdef and/or configure guards.

> + if (ret == -1 && errno == EINVAL) { > + ret = dup(fd); > + if (ret != -1 && fcntl_setfl(ret, O_CLOEXEC) == -1) { You _can't_ call F_SETFL with O_CLOEXEC. O_CLOEXEC only causes open() to set FD_CLOEXEC; thereafter, including in the case of this dup, what you want to do is instead set FD_CLOEXEC via F_SETFD (aka call qemu_set_cloexec, not fcntl_setfl).

> + goto fail; > + } > + } > + } else { > + ret = dup(fd); > + } > + > + if (ret == -1) { > + goto fail; > + } > + > + dup_flags = fcntl(ret, F_GETFL); > + if (dup_flags == -1) { > + goto fail; > + } > + > + if ((flags & O_SYNC) != (dup_flags & O_SYNC)) { > + errno = EINVAL; > + goto fail; > + } > + > + /* Set/unset flags that we can with fcntl */ > + i = 0; > + while (setfl_flags[i] != 0) { > + if (flags & setfl_flags[i]) { > + dup_flags = (dup_flags | setfl_flags[i]); > + } else { > + dup_flags = (dup_flags & ~setfl_flags[i]); > + } > + i++; > + } Rather than looping one bit at a time, you should do this as a mask operation.

> + > + if (fcntl(ret, F_SETFL, dup_flags) == -1) { > + goto fail; > + } > + > + /* Truncate the file in the cases that open() would truncate it */ > + if (flags & O_TRUNC || > + ((flags & (O_CREAT | O_EXCL)) == (O_CREAT | O_EXCL))) { > + if (ftruncate(ret, 0) == -1) { > + goto fail; > + } > + } > + > + qemu_set_cloexec(ret); If we're going to blindly set FD_CLOEXEC at the end of the day, rather than try to honor O_CLOEXEC, then why not simplify the beginning of this function:

ret = fcntl(fd, F_DUPFD_CLOEXEC, 0); if (ret == -1 && errno == EINVAL) { ret = dup(fd); if (ret != -1) { qemu_set_cloexec(ret); } } if (ret == -1) { goto fail; }