* [PATCH] monitor: Use LOCK_GUARD macros
@ 2020-09-08 13:52 Dr. David Alan Gilbert (git)
2020-09-08 14:21 ` Markus Armbruster
0 siblings, 1 reply; 3+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2020-09-08 13:52 UTC (permalink / raw)
To: qemu-devel, armbru
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Use the lock guard macros in monitor/misc.c - saves
a bunch of goto's, and a temporary variable, but mostly
because I prefer not having to release them in error paths.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
monitor/misc.c | 62 ++++++++++++++++----------------------------------
1 file changed, 20 insertions(+), 42 deletions(-)
diff --git a/monitor/misc.c b/monitor/misc.c
index e847b58a8c..7f0b313d53 100644
--- a/monitor/misc.c
+++ b/monitor/misc.c
@@ -141,13 +141,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
handle_hmp_command(&hmp, command_line);
cur_mon = old_mon;
- qemu_mutex_lock(&hmp.common.mon_lock);
- if (qstring_get_length(hmp.common.outbuf) > 0) {
- output = g_strdup(qstring_get_str(hmp.common.outbuf));
- } else {
- output = g_strdup("");
+ WITH_QEMU_LOCK_GUARD(&hmp.common.mon_lock) {
+ if (qstring_get_length(hmp.common.outbuf) > 0) {
+ output = g_strdup(qstring_get_str(hmp.common.outbuf));
+ } else {
+ output = g_strdup("");
+ }
}
- qemu_mutex_unlock(&hmp.common.mon_lock);
out:
monitor_data_destroy(&hmp.common);
@@ -1248,7 +1248,7 @@ void qmp_getfd(const char *fdname, Error **errp)
return;
}
- qemu_mutex_lock(&cur_mon->mon_lock);
+ QEMU_LOCK_GUARD(&cur_mon->mon_lock);
QLIST_FOREACH(monfd, &cur_mon->fds, next) {
if (strcmp(monfd->name, fdname) != 0) {
continue;
@@ -1256,7 +1256,6 @@ 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;
@@ -1267,7 +1266,6 @@ 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)
@@ -1299,7 +1297,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
{
mon_fd_t *monfd;
- qemu_mutex_lock(&mon->mon_lock);
+ QEMU_LOCK_GUARD(&mon->mon_lock);
QLIST_FOREACH(monfd, &mon->fds, next) {
int fd;
@@ -1313,12 +1311,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
QLIST_REMOVE(monfd, next);
g_free(monfd->name);
g_free(monfd);
- qemu_mutex_unlock(&mon->mon_lock);
return fd;
}
- qemu_mutex_unlock(&mon->mon_lock);
error_setg(errp, "File descriptor named '%s' has not been found", fdname);
return -1;
}
@@ -1350,11 +1346,10 @@ void monitor_fdsets_cleanup(void)
MonFdset *mon_fdset;
MonFdset *mon_fdset_next;
- qemu_mutex_lock(&mon_fdsets_lock);
+ QEMU_LOCK_GUARD(&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,
@@ -1389,7 +1384,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);
+ QEMU_LOCK_GUARD(&mon_fdsets_lock);
QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
if (mon_fdset->id != fdset_id) {
continue;
@@ -1409,12 +1404,10 @@ 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);
@@ -1430,7 +1423,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
MonFdsetFd *mon_fdset_fd;
FdsetInfoList *fdset_list = NULL;
- qemu_mutex_lock(&mon_fdsets_lock);
+ QEMU_LOCK_GUARD(&mon_fdsets_lock);
QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info));
FdsetFdInfoList *fdsetfd_list = NULL;
@@ -1460,7 +1453,6 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
fdset_info->next = fdset_list;
fdset_list = fdset_info;
}
- qemu_mutex_unlock(&mon_fdsets_lock);
return fdset_list;
}
@@ -1555,9 +1547,8 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
MonFdset *mon_fdset;
MonFdsetFd *mon_fdset_fd;
int mon_fd_flags;
- int ret;
- qemu_mutex_lock(&mon_fdsets_lock);
+ QEMU_LOCK_GUARD(&mon_fdsets_lock);
QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
if (mon_fdset->id != fdset_id) {
continue;
@@ -1565,23 +1556,16 @@ 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) {
- ret = -errno;
- goto out;
+ return -errno;
}
if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
- ret = mon_fdset_fd->fd;
- goto out;
+ return mon_fdset_fd->fd;
}
}
- ret = -EACCES;
- goto out;
+ return -EACCES;
}
- ret = -ENOENT;
-
-out:
- qemu_mutex_unlock(&mon_fdsets_lock);
- return ret;
+ return -ENOENT;
#endif
}
@@ -1590,25 +1574,22 @@ 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);
+ QEMU_LOCK_GUARD(&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) {
- goto err;
+ return -1;
}
}
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;
}
@@ -1617,7 +1598,7 @@ static int64_t 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);
+ QEMU_LOCK_GUARD(&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) {
@@ -1627,17 +1608,14 @@ static int64_t monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
monitor_fdset_cleanup(mon_fdset);
}
- goto err;
+ return -1;
} else {
- qemu_mutex_unlock(&mon_fdsets_lock);
return mon_fdset->id;
}
}
}
}
-err:
- qemu_mutex_unlock(&mon_fdsets_lock);
return -1;
}
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] monitor: Use LOCK_GUARD macros
2020-09-08 13:52 [PATCH] monitor: Use LOCK_GUARD macros Dr. David Alan Gilbert (git)
@ 2020-09-08 14:21 ` Markus Armbruster
2020-09-08 14:43 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 3+ messages in thread
From: Markus Armbruster @ 2020-09-08 14:21 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Use the lock guard macros in monitor/misc.c - saves
> a bunch of goto's, and a temporary variable, but mostly
> because I prefer not having to release them in error paths.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> monitor/misc.c | 62 ++++++++++++++++----------------------------------
> 1 file changed, 20 insertions(+), 42 deletions(-)
>
> diff --git a/monitor/misc.c b/monitor/misc.c
> index e847b58a8c..7f0b313d53 100644
> --- a/monitor/misc.c
> +++ b/monitor/misc.c
> @@ -141,13 +141,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> handle_hmp_command(&hmp, command_line);
> cur_mon = old_mon;
>
> - qemu_mutex_lock(&hmp.common.mon_lock);
> - if (qstring_get_length(hmp.common.outbuf) > 0) {
> - output = g_strdup(qstring_get_str(hmp.common.outbuf));
> - } else {
> - output = g_strdup("");
> + WITH_QEMU_LOCK_GUARD(&hmp.common.mon_lock) {
> + if (qstring_get_length(hmp.common.outbuf) > 0) {
> + output = g_strdup(qstring_get_str(hmp.common.outbuf));
> + } else {
> + output = g_strdup("");
> + }
> }
> - qemu_mutex_unlock(&hmp.common.mon_lock);
>
> out:
> monitor_data_destroy(&hmp.common);
Hardly an improvement here. I figure you want it for the sake of
consistency.
> @@ -1248,7 +1248,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> return;
> }
>
> - qemu_mutex_lock(&cur_mon->mon_lock);
> + QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> if (strcmp(monfd->name, fdname) != 0) {
> continue;
> @@ -1256,7 +1256,6 @@ 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;
> @@ -1267,7 +1266,6 @@ 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);
> }
>
This one makes more sense.
[More of the same...]
Reviewed-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] monitor: Use LOCK_GUARD macros
2020-09-08 14:21 ` Markus Armbruster
@ 2020-09-08 14:43 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 3+ messages in thread
From: Dr. David Alan Gilbert @ 2020-09-08 14:43 UTC (permalink / raw)
To: Markus Armbruster; +Cc: qemu-devel
* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Use the lock guard macros in monitor/misc.c - saves
> > a bunch of goto's, and a temporary variable, but mostly
> > because I prefer not having to release them in error paths.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > monitor/misc.c | 62 ++++++++++++++++----------------------------------
> > 1 file changed, 20 insertions(+), 42 deletions(-)
> >
> > diff --git a/monitor/misc.c b/monitor/misc.c
> > index e847b58a8c..7f0b313d53 100644
> > --- a/monitor/misc.c
> > +++ b/monitor/misc.c
> > @@ -141,13 +141,13 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
> > handle_hmp_command(&hmp, command_line);
> > cur_mon = old_mon;
> >
> > - qemu_mutex_lock(&hmp.common.mon_lock);
> > - if (qstring_get_length(hmp.common.outbuf) > 0) {
> > - output = g_strdup(qstring_get_str(hmp.common.outbuf));
> > - } else {
> > - output = g_strdup("");
> > + WITH_QEMU_LOCK_GUARD(&hmp.common.mon_lock) {
> > + if (qstring_get_length(hmp.common.outbuf) > 0) {
> > + output = g_strdup(qstring_get_str(hmp.common.outbuf));
> > + } else {
> > + output = g_strdup("");
> > + }
> > }
> > - qemu_mutex_unlock(&hmp.common.mon_lock);
> >
> > out:
> > monitor_data_destroy(&hmp.common);
>
>
> Hardly an improvement here. I figure you want it for the sake of
> consistency.
Right, I was trying to do all the ones that weren't
worse; there's one case I left where it explicitly needed to do
something after the unlock.
> > @@ -1248,7 +1248,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> > return;
> > }
> >
> > - qemu_mutex_lock(&cur_mon->mon_lock);
> > + QEMU_LOCK_GUARD(&cur_mon->mon_lock);
> > QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> > if (strcmp(monfd->name, fdname) != 0) {
> > continue;
> > @@ -1256,7 +1256,6 @@ 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;
> > @@ -1267,7 +1266,6 @@ 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);
> > }
> >
>
> This one makes more sense.
>
> [More of the same...]
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
Thanks.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-08 14:44 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 13:52 [PATCH] monitor: Use LOCK_GUARD macros Dr. David Alan Gilbert (git)
2020-09-08 14:21 ` Markus Armbruster
2020-09-08 14:43 ` Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).