| Message ID | 20230207142535.1153722-7-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Teach 'getfd' QMP command to import win32 sockets | expand |
On 7/2/23 15:25, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > As per comment, presumably to avoid syscall in critical section. > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > monitor/fds.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/monitor/fds.c b/monitor/fds.c > index 26b39a0ce6..03c5e97c35 100644 > --- a/monitor/fds.c > +++ b/monitor/fds.c > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) > return; > } > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); > + qemu_mutex_lock(&cur_mon->mon_lock); If you respin, please add /* See close() call below. */ comment. > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > if (strcmp(monfd->name, fdname) != 0) { > continue; > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > tmp_fd = monfd->fd; > monfd->fd = fd; > + qemu_mutex_unlock(&cur_mon->mon_lock); > /* Make sure close() is outside critical section */ > close(tmp_fd); > return; > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) > monfd->fd = fd; > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); > + qemu_mutex_unlock(&cur_mon->mon_lock); > } Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > As per comment, presumably to avoid syscall in critical section. > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > monitor/fds.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/monitor/fds.c b/monitor/fds.c > index 26b39a0ce6..03c5e97c35 100644 > --- a/monitor/fds.c > +++ b/monitor/fds.c > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) > return; > } > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); > + qemu_mutex_lock(&cur_mon->mon_lock); > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > if (strcmp(monfd->name, fdname) != 0) { > continue; > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > tmp_fd = monfd->fd; > monfd->fd = fd; > + qemu_mutex_unlock(&cur_mon->mon_lock); > /* Make sure close() is outside critical section */ > close(tmp_fd); > return; > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) > monfd->fd = fd; > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); > + qemu_mutex_unlock(&cur_mon->mon_lock); > } > > void qmp_closefd(const char *fdname, Error **errp) This confused me. I think I understand now, but let's double-check. You're reverting commit 0210c3b39bef08 for qmp_getfd() because it extended the criticial section beyond the close(), invalidating the comment. Correct? Did it actually break anything?
Hi On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote: > > marcandre.lureau@redhat.com writes: > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > As per comment, presumably to avoid syscall in critical section. > > > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > monitor/fds.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/monitor/fds.c b/monitor/fds.c > > index 26b39a0ce6..03c5e97c35 100644 > > --- a/monitor/fds.c > > +++ b/monitor/fds.c > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > return; > > } > > > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); > > + qemu_mutex_lock(&cur_mon->mon_lock); > > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > > if (strcmp(monfd->name, fdname) != 0) { > > continue; > > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > > tmp_fd = monfd->fd; > > monfd->fd = fd; > > + qemu_mutex_unlock(&cur_mon->mon_lock); > > /* Make sure close() is outside critical section */ > > close(tmp_fd); > > return; > > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > monfd->fd = fd; > > > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); > > + qemu_mutex_unlock(&cur_mon->mon_lock); > > } > > > > void qmp_closefd(const char *fdname, Error **errp) > > This confused me. I think I understand now, but let's double-check. > > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it > extended the criticial section beyond the close(), invalidating the > comment. Correct? Correct > > Did it actually break anything? Not that I know of (David admitted over IRC that this was not intended)
On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote: > Hi > > On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote: > > > > marcandre.lureau@redhat.com writes: > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > As per comment, presumably to avoid syscall in critical section. > > > > > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > --- > > > monitor/fds.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/monitor/fds.c b/monitor/fds.c > > > index 26b39a0ce6..03c5e97c35 100644 > > > --- a/monitor/fds.c > > > +++ b/monitor/fds.c > > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > return; > > > } > > > > > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); > > > + qemu_mutex_lock(&cur_mon->mon_lock); > > > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > > > if (strcmp(monfd->name, fdname) != 0) { > > > continue; > > > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > > > > tmp_fd = monfd->fd; > > > monfd->fd = fd; > > > + qemu_mutex_unlock(&cur_mon->mon_lock); > > > /* Make sure close() is outside critical section */ > > > close(tmp_fd); > > > return; > > > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > monfd->fd = fd; > > > > > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); > > > + qemu_mutex_unlock(&cur_mon->mon_lock); > > > } > > > > > > void qmp_closefd(const char *fdname, Error **errp) > > > > This confused me. I think I understand now, but let's double-check. > > > > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it > > extended the criticial section beyond the close(), invalidating the > > comment. Correct? > > Correct > > > Did it actually break anything? > > Not that I know of (David admitted over IRC that this was not intended) Conceptually the only risk here is that 'close()' blocks for a prolonged period of time, which prevents another thread from acquiring the mutex. First, the chances of close() blocking are incredibly low for socket FDs which have not yet been used to transmit data. It would require a malicious mgmt app to pass an unexpected FD type that could block but that's quite hard, and we consider the QMP client be a trusted entity anyway. As for another thread blocking on the mutex I'm not convinced that'll happen either. The FD set is scoped to the current monitor. Almost certainly the FD is going to be consumed by a later QMP device-add/object-add command, in the same thread. Processing of that later QMP command will be delayed regardless of whether the close is inside or outside the critical section. AFAICT keeping close() oujtside the critical section serves no purpose and we could just stick with the lock guard and delete the comment. With regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote: >> Hi >> >> On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote: >> > >> > marcandre.lureau@redhat.com writes: >> > >> > > From: Marc-André Lureau <marcandre.lureau@redhat.com> >> > > >> > > As per comment, presumably to avoid syscall in critical section. >> > > >> > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") >> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> > > --- >> > > monitor/fds.c | 4 +++- >> > > 1 file changed, 3 insertions(+), 1 deletion(-) >> > > >> > > diff --git a/monitor/fds.c b/monitor/fds.c >> > > index 26b39a0ce6..03c5e97c35 100644 >> > > --- a/monitor/fds.c >> > > +++ b/monitor/fds.c >> > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) >> > > return; >> > > } >> > > >> > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); >> > > + qemu_mutex_lock(&cur_mon->mon_lock); >> > > QLIST_FOREACH(monfd, &cur_mon->fds, next) { >> > > if (strcmp(monfd->name, fdname) != 0) { >> > > continue; >> > > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) >> > > >> > > tmp_fd = monfd->fd; >> > > monfd->fd = fd; >> > > + qemu_mutex_unlock(&cur_mon->mon_lock); >> > > /* Make sure close() is outside critical section */ >> > > close(tmp_fd); >> > > return; >> > > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) >> > > monfd->fd = fd; >> > > >> > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); >> > > + qemu_mutex_unlock(&cur_mon->mon_lock); >> > > } >> > > >> > > void qmp_closefd(const char *fdname, Error **errp) >> > >> > This confused me. I think I understand now, but let's double-check. >> > >> > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it >> > extended the criticial section beyond the close(), invalidating the >> > comment. Correct? >> >> Correct >> >> > Did it actually break anything? >> >> Not that I know of (David admitted over IRC that this was not intended) > > Conceptually the only risk here is that 'close()' blocks for a > prolonged period of time, which prevents another thread from > acquiring the mutex. > > First, the chances of close() blocking are incredibly low for > socket FDs which have not yet been used to transmit data. It > would require a malicious mgmt app to pass an unexpected FD > type that could block but that's quite hard, and we consider > the QMP client be a trusted entity anyway. > > As for another thread blocking on the mutex I'm not convinced > that'll happen either. The FD set is scoped to the current > monitor. Almost certainly the FD is going to be consumed by > a later QMP device-add/object-add command, in the same thread. > Processing of that later QMP command will be delayed regardless > of whether the close is inside or outside the critical section. > > AFAICT keeping close() oujtside the critical section serves > no purpose and we could just stick with the lock guard and > delete the comment. Makes sense to me. There's another one in monitor_add_fd(). Both are from Peter's commit 9409fc05fe2 "monitor: protect mon->fds with mon_lock". Peter, do you remember why you took the trouble to keep close() outside the critical section? I know it's been a while...
On Tue, Feb 14, 2023 at 05:23:08PM +0100, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote: > >> Hi > >> > >> On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > > >> > marcandre.lureau@redhat.com writes: > >> > > >> > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > > > >> > > As per comment, presumably to avoid syscall in critical section. > >> > > > >> > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > >> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >> > > --- > >> > > monitor/fds.c | 4 +++- > >> > > 1 file changed, 3 insertions(+), 1 deletion(-) > >> > > > >> > > diff --git a/monitor/fds.c b/monitor/fds.c > >> > > index 26b39a0ce6..03c5e97c35 100644 > >> > > --- a/monitor/fds.c > >> > > +++ b/monitor/fds.c > >> > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) > >> > > return; > >> > > } > >> > > > >> > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); > >> > > + qemu_mutex_lock(&cur_mon->mon_lock); > >> > > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > >> > > if (strcmp(monfd->name, fdname) != 0) { > >> > > continue; > >> > > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) > >> > > > >> > > tmp_fd = monfd->fd; > >> > > monfd->fd = fd; > >> > > + qemu_mutex_unlock(&cur_mon->mon_lock); > >> > > /* Make sure close() is outside critical section */ > >> > > close(tmp_fd); > >> > > return; > >> > > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) > >> > > monfd->fd = fd; > >> > > > >> > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); > >> > > + qemu_mutex_unlock(&cur_mon->mon_lock); > >> > > } > >> > > > >> > > void qmp_closefd(const char *fdname, Error **errp) > >> > > >> > This confused me. I think I understand now, but let's double-check. > >> > > >> > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it > >> > extended the criticial section beyond the close(), invalidating the > >> > comment. Correct? > >> > >> Correct > >> > >> > Did it actually break anything? > >> > >> Not that I know of (David admitted over IRC that this was not intended) > > > > Conceptually the only risk here is that 'close()' blocks for a > > prolonged period of time, which prevents another thread from > > acquiring the mutex. > > > > First, the chances of close() blocking are incredibly low for > > socket FDs which have not yet been used to transmit data. It > > would require a malicious mgmt app to pass an unexpected FD > > type that could block but that's quite hard, and we consider > > the QMP client be a trusted entity anyway. > > > > As for another thread blocking on the mutex I'm not convinced > > that'll happen either. The FD set is scoped to the current > > monitor. Almost certainly the FD is going to be consumed by > > a later QMP device-add/object-add command, in the same thread. > > Processing of that later QMP command will be delayed regardless > > of whether the close is inside or outside the critical section. > > > > AFAICT keeping close() oujtside the critical section serves > > no purpose and we could just stick with the lock guard and > > delete the comment. > > Makes sense to me. > > There's another one in monitor_add_fd(). > > Both are from Peter's commit 9409fc05fe2 "monitor: protect mon->fds with > mon_lock". Peter, do you remember why you took the trouble to keep > close() outside the critical section? I know it's been a while... IIRC the whole purpose of keeping close() out of the mutex section was to make sure the mutex won't take for too long in any possible way since the mutex will be held too in the monitor iothread (which will service the out-of-band commands), at that time we figured the close() has a chance of getting blocked (even if unlikely!). So to me it still makes sense to keep the close() out of the mutex section, unless the monitor code changed in the past few years on that, and sorry in advance if I didn't really follow what's happening.. What's the major beneift if we move it into the critical section? We can use the lock guard, but IMHO that's for making programming convenient only, we should not pay for it if there's an unwanted functional difference. In this case of close() I think it introduces back the possiblity of having a very slow close() - I'd bet it happen only if there's a remote socket connection to the QMP server and with unreliable network, but I really can't really tell. I think I used to discuss this with Dave. I'm wondering whether I should have used a userspace spinlock, that sounds even more proper for this case, but that's slightly off topic. It's just that if the original goal of "trying our best to make sure out-of-band monitor channels is always responsive" doesn't change, hence IMHO the comment on the lock should still be valid to me. Thanks,
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Tue, Feb 14, 2023 at 05:36:32PM +0400, Marc-André Lureau wrote: > > Hi > > > > On Tue, Feb 14, 2023 at 5:34 PM Markus Armbruster <armbru@redhat.com> wrote: > > > > > > marcandre.lureau@redhat.com writes: > > > > > > > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > > > > > As per comment, presumably to avoid syscall in critical section. > > > > > > > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > > > --- > > > > monitor/fds.c | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/monitor/fds.c b/monitor/fds.c > > > > index 26b39a0ce6..03c5e97c35 100644 > > > > --- a/monitor/fds.c > > > > +++ b/monitor/fds.c > > > > @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > > return; > > > > } > > > > > > > > - QEMU_LOCK_GUARD(&cur_mon->mon_lock); > > > > + qemu_mutex_lock(&cur_mon->mon_lock); > > > > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > > > > if (strcmp(monfd->name, fdname) != 0) { > > > > continue; > > > > @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > > > > > > tmp_fd = monfd->fd; > > > > monfd->fd = fd; > > > > + qemu_mutex_unlock(&cur_mon->mon_lock); > > > > /* Make sure close() is outside critical section */ > > > > close(tmp_fd); > > > > return; > > > > @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) > > > > monfd->fd = fd; > > > > > > > > QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); > > > > + qemu_mutex_unlock(&cur_mon->mon_lock); > > > > } > > > > > > > > void qmp_closefd(const char *fdname, Error **errp) > > > > > > This confused me. I think I understand now, but let's double-check. > > > > > > You're reverting commit 0210c3b39bef08 for qmp_getfd() because it > > > extended the criticial section beyond the close(), invalidating the > > > comment. Correct? > > > > Correct > > > > > Did it actually break anything? > > > > Not that I know of (David admitted over IRC that this was not intended) > > Conceptually the only risk here is that 'close()' blocks for a > prolonged period of time, which prevents another thread from > acquiring the mutex. > > First, the chances of close() blocking are incredibly low for > socket FDs which have not yet been used to transmit data. It > would require a malicious mgmt app to pass an unexpected FD > type that could block but that's quite hard, and we consider > the QMP client be a trusted entity anyway. I agree it's unlikely; I'm not sure it actually requires something malicious though; e.g. a managmeent app that is itself blocked, a socket connection connection over a dead network etc are the ones we're worrying about - stuff that's not so much slow as either deadlocked or taking minutes for recovery/timeout. Dave > As for another thread blocking on the mutex I'm not convinced > that'll happen either. The FD set is scoped to the current > monitor. Almost certainly the FD is going to be consumed by > a later QMP device-add/object-add command, in the same thread. > Processing of that later QMP command will be delayed regardless > of whether the close is inside or outside the critical section. > > AFAICT keeping close() oujtside the critical section serves > no purpose and we could just stick with the lock guard and > delete the comment. > > With regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
marcandre.lureau@redhat.com writes: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > As per comment, presumably to avoid syscall in critical section. > > Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> I know this is already merged but as an academic exercise we could have kept the lock guard with a little restructuring like this: void qmp_closefd(const char *fdname, Error **errp) { Monitor *cur_mon = monitor_cur(); mon_fd_t *monfd; int tmp_fd = -1; WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) { QLIST_FOREACH(monfd, &cur_mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; } QLIST_REMOVE(monfd, next); tmp_fd = monfd->fd; g_free(monfd->name); g_free(monfd); break; } } if (tmp_fd > 0) { /* close() must be outside critical section */ close(tmp_fd); } else { error_setg(errp, "File descriptor named '%s' not found", fdname); } } To my mind it makes it easier to reason about locking but I probably have an irrational aversion to multiple exit paths for locks.
Alex Bennée <alex.bennee@linaro.org> writes: > marcandre.lureau@redhat.com writes: > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> As per comment, presumably to avoid syscall in critical section. >> >> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > I know this is already merged but as an academic exercise we could have > kept the lock guard with a little restructuring like this: > > void qmp_closefd(const char *fdname, Error **errp) > { > Monitor *cur_mon = monitor_cur(); > mon_fd_t *monfd; > int tmp_fd = -1; > > WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) { > QLIST_FOREACH(monfd, &cur_mon->fds, next) { No need for QLIST_FOREACH_SAFE(), because .... > if (strcmp(monfd->name, fdname) != 0) { > continue; > } > > QLIST_REMOVE(monfd, next); > tmp_fd = monfd->fd; > g_free(monfd->name); > g_free(monfd); > break; ... we break the loop right after modifying the list. Correct? > } > } > > if (tmp_fd > 0) { You mean >= > /* close() must be outside critical section */ > close(tmp_fd); > } else { > error_setg(errp, "File descriptor named '%s' not found", fdname); This error is new. See also my reply to v4 of this patch. > } If we don't need the new error, we could simply close(tmp_fd); unconditionally. > } > > To my mind it makes it easier to reason about locking but I probably > have an irrational aversion to multiple exit paths for locks. My (possibly irrational) aversion is to if (good) { do some more } else { handle error } I prefer if (!good) { handle error bail out } do some more
Alex Bennée <alex.bennee@linaro.org> writes: > marcandre.lureau@redhat.com writes: > >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> As per comment, presumably to avoid syscall in critical section. >> >> Fixes: 0210c3b39bef08 ("monitor: Use LOCK_GUARD macros") >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > I know this is already merged but as an academic exercise we could have > kept the lock guard with a little restructuring like this: > > void qmp_closefd(const char *fdname, Error **errp) Marc-André is patching qmp_getfd(), not qmp_closefd(). > { > Monitor *cur_mon = monitor_cur(); > mon_fd_t *monfd; > int tmp_fd = -1; > > WITH_QEMU_LOCK_GUARD(&cur_mon->mon_lock) { > QLIST_FOREACH(monfd, &cur_mon->fds, next) { > if (strcmp(monfd->name, fdname) != 0) { > continue; > } > > QLIST_REMOVE(monfd, next); > tmp_fd = monfd->fd; > g_free(monfd->name); > g_free(monfd); > break; > } > } > > if (tmp_fd > 0) { > /* close() must be outside critical section */ > close(tmp_fd); > } else { > error_setg(errp, "File descriptor named '%s' not found", fdname); > } > } > > To my mind it makes it easier to reason about locking but I probably > have an irrational aversion to multiple exit paths for locks.
diff --git a/monitor/fds.c b/monitor/fds.c index 26b39a0ce6..03c5e97c35 100644 --- a/monitor/fds.c +++ b/monitor/fds.c @@ -80,7 +80,7 @@ void qmp_getfd(const char *fdname, Error **errp) return; } - QEMU_LOCK_GUARD(&cur_mon->mon_lock); + qemu_mutex_lock(&cur_mon->mon_lock); QLIST_FOREACH(monfd, &cur_mon->fds, next) { if (strcmp(monfd->name, fdname) != 0) { continue; @@ -88,6 +88,7 @@ void qmp_getfd(const char *fdname, Error **errp) tmp_fd = monfd->fd; monfd->fd = fd; + qemu_mutex_unlock(&cur_mon->mon_lock); /* Make sure close() is outside critical section */ close(tmp_fd); return; @@ -98,6 +99,7 @@ void qmp_getfd(const char *fdname, Error **errp) monfd->fd = fd; QLIST_INSERT_HEAD(&cur_mon->fds, monfd, next); + qemu_mutex_unlock(&cur_mon->mon_lock); } void qmp_closefd(const char *fdname, Error **errp)