Similar to previous patch, but introduce a new global big lock for
mon_fdsets.  Take it where needed.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 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;
 
+    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;
             }
 
             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
     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;
 
+    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;
 
+    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)
-- 
2.17.0

Peter Xu <peterx@redhat.com> writes:

> Similar to previous patch, but introduce a new global big lock for
> mon_fdsets.  Take it where needed.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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)

On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:

[...]

> > @@ -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.

[1]

But there is a third hidden failure path that we failed to find the fd
specified?  In that case we still need that initial value.

But I didn't really notice that this function is returning error with
-1 paired with errno.  So instead of set -1 here I may need to
initialize it to -ENOENT, and I can convert it back to errno when
return.  Please see below.

> 
> >  
> > +    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.

Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
the errno might be polluted by the mutex unlocking operation.

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

Yes that'll be nice, but it's getting out of the scope of this
patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
its callers.

> 
> >              }
> >  
> >              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);

[2]

> > +    return ret;

So in my next post I'll make sure I return -1 when error happens, and
errno should contain the correct (positive) error.

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

Sure I can.

> 
> >      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.

IMHO it's the same as above [1], so we still need that, right?

> 
> >  
> > +    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.

Same as [1]?

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
>
> [...]
>
>> > @@ -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.
>
> [1]
>
> But there is a third hidden failure path that we failed to find the fd
> specified?  In that case we still need that initial value.

You're right.  However, that failure path could be made explicit easily:

        QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
            [got out on error and on finding the right one...]
        }
        ret = -1;
        errno = ENOENT;

    out:
        qemu_mutex_unlock(&mon_fdsets_lock);
        return ret;

I find this clearer.  Your choice.

> But I didn't really notice that this function is returning error with
> -1 paired with errno.  So instead of set -1 here I may need to
> initialize it to -ENOENT, and I can convert it back to errno when
> return.  Please see below.
>
>> 
>> >  
>> > +    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.
>
> Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
> the errno might be polluted by the mutex unlocking operation.

Good point.

>> Can we avoid setting errno and return a negative errno code instead?
>
> Yes that'll be nice, but it's getting out of the scope of this
> patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
> its callers.

I'd change just monitor_fdset_get_fd(), and have its only caller
qemu_open() do

        fd = monitor_fdset_get_fd(fdset_id, flags);
        if (fd < 0) {
            errno = -fd;
            return -1;
        }

>> >              }
>> >  
>> >              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);
>
> [2]
>
>> > +    return ret;
>
> So in my next post I'll make sure I return -1 when error happens, and
> errno should contain the correct (positive) error.
>
>> > +#else
>> 
>> #ifndef _WIN32 ... #endif becomes #ifndef _WIN32 ... #else ... #endif.
>> I hate negative conditionals with else.  Mind to swap?
>
> Sure I can.
>
>> 
>> >      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.
>
> IMHO it's the same as above [1], so we still need that, right?

You're right.

>> >  
>> > +    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.
>
> Same as [1]?

Right again.

On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
> >
> > [...]
> >
> >> > @@ -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.
> >
> > [1]
> >
> > But there is a third hidden failure path that we failed to find the fd
> > specified?  In that case we still need that initial value.
> 
> You're right.  However, that failure path could be made explicit easily:
> 
>         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>             [got out on error and on finding the right one...]
>         }
>         ret = -1;
>         errno = ENOENT;
> 
>     out:
>         qemu_mutex_unlock(&mon_fdsets_lock);
>         return ret;
> 
> I find this clearer.  Your choice.

Yes this works too.  Considering that I just posted v6, I'll
temporarily just keep the old way.

> 
> > But I didn't really notice that this function is returning error with
> > -1 paired with errno.  So instead of set -1 here I may need to
> > initialize it to -ENOENT, and I can convert it back to errno when
> > return.  Please see below.
> >
> >> 
> >> >  
> >> > +    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.
> >
> > Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
> > the errno might be polluted by the mutex unlocking operation.
> 
> Good point.
> 
> >> Can we avoid setting errno and return a negative errno code instead?
> >
> > Yes that'll be nice, but it's getting out of the scope of this
> > patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
> > its callers.
> 
> I'd change just monitor_fdset_get_fd(), and have its only caller
> qemu_open() do
> 
>         fd = monitor_fdset_get_fd(fdset_id, flags);
>         if (fd < 0) {
>             errno = -fd;
>             return -1;
>         }

