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; } -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org