On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant <coreyb@linux.vnet.ibm.com> wrote:
diff --git a/monitor.c b/monitor.c index 49dccfe..9aa9f7e 100644 --- a/monitor.c +++ b/monitor.c @@ -140,6 +140,24 @@ struct mon_fd_t { QLIST_ENTRY(mon_fd_t) next; };
+/* file descriptor associated with a file descriptor set */ +typedef struct mon_fdset_fd_t mon_fdset_fd_t; +struct mon_fdset_fd_t {
QEMU coding style is: typedef struct MonFdsetFd MonFdsetFd; struct MonFdsetFd { See ./CODING_STYLE for more info.
+ int fd; + bool removed; + QLIST_ENTRY(mon_fdset_fd_t) next; +}; + +/* file descriptor set containing fds passed via SCM_RIGHTS */ +typedef struct mon_fdset_t mon_fdset_t; +struct mon_fdset_t { + int64_t id; + int refcount; + bool in_use; + QLIST_HEAD(, mon_fdset_fd_t) fds; + QLIST_ENTRY(mon_fdset_t) next; +};
At this point in the patch series it's not clear to me whether the removed and refcount/in_use fields are a clean and correct solution. Exposing these fields via QMP is also something I'm going to carefully review because they seem like internals.
+ typedef struct MonitorControl { QObject *id; JSONMessageParser parser; @@ -176,7 +194,8 @@ struct Monitor { int print_calls_nr; #endif QError *error; - QLIST_HEAD(,mon_fd_t) fds; + QLIST_HEAD(, mon_fd_t) fds; + QLIST_HEAD(, mon_fdset_t) fdsets; QLIST_ENTRY(Monitor) entry; };
@@ -2389,6 +2408,157 @@ int monitor_get_fd(Monitor *mon, const char *fdname) return -1; }
+static void monitor_fdset_cleanup(mon_fdset_t *mon_fdset) +{ + mon_fdset_fd_t *mon_fdset_fd; + mon_fdset_fd_t *mon_fdset_fd_next; + + if (mon_fdset->refcount != 0) { + return; + } + + QLIST_FOREACH_SAFE(mon_fdset_fd, &mon_fdset->fds, next, mon_fdset_fd_next) { + if (!mon_fdset->in_use || mon_fdset_fd->removed) { + close(mon_fdset_fd->fd); + QLIST_REMOVE(mon_fdset_fd, next); + g_free(mon_fdset_fd); + } + } + + if (QLIST_EMPTY(&mon_fdset->fds)) { + QLIST_REMOVE(mon_fdset, next); + g_free(mon_fdset); + } +} + +AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, Error **errp) +{ + int fd; + Monitor *mon = cur_mon; + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *mon_fdset_fd; + AddfdInfo *fdinfo; + + fd = qemu_chr_fe_get_msgfd(mon->chr); + if (fd == -1) { + qerror_report(QERR_FD_NOT_SUPPLIED); + return NULL; + } + + if (has_fdset_id) { + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id == fdset_id) { + break; + } + } + if (mon_fdset == NULL) { + qerror_report(QERR_FDSET_NOT_FOUND, fdset_id); + return NULL;
fd is leaked?
+ } + } else { + int64_t fdset_id_prev = -1; + mon_fdset_t *mon_fdset_cur = QLIST_FIRST(&mon->fdsets); + + /* Use first available fdset ID */ + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + mon_fdset_cur = mon_fdset; + if (fdset_id_prev == mon_fdset_cur->id - 1) { + fdset_id_prev = mon_fdset_cur->id; + continue; + } + break; + } + + mon_fdset = g_malloc0(sizeof(*mon_fdset)); + mon_fdset->id = fdset_id_prev + 1; + mon_fdset->refcount = 0; + mon_fdset->in_use = true; + + /* The fdset list is ordered by fdset ID */ + if (mon_fdset->id == 0) { + QLIST_INSERT_HEAD(&mon->fdsets, mon_fdset, next); + } else if (mon_fdset->id < mon_fdset_cur->id) { + QLIST_INSERT_BEFORE(mon_fdset_cur, mon_fdset, next); + } else { + QLIST_INSERT_AFTER(mon_fdset_cur, mon_fdset, next); + } + } + + mon_fdset_fd = g_malloc0(sizeof(*mon_fdset_fd)); + mon_fdset_fd->fd = fd; + mon_fdset_fd->removed = false; + QLIST_INSERT_HEAD(&mon_fdset->fds, mon_fdset_fd, next); + + fdinfo = g_malloc0(sizeof(*fdinfo)); + fdinfo->fdset_id = mon_fdset->id; + fdinfo->fd = mon_fdset_fd->fd; + + return fdinfo; +} + +void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp) +{ + Monitor *mon = cur_mon; + mon_fdset_t *mon_fdset; + mon_fdset_fd_t *mon_fdset_fd; + char fd_str[20]; + + QLIST_FOREACH(mon_fdset, &mon->fdsets, next) { + if (mon_fdset->id != fdset_id) { + continue; + } + QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { + if (has_fd && mon_fdset_fd->fd != fd) { + continue; + } + mon_fdset_fd->removed = true; + if (has_fd) { + break; + } + } + monitor_fdset_cleanup(mon_fdset); + return; + } + snprintf(fd_str, sizeof(fd_str), "%ld", fd); + qerror_report(QERR_FD_NOT_FOUND, fd_str);
Why use an fd_str instead of passing an int64_t into the error message? This also assumed sizeof(long) == 8, which isn't true on 32-bit hosts, so %ld should be %"PRId64". There is a new policy on error reporting. I think this patch series may be affected/conflict, please qemu-devel to check. I think Luiz can also help here. Stefan