[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/3] monitor: take mon_lock where proper |
Date: |
Mon, 30 Apr 2018 11:10:50 +0100 |
User-agent: |
Mutt/1.9.2 (2017-12-15) |
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!
signature.asc
Description: PGP signature