From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48693) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fLmAE-0002ov-SW for qemu-devel@nongnu.org; Thu, 24 May 2018 05:04:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fLmA9-00052V-Sf for qemu-devel@nongnu.org; Thu, 24 May 2018 05:04:02 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:40594 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fLmA9-00051z-Nk for qemu-devel@nongnu.org; Thu, 24 May 2018 05:03:57 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 05E07402679B for ; Thu, 24 May 2018 09:03:57 +0000 (UTC) From: Markus Armbruster References: <20180524043952.11245-1-peterx@redhat.com> <20180524043952.11245-5-peterx@redhat.com> Date: Thu, 24 May 2018 11:03:55 +0200 In-Reply-To: <20180524043952.11245-5-peterx@redhat.com> (Peter Xu's message of "Thu, 24 May 2018 12:39:52 +0800") Message-ID: <87sh6h77d0.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 4/4] monitor: add lock to protect mon_fdsets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: qemu-devel@nongnu.org, "Dr . David Alan Gilbert" , Stefan Hajnoczi , =?utf-8?Q?Marc-Andr=C3=A9?= Lureau Peter Xu writes: > Similar to previous patch, but introduce a new global big lock for > mon_fdsets. Take it where needed. The previous patch is "monitor: more comments on lock-free fleids/funcs". Sure you mean that one? > > The monitor_fdset_get_fd() handling is a bit tricky: now we need to call > qemu_mutex_unlock() which might pollute errno, so we need to make sure > the correct errno be passed up to the callers. To make things simpler, > we let monitor_fdset_get_fd() return the -errno directly when error > happens, then in qemu_open() we translate it back into errno. Suggest s/translate/move/. > > Signed-off-by: Peter Xu > --- > monitor.c | 70 +++++++++++++++++++++++++++++++++++++++++----------- > util/osdep.c | 3 ++- > 2 files changed, 58 insertions(+), 15 deletions(-) > > diff --git a/monitor.c b/monitor.c > index f01589f945..6266ff65c4 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -271,6 +271,9 @@ typedef struct QMPRequest QMPRequest; > /* Protects mon_list, monitor_event_state. */ Not this patch's problem: there is no monitor_event_state. Screwed up in commit d622cb5879c. I guess it's monitor_qapi_event_state. > static QemuMutex monitor_lock; > > +/* Protects mon_fdsets */ > +static QemuMutex mon_fdsets_lock; > + > static QTAILQ_HEAD(mon_list, Monitor) mon_list; > static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets; > static int mon_refcount; Suggest to move mon_fdsets next to the lock protecting it. > @@ -287,6 +290,16 @@ static QEMUClockType event_clock_type = QEMU_CLOCK_REALTIME; > static void monitor_command_cb(void *opaque, const char *cmdline, > void *readline_opaque); > > +/* > + * This lock can be used very early, even during param parsing. Do you mean CLI parsing? > + * Meanwhile it can also be used even at the end of main. Let's keep > + * it initialized for the whole lifecycle of QEMU. > + */ Awkward question, since our main() is such a tangled mess, but here goes anyway... The existing place to initialize monitor.c's globals is monitor_init_globals(). But that one runs too late, I guess: parse_add_fd() runs earlier, and calls monitor_fdset_add_fd(). Unclean even without this lock; no module should be used before its initialization function runs. Sure we can't run monitor_init_globals() sufficiently early? > +static void __attribute__((constructor)) mon_fdsets_lock_init(void) > +{ > + qemu_mutex_init(&mon_fdsets_lock); > +} > + > /** > * Is @mon a QMP monitor? > */ > @@ -2316,9 +2329,11 @@ static void monitor_fdsets_cleanup(void) > MonFdset *mon_fdset; > MonFdset *mon_fdset_next; > > + qemu_mutex_lock(&mon_fdsets_lock); > QLIST_FOREACH_SAFE(mon_fdset, &mon_fdsets, next, mon_fdset_next) { > monitor_fdset_cleanup(mon_fdset); > } > + qemu_mutex_unlock(&mon_fdsets_lock); > } > > AddfdInfo *qmp_add_fd(bool has_fdset_id, int64_t fdset_id, bool has_opaque, > @@ -2353,6 +2368,7 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp) > MonFdsetFd *mon_fdset_fd; > char fd_str[60]; > > + qemu_mutex_lock(&mon_fdsets_lock); > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > if (mon_fdset->id != fdset_id) { > continue; > @@ -2372,10 +2388,12 @@ void qmp_remove_fd(int64_t fdset_id, bool has_fd, int64_t fd, Error **errp) > goto error; > } > monitor_fdset_cleanup(mon_fdset); > + qemu_mutex_unlock(&mon_fdsets_lock); > return; > } > > error: > + qemu_mutex_unlock(&mon_fdsets_lock); > if (has_fd) { > snprintf(fd_str, sizeof(fd_str), "fdset-id:%" PRId64 ", fd:%" PRId64, > fdset_id, fd); > @@ -2391,6 +2409,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp) > MonFdsetFd *mon_fdset_fd; > FdsetInfoList *fdset_list = NULL; > > + qemu_mutex_lock(&mon_fdsets_lock); > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info)); > FdsetFdInfoList *fdsetfd_list = NULL; > @@ -2420,6 +2439,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp) > fdset_info->next = fdset_list; > fdset_list = fdset_info; > } > + qemu_mutex_unlock(&mon_fdsets_lock); > > return fdset_list; > } > @@ -2432,6 +2452,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id, > MonFdsetFd *mon_fdset_fd; > AddfdInfo *fdinfo; > > + qemu_mutex_lock(&mon_fdsets_lock); > if (has_fdset_id) { > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > /* Break if match found or match impossible due to ordering by ID */ > @@ -2452,6 +2473,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id, > if (fdset_id < 0) { > error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id", > "a non-negative value"); > + qemu_mutex_unlock(&mon_fdsets_lock); > return NULL; > } > /* Use specified fdset ID */ > @@ -2502,16 +2524,21 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool has_fdset_id, int64_t fdset_id, > fdinfo->fdset_id = mon_fdset->id; > fdinfo->fd = mon_fdset_fd->fd; > > + qemu_mutex_unlock(&mon_fdsets_lock); > return fdinfo; > } > > int monitor_fdset_get_fd(int64_t fdset_id, int flags) > { > -#ifndef _WIN32 > +#ifdef _WIN32 > + return -ENOENT; ENOENT feels odd, but I guess makes some sense since there is no way to add entries. > +#else > MonFdset *mon_fdset; > MonFdsetFd *mon_fdset_fd; > int mon_fd_flags; > + int ret = -ENOENT; > > + qemu_mutex_lock(&mon_fdsets_lock); > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > if (mon_fdset->id != fdset_id) { > continue; > @@ -2519,49 +2546,60 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags) > QLIST_FOREACH(mon_fdset_fd, &mon_fdset->fds, next) { > mon_fd_flags = fcntl(mon_fdset_fd->fd, F_GETFL); > if (mon_fd_flags == -1) { > - return -1; > + ret = -errno; > + goto out; > } > > if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) { > - return mon_fdset_fd->fd; > + ret = mon_fdset_fd->fd; > + goto out; > } > } > - errno = EACCES; > - return -1; > + ret = -EACCES; > + break; > } I still think ret = -EACCES; goto out; } ret = -ENOENT; out: would be clearer. > +out: > + qemu_mutex_unlock(&mon_fdsets_lock); > + return ret; > #endif > - > - errno = ENOENT; > - return -1; > } > > int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd) > { > MonFdset *mon_fdset; > MonFdsetFd *mon_fdset_fd_dup; > + int ret = -1; > > + qemu_mutex_lock(&mon_fdsets_lock); > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > if (mon_fdset->id != fdset_id) { > continue; > } > QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) { > if (mon_fdset_fd_dup->fd == dup_fd) { > - return -1; > + ret = -1; Dead assignment. Alternatively, > + goto out; > } > } > mon_fdset_fd_dup = g_malloc0(sizeof(*mon_fdset_fd_dup)); > mon_fdset_fd_dup->fd = dup_fd; > QLIST_INSERT_HEAD(&mon_fdset->dup_fds, mon_fdset_fd_dup, next); > - return 0; > + ret = 0; > + break; > } ... add ret = -1; here, and drop the initializer. Your choice. > - return -1; > + > +out: > + qemu_mutex_unlock(&mon_fdsets_lock); > + return ret; > } > > static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove) > { > MonFdset *mon_fdset; > MonFdsetFd *mon_fdset_fd_dup; > + int ret = -1; > > + qemu_mutex_lock(&mon_fdsets_lock); > QLIST_FOREACH(mon_fdset, &mon_fdsets, next) { > QLIST_FOREACH(mon_fdset_fd_dup, &mon_fdset->dup_fds, next) { > if (mon_fdset_fd_dup->fd == dup_fd) { > @@ -2570,14 +2608,18 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove) > if (QLIST_EMPTY(&mon_fdset->dup_fds)) { > monitor_fdset_cleanup(mon_fdset); > } > - return -1; > + ret = -1; > + goto out; > } else { > - return mon_fdset->id; > + ret = mon_fdset->id; > + goto out; > } > } > } > } > - return -1; > +out: > + qemu_mutex_unlock(&mon_fdsets_lock); > + return ret; > } Likewise. > > int monitor_fdset_dup_fd_find(int dup_fd) > diff --git a/util/osdep.c b/util/osdep.c > index a73de0e1ba..ea51d500b6 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -302,7 +302,8 @@ int qemu_open(const char *name, int flags, ...) > } > > fd = monitor_fdset_get_fd(fdset_id, flags); > - if (fd == -1) { > + if (fd < 0) { > + errno = -fd; > return -1; > }