From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:43643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sypy4-0000DL-CS for qemu-devel@nongnu.org; Tue, 07 Aug 2012 16:01:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sypy2-0002j8-5L for qemu-devel@nongnu.org; Tue, 07 Aug 2012 16:01:28 -0400 Received: from e33.co.us.ibm.com ([32.97.110.151]:49845) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sypy1-0002g8-T3 for qemu-devel@nongnu.org; Tue, 07 Aug 2012 16:01:26 -0400 Received: from /spool/local by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 7 Aug 2012 14:01:19 -0600 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 06EB119D803F for ; Tue, 7 Aug 2012 19:59:59 +0000 (WET) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q77Jxox9047270 for ; Tue, 7 Aug 2012 13:59:50 -0600 Received: from d03av03.boulder.ibm.com (loopback [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q77Jxk7L007891 for ; Tue, 7 Aug 2012 13:59:46 -0600 Message-ID: <502173AF.9060909@linux.vnet.ibm.com> Date: Tue, 07 Aug 2012 15:59:43 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1344014889-12390-1-git-send-email-coreyb@linux.vnet.ibm.com> <1344014889-12390-3-git-send-email-coreyb@linux.vnet.ibm.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v6 2/6] qapi: Introduce add-fd, remove-fd, query-fdsets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: kwolf@redhat.com, aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, eblake@redhat.com On 08/07/2012 02:16 PM, Stefan Hajnoczi wrote: > On Fri, Aug 3, 2012 at 6:28 PM, Corey Bryant 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