2:43 p.m.
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(a)redhat.com +1-919-301-3266
Libvirt virtualization library http://libvirt.org