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

[Markus Armbruster]

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.

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