[libvirt] [Qemu-devel] [PATCH v6 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets
Corey Bryant
coreyb at linux.vnet.ibm.com
Tue Aug 7 19:59:43 UTC 2012
On 08/07/2012 02:16 PM, Stefan Hajnoczi wrote:
> On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant <coreyb at 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.
>
Thanks, I'll fix that.
>> + 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.
>
Yes, please review the v7 patches and let me know what you think. I
explained the purpose of these fields in the previous email I just sent
you, so I won't go into their details again here. But I will point out
that refcount/in-use came about after concern of fd leakage if libvirt's
monitor connection disconnects. query-fdsets allows the client to
determine the state of the fd sets after reconnecting.
>> +
>> 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?
>
Yes, it looks like it is. I'll fix that.
>> + }
>> + } 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".
Can I pass an int64_t into the message if it takes a string?
I thought int64_t was a long long in 32-bit mode, but perhaps that's not
always the case?
>
> 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.
Ok thanks, I'll take a look at qemu-devel.
--
Regards,
Corey
More information about the libvir-list
mailing list