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

[Markus Armbruster]

Peter Xu <address@hidden> 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.