Peter Xu <pet...@redhat.com> writes:
> 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.
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.
