On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant <coreyb@xxxxxxxxxxxxxxxxxx> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list