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

[Peter Xu <peterx@redhat.com>]

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