Re: [libvirt] [Qemu-devel] [PATCH v6 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets

On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant <coreyb(a)linux.vnet.ibm.com&gt; 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.