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