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
