Re: [Qemu-devel] [PATCH v5 4/4] monitor: add lock to protect mon_fdsets

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> Similar to previous patch, but introduce a new global big lock for
> mon_fdsets.  Take it where needed.
>
> Reviewed-by: Stefan Hajnoczi <address@hidden>
> Signed-off-by: Peter Xu <address@hidden>
> ---
>  monitor.c | 64 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 53 insertions(+), 11 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index ae5bca9d7c..d72b695156 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -262,6 +262,9 @@ typedef struct QMPRequest QMPRequest;
>  /* Protects mon_list, monitor_event_state.  */
>  static QemuMutex monitor_lock;
>  
> +/* Protects mon_fdsets */
> +static QemuMutex mon_fdsets_lock;
> +
>  static QTAILQ_HEAD(mon_list, Monitor) mon_list;
>  static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;
>  static int mon_refcount;
> @@ -278,6 +281,16 @@ static QEMUClockType event_clock_type = 
> QEMU_CLOCK_REALTIME;
>  static void monitor_command_cb(void *opaque, const char *cmdline,
>                                 void *readline_opaque);
>  
> +/*
> + * This lock can be used very early, even during param parsing.
> + * Meanwhile it can also be used even at the end of main.  Let's keep
> + * it initialized for the whole lifecycle of QEMU.
> + */
> +static void __attribute__((constructor)) mon_fdsets_lock_init(void)
> +{
> +    qemu_mutex_init(&mon_fdsets_lock);
> +}
> +
>  /**
>   * Is @mon a QMP monitor?
>   */
> @@ -2307,9 +2320,11 @@ static void monitor_fdsets_cleanup(void)
>      MonFdset *mon_fdset;
>      MonFdset *mon_fdset_next;
>  
> +    qemu_mutex_lock(&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,
> @@ -2344,6 +2359,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);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          if (mon_fdset->id != fdset_id) {
>              continue;
> @@ -2363,10 +2379,12 @@ 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);
> @@ -2382,6 +2400,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>      MonFdsetFd *mon_fdset_fd;
>      FdsetInfoList *fdset_list = NULL;
>  
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          FdsetInfoList *fdset_info = g_malloc0(sizeof(*fdset_info));
>          FdsetFdInfoList *fdsetfd_list = NULL;
> @@ -2411,6 +2430,7 @@ FdsetInfoList *qmp_query_fdsets(Error **errp)
>          fdset_info->next = fdset_list;
>          fdset_list = fdset_info;
>      }
> +    qemu_mutex_unlock(&mon_fdsets_lock);
>  
>      return fdset_list;
>  }
> @@ -2423,6 +2443,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool 
> has_fdset_id, int64_t fdset_id,
>      MonFdsetFd *mon_fdset_fd;
>      AddfdInfo *fdinfo;
>  
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      if (has_fdset_id) {
>          QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>              /* Break if match found or match impossible due to ordering by 
> ID */
> @@ -2443,6 +2464,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool 
> has_fdset_id, int64_t fdset_id,
>              if (fdset_id < 0) {
>                  error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "fdset-id",
>                             "a non-negative value");
> +                qemu_mutex_unlock(&mon_fdsets_lock);
>                  return NULL;
>              }
>              /* Use specified fdset ID */
> @@ -2493,6 +2515,7 @@ AddfdInfo *monitor_fdset_add_fd(int fd, bool 
> has_fdset_id, int64_t fdset_id,
>      fdinfo->fdset_id = mon_fdset->id;
>      fdinfo->fd = mon_fdset_fd->fd;
>  
> +    qemu_mutex_unlock(&mon_fdsets_lock);
>      return fdinfo;
>  }
>  
> @@ -2502,7 +2525,9 @@ int monitor_fdset_get_fd(int64_t fdset_id, int flags)
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd;
>      int mon_fd_flags;
> +    int ret = -1;

Suggest not to initialize ret, and instead ret = -1 on both failure
paths.

>  
> +    qemu_mutex_lock(&mon_fdsets_lock);
>      QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>          if (mon_fdset->id != fdset_id) {
>              continue;
> @@ -2510,49 +2535,62 @@ 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) {
> -                return -1;
> +                goto out;

Preexisting: we fail without setting errno.  Smells buggy.

Can we avoid setting errno and return a negative errno code instead?

>              }
>  
>              if ((flags & O_ACCMODE) == (mon_fd_flags & O_ACCMODE)) {
> -                return mon_fdset_fd->fd;
> +                ret = mon_fdset_fd->fd;
> +                goto out;
>              }
>          }
>          errno = EACCES;
> -        return -1;
> +        break;
>      }
> -#endif
> -
> +out:
> +    qemu_mutex_unlock(&mon_fdsets_lock);
> +    return ret;
> +#else

#ifndef _WIN32 ... #endif becomes #ifndef _WIN32 ... #else ... #endif.
I hate negative conditionals with else.  Mind to swap?

>      errno = ENOENT;
>      return -1;
> +#endif
>  }
>  
>  int monitor_fdset_dup_fd_add(int64_t fdset_id, int dup_fd)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
> +    int ret = -1;

Dead initializer, please remove.

>  
> +    qemu_mutex_lock(&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) {
> -                return -1;
> +                ret = -1;
> +                goto out;
>              }
>          }
>          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);
> -        return 0;
> +        ret = 0;
> +        break;
>      }
> -    return -1;
> +
> +out:
> +    qemu_mutex_unlock(&mon_fdsets_lock);
> +    return ret;
>  }
>  
>  static int monitor_fdset_dup_fd_find_remove(int dup_fd, bool remove)
>  {
>      MonFdset *mon_fdset;
>      MonFdsetFd *mon_fdset_fd_dup;
> +    int ret = -1;

Likewise.

>  
> +    qemu_mutex_lock(&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) {
> @@ -2561,14 +2599,18 @@ static int monitor_fdset_dup_fd_find_remove(int 
> dup_fd, bool remove)
>                      if (QLIST_EMPTY(&mon_fdset->dup_fds)) {
>                          monitor_fdset_cleanup(mon_fdset);
>                      }
> -                    return -1;
> +                    ret = -1;
> +                    goto out;
>                  } else {
> -                    return mon_fdset->id;
> +                    ret = mon_fdset->id;
> +                    goto out;
>                  }
>              }
>          }
>      }
> -    return -1;
> +out:
> +    qemu_mutex_unlock(&mon_fdsets_lock);
> +    return ret;
>  }
>  
>  int monitor_fdset_dup_fd_find(int dup_fd)