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

On 08/11/2012 07:14 AM, Corey Bryant wrote: > This patch adds support that enables passing of file descriptors > to the QEMU monitor where they will be stored in specified file > descriptor sets. > > > v9: > -Use fdset-id rather than fdset_id. (eblake(a)redhat.com) > -Update example for query-fdsets. (eblake(a)redhat.com) > -Close fd immediately on remove-fd. > (kwolf(a)redhat.com, eblake(a)redhat.com) > -Drop fdset refcount, and check dup_fds instead (in a later patch). > (eblake(a)redhat.com) > -Move mon_refcount code to a later patch. (kwolf(a)redhat.com) > > +AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, > + const char *opaque, Error **errp) > +{ > + int fd; > + Monitor *mon = cur_mon; > + MonFdset *mon_fdset; > + MonFdsetFd *mon_fdset_fd; > + AddfdInfo *fdinfo; > + > + fd = qemu_chr_fe_get_msgfd(mon->chr); > + if (fd == -1) { > + error_set(errp, QERR_FD_NOT_SUPPLIED); > + goto error; > + } > + > + if (has_fdset_id) { > + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > + if (mon_fdset->id == fdset_id) { > + break; > + } > + } > + if (mon_fdset == NULL) { > + error_set(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", > + "an existing fdset-id or no fdset-id"); The 'no fdset-id' portion of this error message doesn't make sense - it can only be reached if has_fdset_id was true. > + > +void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp) > +{ > + MonFdset *mon_fdset; > + MonFdsetFd *mon_fdset_fd; > + char fd_str[60]; > + > + QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { ... > + } > + > +error: > + snprintf(fd_str, sizeof(fd_str), > + "fdset-id:%" PRId64 ", fd:%" PRId64, fdset_id, fd); Oops - fd is uninitialized if has_fd is false and the outer loop failed to find fdset_id. You need two separate error messages here, based on whether fd was provided. > +-> { "execute": "query-fdsets" } > +<- { "return": [ > + { > + "fds": [ > + { > + "fd": 30, > + "opaque": "rdonly:/path/to/file" > + }, > + { > + "fd": 24, > + "opaque": "rdwr:/path/to/file" > + } > + ], > + "fdset-id": 1 > + }, > + { > + "fds": [ > + { > + "fd": 28 > + }, > + { > + "fd": 29 > + } > + ], > + "fdset-id": 0 > + }, No trailing comma here.