Re: [Qemu-devel] [PATCH v4 2/4] monitor: protect mon->fds with mon_lock

[Peter Xu <peterx@redhat.com>]

On Wed, May 02, 2018 at 12:15:07PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (peterx@redhat.com) wrote:
> > mon->fds were protected by BQL.  Now protect it by mon_lock so that it
> > can even be used in monitor iothread.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  monitor.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 48882d28ae..9f361ec21e 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -213,7 +213,6 @@ struct Monitor {
> >      BlockCompletionFunc *password_completion_cb;
> >      void *password_opaque;
> >      mon_cmd_t *cmd_table;
> > -    QLIST_HEAD(,mon_fd_t) fds;
> >      QTAILQ_ENTRY(Monitor) entry;
> >  
> >      /*
> > @@ -225,6 +224,7 @@ struct Monitor {
> >      /*
> >       * Fields that are protected by the per-monitor lock.
> >       */
> > +    QLIST_HEAD(, mon_fd_t) fds;
> >      QString *outbuf;
> >      guint out_watch;
> >      /* Read under either BQL or mon_lock, written with BQL+mon_lock.  */
> > @@ -2207,6 +2207,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> >          return;
> >      }
> >  
> > +    qemu_mutex_lock(&cur_mon->mon_lock);
> >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >          if (strcmp(monfd->name, fdname) != 0) {
> >              continue;
> > @@ -2214,6 +2215,7 @@ void qmp_getfd(const char *fdname, Error **errp)
> >  
> >          close(monfd->fd);
> >          monfd->fd = fd;
> > +        qemu_mutex_unlock(&cur_mon->mon_lock);
> 
> Why is it safe to have a close() in a mon_lock'd region?
> We've got to make sure everything that happens in it is non-blocking.

Hmm indeed.  Let me move that close() out of the critical section. I
think I need to touch up below [1] too since it'll has similar
problem.

I'll wait for some more comments to repost.  Thanks,

> 
> Dave
> 
> >          return;
> >      }
> >  
> > @@ -2222,12 +2224,14 @@ 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)
> >  {
> >      mon_fd_t *monfd;
> >  
> > +    qemu_mutex_lock(&cur_mon->mon_lock);
> >      QLIST_FOREACH(monfd, &cur_mon->fds, next) {
> >          if (strcmp(monfd->name, fdname) != 0) {
> >              continue;
> > @@ -2237,9 +2241,11 @@ void qmp_closefd(const char *fdname, Error **errp)
> >          close(monfd->fd);
> >          g_free(monfd->name);
> >          g_free(monfd);
> > +        qemu_mutex_unlock(&cur_mon->mon_lock);

[1]

> >          return;
> >      }
> >  
> > +    qemu_mutex_unlock(&cur_mon->mon_lock);
> >      error_setg(errp, QERR_FD_NOT_FOUND, fdname);
> >  }
> >  
> > @@ -2247,6 +2253,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;
> >  
> > @@ -2260,10 +2267,12 @@ 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
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Peter Xu