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

[Peter Xu]

On Wed, May 23, 2018 at 10:39:20AM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
> >> 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.
> >
> > 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: <address@hidden>

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

Regards,

-- 
Peter Xu