| Message ID | 20180524043952.11245-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: let Monitor be thread safe | expand |
Peter Xu <peterx@redhat.com> 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 <peterx@redhat.com> > --- > 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; > }
On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote: > int monitor_fdset_get_fd(int64_t fdset_id, int flags) > { > -#ifndef _WIN32 > +#ifdef _WIN32 > + return -ENOENT; stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1 now.
On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote: > On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote: > > int monitor_fdset_get_fd(int64_t fdset_id, int flags) > > { > > -#ifndef _WIN32 > > +#ifdef _WIN32 > > + return -ENOENT; > > stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1 > now. Yes that's intended. That's actually a suggestion from Markus since changing the return code will simplify the code. I mentioned it in the commit message too: """ 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. """ Thanks,
On Fri, May 25, 2018 at 11:30:22AM +0800, Peter Xu wrote: > On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote: > > On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote: > > > int monitor_fdset_get_fd(int64_t fdset_id, int flags) > > > { > > > -#ifndef _WIN32 > > > +#ifdef _WIN32 > > > + return -ENOENT; > > > > stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1 > > now. > > Yes that's intended. That's actually a suggestion from Markus since > changing the return code will simplify the code. No, I understand that part. I'm pointing out that stubs/fdset.c:monitor_fdset_get_fd() still returns -1 because it was not updated by this patch. Since this patch changes the return value to -errno, the stub function should be updated to return a sensible -errno value too. Stefan
On Fri, May 25, 2018 at 10:01:57AM +0100, Stefan Hajnoczi wrote: > On Fri, May 25, 2018 at 11:30:22AM +0800, Peter Xu wrote: > > On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote: > > > On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote: > > > > int monitor_fdset_get_fd(int64_t fdset_id, int flags) > > > > { > > > > -#ifndef _WIN32 > > > > +#ifdef _WIN32 > > > > + return -ENOENT; > > > > > > stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1 > > > now. > > > > Yes that's intended. That's actually a suggestion from Markus since > > changing the return code will simplify the code. > > No, I understand that part. I'm pointing out that > stubs/fdset.c:monitor_fdset_get_fd() still returns -1 because it was not > updated by this patch. > > Since this patch changes the return value to -errno, the stub function > should be updated to return a sensible -errno value too. Sorry I overlooked. You are right, I'll touch that up. Regards,
On Thu, May 24, 2018 at 11:03:55AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> 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? No... I'll remove that sentence directly: "Introduce a new global big lock for mon_fdsets. Take it where needed." > > > > 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/. Okay. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > 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. I'll append another patch to do that, and move these fields together. > > > 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. Will do. > > > @@ -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? Yes, will s/param/CLI/. > > > + * 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? Please see the comment for monitor_init_globals(): /* * Note: qtest_enabled() (which is used in monitor_qapi_event_init()) * depends on configure_accelerator() above. */ monitor_init_globals(); So I guess it won't work to directly move it earlier. The init dependency of QEMU is really complicated. I'll be fine now that we mark totally independent init functions (like this one, which is a mutex init only) as constructor, then we can save some time on ordering issue. > > > +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. I'll take your advice. > > > +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. The variable "ret" brought some trouble, so I decided to remove it directly. :) @@ -2538,20 +2569,25 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd) MonFdset *mon_fdset; MonFdsetFd *mon_fdset_fd_dup; + 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; + goto err; } } 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); + qemu_mutex_unlock(&mon_fdsets_lock); return 0; } + +err: + qemu_mutex_unlock(&mon_fdsets_lock); return -1; } > > > - 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. I'll do similar thing to drop "ret": @@ -2560,6 +2596,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove) MonFdset *mon_fdset; MonFdsetFd *mon_fdset_fd_dup; + 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) { @@ -2568,13 +2605,17 @@ 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; + goto err; } else { + qemu_mutex_unlock(&mon_fdsets_lock); return mon_fdset->id; } } } } + +err: + qemu_mutex_unlock(&mon_fdsets_lock); return -1; } Thanks,
Peter Xu <peterx@redhat.com> writes: > On Thu, May 24, 2018 at 11:03:55AM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> 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? > > No... I'll remove that sentence directly: > > "Introduce a new global big lock for mon_fdsets. Take it where needed." Works for me. >> > 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/. > > Okay. > >> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > 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. > > I'll append another patch to do that, and move these fields together. > >> >> > 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. > > Will do. > >> >> > @@ -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? > > Yes, will s/param/CLI/. > >> >> > + * 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? > > Please see the comment for monitor_init_globals(): > > /* > * Note: qtest_enabled() (which is used in monitor_qapi_event_init()) > * depends on configure_accelerator() above. > */ > monitor_init_globals(); > > So I guess it won't work to directly move it earlier. The init > dependency of QEMU is really complicated. I'll be fine now that we > mark totally independent init functions (like this one, which is a > mutex init only) as constructor, then we can save some time on > ordering issue. Let me rephrase. There's a preexisting issue: main() calls monitor.c functions before calling its initialization function monitor_init_globals(). This needs to be cleaned up. Would you be willing to do it? Possible solutions: * Perhaps we can move monitor_init_globals() up and/or the code calling into monitor.c early down sufficiently. * Calculate event_clock_type on each use instead of ahead of time. It's qtest_enabled ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME, and neither of its users needs to be fast. Then move monitor_init_globals before the code calling into monitor.c. I'm not opposed to use of constructors for self-contained initialization (no calls to other modules). But I don't like initialization spread over multiple functions. >> > +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. > > I'll take your advice. > >> >> > +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. > > The variable "ret" brought some trouble, so I decided to remove it > directly. :) > > @@ -2538,20 +2569,25 @@ int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd) > MonFdset *mon_fdset; > MonFdsetFd *mon_fdset_fd_dup; > > + 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; > + goto err; > } > } > 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); > + qemu_mutex_unlock(&mon_fdsets_lock); > return 0; > } > + > +err: > + qemu_mutex_unlock(&mon_fdsets_lock); > return -1; > } Works for me. >> >> > - 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. > > I'll do similar thing to drop "ret": > > @@ -2560,6 +2596,7 @@ static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove) > MonFdset *mon_fdset; > MonFdsetFd *mon_fdset_fd_dup; > > + 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) { > @@ -2568,13 +2605,17 @@ 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; > + goto err; > } else { > + qemu_mutex_unlock(&mon_fdsets_lock); > return mon_fdset->id; > } > } > } > } > + > +err: > + qemu_mutex_unlock(&mon_fdsets_lock); > return -1; > } Also works for me.
On Mon, May 28, 2018 at 05:19:08PM +0200, Markus Armbruster wrote: [...] > >> > >> > + * 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? > > > > Please see the comment for monitor_init_globals(): > > > > /* > > * Note: qtest_enabled() (which is used in monitor_qapi_event_init()) > > * depends on configure_accelerator() above. > > */ > > monitor_init_globals(); > > > > So I guess it won't work to directly move it earlier. The init > > dependency of QEMU is really complicated. I'll be fine now that we > > mark totally independent init functions (like this one, which is a > > mutex init only) as constructor, then we can save some time on > > ordering issue. > > Let me rephrase. There's a preexisting issue: main() calls monitor.c > functions before calling its initialization function > monitor_init_globals(). This needs to be cleaned up. Would you be > willing to do it? > > Possible solutions: > > * Perhaps we can move monitor_init_globals() up and/or the code calling > into monitor.c early down sufficiently. > > * Calculate event_clock_type on each use instead of ahead of time. It's > qtest_enabled ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME, and neither > of its users needs to be fast. Then move monitor_init_globals before > the code calling into monitor.c. Indeed. Obviously you thought a step further. :) > > I'm not opposed to use of constructors for self-contained initialization > (no calls to other modules). But I don't like initialization spread > over multiple functions. Since this work will actually decide where I should init this new fdset lock, so I'll try to do that altogether within the series. Thanks for your suggestions! It makes sense.
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. */ 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; @@ -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. + * Meanwhile it can also be used even at the end of main. Let's keep + * it initialized for the whole lifecycle of QEMU. + */ +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; +#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; } +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; + 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; } - 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; } 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; }
Similar to previous patch, but introduce a new global big lock for mon_fdsets. Take it where needed. 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. Signed-off-by: Peter Xu <peterx@redhat.com> --- monitor.c | 70 +++++++++++++++++++++++++++++++++++++++++----------- util/osdep.c | 3 ++- 2 files changed, 58 insertions(+), 15 deletions(-)