Yes this I can do.  I'll avoid resending for this change only (and
IMHO it can also be a follow-up patch).  If the latest version 6 will
need further refinings I'll touch up qemu_open() for this altogether.

Thanks,

-- 
Peter Xu

Peter Xu <peterx@redhat.com> writes:

> On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
>> >
>> > [...]
>> >
>> >> > @@ -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.
>> >
>> > [1]
>> >
>> > But there is a third hidden failure path that we failed to find the fd
>> > specified?  In that case we still need that initial value.
>> 
>> You're right.  However, that failure path could be made explicit easily:
>> 
>>         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
>>             [got out on error and on finding the right one...]
>>         }
>>         ret = -1;
>>         errno = ENOENT;
>> 
>>     out:
>>         qemu_mutex_unlock(&mon_fdsets_lock);
>>         return ret;
>> 
>> I find this clearer.  Your choice.
>
> Yes this works too.  Considering that I just posted v6, I'll
> temporarily just keep the old way.

Your v6 raced with my review of v5.  Do you intend to post v7?  If not,
I need to figure out what I can and want to do to v6 on commit to my
tree.

>> > But I didn't really notice that this function is returning error with
>> > -1 paired with errno.  So instead of set -1 here I may need to
>> > initialize it to -ENOENT, and I can convert it back to errno when
>> > return.  Please see below.
>> >
>> >> 
>> >> >  
>> >> > +    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.
>> >
>> > Indeed.  Here I possibly need to set "ret = -errno" since at [2] below
>> > the errno might be polluted by the mutex unlocking operation.
>> 
>> Good point.
>> 
>> >> Can we avoid setting errno and return a negative errno code instead?
>> >
>> > Yes that'll be nice, but it's getting out of the scope of this
>> > patchset.  So I'll try to avoid touching that.  I mean qemu_open() and
>> > its callers.
>> 
>> I'd change just monitor_fdset_get_fd(), and have its only caller
>> qemu_open() do
>> 
>>         fd = monitor_fdset_get_fd(fdset_id, flags);
>>         if (fd < 0) {
>>             errno = -fd;
>>             return -1;
>>         }
>
> Yes this I can do.  I'll avoid resending for this change only (and
> IMHO it can also be a follow-up patch).

Followup patch is fine.

>                                          If the latest version 6 will
> need further refinings I'll touch up qemu_open() for this altogether.

Just to avoid misunderstandings: I'm not asking you to change
qemu_open()'s contract.  Since qemu_open() is basically a compatibility
helper to emulate modern open() with O_CLOEXEC on old systems, with some
entirely undocumented fd set functionality thrown in (grrr...), having
it set errno on failure just like open() makes some sense.

On Wed, May 23, 2018 at 10:39:20AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Thu, May 17, 2018 at 03:03:02PM +0200, Markus Armbruster wrote:
> >> >
> >> > [...]
> >> >
> >> >> > @@ -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.
> >> >
> >> > [1]
> >> >
> >> > But there is a third hidden failure path that we failed to find the fd
> >> > specified?  In that case we still need that initial value.
> >> 
> >> You're right.  However, that failure path could be made explicit easily:
> >> 
> >>         QLIST_FOREACH(mon_fdset, &mon_fdsets, next) {
> >>             [got out on error and on finding the right one...]
> >>         }
> >>         ret = -1;
> >>         errno = ENOENT;
> >> 
> >>     out:
> >>         qemu_mutex_unlock(&mon_fdsets_lock);
> >>         return ret;
> >> 
> >> I find this clearer.  Your choice.
> >
> > Yes this works too.  Considering that I just posted v6, I'll
> > temporarily just keep the old way.
> 
> Your v6 raced with my review of v5.  Do you intend to post v7?  If not,
> I need to figure out what I can and want to do to v6 on commit to my
> tree.

I can repost v7 after we finish the discussion in the other thread:

  [PATCH v5 3/4] monitor: more comments on lock-free fleids/funcs
  Message-ID: <87muwqixla.fsf@dusky.pond.sub.org>

I think there is a comment to be refined, meanwhile I can at least
pick up the qemu_open() suggestion too.

Regards,

-- 
Peter Xu