On Fri, May 18, 2018 at 02:27:00PM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@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
