Went through all the montior.h APIs to make sure existing Monitor
related APIs will always take the new monitor lock when necessary.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
monitor.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/monitor.c b/monitor.c
index c93aa4e22b..f4951cafbc 100644
--- a/monitor.c
+++ b/monitor.c
@@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
if (!mon->rs)
return;
+ qemu_mutex_lock(&mon->mon_lock);
readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
if (show_prompt)
readline_show_prompt(mon->rs);
+ qemu_mutex_unlock(&mon->mon_lock);
}
int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
void *opaque)
{
if (mon->rs) {
+ qemu_mutex_lock(&mon->mon_lock);
readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
+ qemu_mutex_unlock(&mon->mon_lock);
/* prompt is printed on return from the command handler */
return 0;
} else {
@@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
cur_mon->qmp.commands = &qmp_commands;
}
-/* set the current CPU defined by the user */
-int monitor_set_cpu(int cpu_index)
+static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
{
CPUState *cpu;
@@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
if (cpu == NULL) {
return -1;
}
- g_free(cur_mon->mon_cpu_path);
- cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
+ g_free(mon->mon_cpu_path);
+ mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
return 0;
}
+/* set the current CPU defined by the user */
+int monitor_set_cpu(int cpu_index)
+{
+ int ret;
+
+ qemu_mutex_lock(&cur_mon->mon_lock);
+ ret = monitor_set_cpu_locked(cur_mon, cpu_index);
+ qemu_mutex_unlock(&cur_mon->mon_lock);
+
+ return ret;
+}
+
static CPUState *mon_get_cpu_sync(bool synchronize)
{
CPUState *cpu;
+ qemu_mutex_lock(&cur_mon->mon_lock);
if (cur_mon->mon_cpu_path) {
cpu = (CPUState *) object_resolve_path_type(cur_mon->mon_cpu_path,
TYPE_CPU, NULL);
@@ -1336,11 +1352,14 @@ static CPUState *mon_get_cpu_sync(bool synchronize)
}
if (!cur_mon->mon_cpu_path) {
if (!first_cpu) {
+ qemu_mutex_unlock(&cur_mon->mon_lock);
return NULL;
}
- monitor_set_cpu(first_cpu->cpu_index);
+ monitor_set_cpu_locked(cur_mon, first_cpu->cpu_index);
cpu = first_cpu;
}
+ qemu_mutex_unlock(&cur_mon->mon_lock);
+
if (synchronize) {
cpu_synchronize_state(cpu);
}
@@ -2239,6 +2258,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
{
mon_fd_t *monfd;
+ qemu_mutex_lock(&mon->mon_lock);
QLIST_FOREACH(monfd, &mon->fds, next) {
int fd;
@@ -2252,9 +2272,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;
--
2.14.3
On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> diff --git a/monitor.c b/monitor.c
> index c93aa4e22b..f4951cafbc 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
> if (!mon->rs)
> return;
>
> + qemu_mutex_lock(&mon->mon_lock);
> readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
> if (show_prompt)
> readline_show_prompt(mon->rs);
> + qemu_mutex_unlock(&mon->mon_lock);
> }
>
> int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> void *opaque)
> {
> if (mon->rs) {
> + qemu_mutex_lock(&mon->mon_lock);
> readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> + qemu_mutex_unlock(&mon->mon_lock);
> /* prompt is printed on return from the command handler */
> return 0;
> } else {
I'm not sure why the lock is being used around readline_start() and
readline_show_prompt(). There are other readline_*() callers who do not
take the lock, which is suspicious.
Can you explain the purpose of this?
> @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> cur_mon->qmp.commands = &qmp_commands;
> }
>
> -/* set the current CPU defined by the user */
> -int monitor_set_cpu(int cpu_index)
> +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
This function requires the BQL since qemu_get_cpu() accesses the cpus
list without acquiring qemu_cpu_list_lock.
Two options:
1. Document that monitor_set_cpu() must be called with the BQL held.
2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
monitor code requirements, document that qemu_cpu_list_lock code must
follow out-of-band monitor code requirements, and then take the lock.
#1 is more practical since we will probably never need to call
monitor_set_cpu() from out-of-band monitor code. Anyway, in that case
mon_lock is not needed unless there is a mon field that needs to be
protected.
> {
> CPUState *cpu;
>
> @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
> if (cpu == NULL) {
> return -1;
> }
> - g_free(cur_mon->mon_cpu_path);
> - cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> + g_free(mon->mon_cpu_path);
> + mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> return 0;
> }
>
> +/* set the current CPU defined by the user */
> +int monitor_set_cpu(int cpu_index)
> +{
> + int ret;
> +
> + qemu_mutex_lock(&cur_mon->mon_lock);
> + ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> + qemu_mutex_unlock(&cur_mon->mon_lock);
> +
> + return ret;
> +}
> +
> static CPUState *mon_get_cpu_sync(bool synchronize)
> {
This function calls monitor_set_cpu() so it must be called from the BQL.
The locking changes are probably not needed. This function just needs
to be documented as BQL-only.
> @@ -2239,6 +2258,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> {
> mon_fd_t *monfd;
>
> + qemu_mutex_lock(&mon->mon_lock);
> QLIST_FOREACH(monfd, &mon->fds, next) {
> int fd;
>
> @@ -2252,9 +2272,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);
What about all the other mon->fds users? They need to lock too,
otherwise we will QLIST_REMOVE() an fd while they are accessing the
list!
On Mon, Apr 30, 2018 at 11:10:50AM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 18, 2018 at 05:02:38PM +0800, Peter Xu wrote:
> > diff --git a/monitor.c b/monitor.c
> > index c93aa4e22b..f4951cafbc 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -306,16 +306,20 @@ void monitor_read_command(Monitor *mon, int show_prompt)
> > if (!mon->rs)
> > return;
> >
> > + qemu_mutex_lock(&mon->mon_lock);
> > readline_start(mon->rs, "(qemu) ", 0, monitor_command_cb, NULL);
> > if (show_prompt)
> > readline_show_prompt(mon->rs);
> > + qemu_mutex_unlock(&mon->mon_lock);
> > }
> >
> > int monitor_read_password(Monitor *mon, ReadLineFunc *readline_func,
> > void *opaque)
> > {
> > if (mon->rs) {
> > + qemu_mutex_lock(&mon->mon_lock);
> > readline_start(mon->rs, "Password: ", 1, readline_func, opaque);
> > + qemu_mutex_unlock(&mon->mon_lock);
> > /* prompt is printed on return from the command handler */
> > return 0;
> > } else {
>
> I'm not sure why the lock is being used around readline_start() and
> readline_show_prompt(). There are other readline_*() callers who do not
> take the lock, which is suspicious.
>
> Can you explain the purpose of this?
>
> > @@ -1308,8 +1312,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable,
> > cur_mon->qmp.commands = &qmp_commands;
> > }
> >
> > -/* set the current CPU defined by the user */
> > -int monitor_set_cpu(int cpu_index)
> > +static int monitor_set_cpu_locked(Monitor *mon, int cpu_index)
>
> This function requires the BQL since qemu_get_cpu() accesses the cpus
> list without acquiring qemu_cpu_list_lock.
>
> Two options:
> 1. Document that monitor_set_cpu() must be called with the BQL held.
> 2. Audit qemu_cpu_list_lock to check that it meets the out-of-band
> monitor code requirements, document that qemu_cpu_list_lock code must
> follow out-of-band monitor code requirements, and then take the lock.
>
> #1 is more practical since we will probably never need to call
> monitor_set_cpu() from out-of-band monitor code. Anyway, in that case
> mon_lock is not needed unless there is a mon field that needs to be
> protected.
You are right.
After a second thought I think readline is not needed to be protected.
IMHO it's only used in parsing phase, so actually we don't have
multi-threading issue with that (parsing is either happening in main
thread only, or monitor iothread only).
So I'll drop all the readline_* protections, and add a comment for
monitor_set_cpu() on BQL.
>
> > {
> > CPUState *cpu;
> >
> > @@ -1317,15 +1320,28 @@ int monitor_set_cpu(int cpu_index)
> > if (cpu == NULL) {
> > return -1;
> > }
> > - g_free(cur_mon->mon_cpu_path);
> > - cur_mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> > + g_free(mon->mon_cpu_path);
> > + mon->mon_cpu_path = object_get_canonical_path(OBJECT(cpu));
> > return 0;
> > }
> >
> > +/* set the current CPU defined by the user */
> > +int monitor_set_cpu(int cpu_index)
> > +{
> > + int ret;
> > +
> > + qemu_mutex_lock(&cur_mon->mon_lock);
> > + ret = monitor_set_cpu_locked(cur_mon, cpu_index);
> > + qemu_mutex_unlock(&cur_mon->mon_lock);
> > +
> > + return ret;
> > +}
> > +
> > static CPUState *mon_get_cpu_sync(bool synchronize)
> > {
>
> This function calls monitor_set_cpu() so it must be called from the BQL.
> The locking changes are probably not needed. This function just needs
> to be documented as BQL-only.
Yes. Will do.
>
> > @@ -2239,6 +2258,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
> > {
> > mon_fd_t *monfd;
> >
> > + qemu_mutex_lock(&mon->mon_lock);
> > QLIST_FOREACH(monfd, &mon->fds, next) {
> > int fd;
> >
> > @@ -2252,9 +2272,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);
>
> What about all the other mon->fds users? They need to lock too,
> otherwise we will QLIST_REMOVE() an fd while they are accessing the
> list!
Indeed! I think I'll drop most of this patch, only add protection for
mon->fds, and add those comments that you suggested. They make sense
to me. Thanks,
--
Peter Xu
© 2016 - 2026 Red Hat, Inc.