On 05/08/2017 05:00 PM, Stefano Stabellini wrote:
>>> Directly calling fcntl(F_SETFD) without first reading fcntl(F_GETFD) is
>>> (theoretically) incorrect. Better might be using qemu_set_cloexec()
>>> instead of open-coding something.
>>
>> Makes sense but the unchecked return of fcntl, discovered by Coverity,
>> would remain unfixed by calling qemu_set_cloexec here. I don't think I
>> am up for fixing all the call sites of qemu_set_cloexec.
>>
>> I am going to drop this change, and resend this patch was only the other
>> two fixes, fixing 1374836 only.
>
> Unless you would be fine with:
>
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 4d9189e..16894ad 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -182,7 +182,9 @@ void qemu_set_cloexec(int fd)
> {
> int f;
> f = fcntl(fd, F_GETFD);
> - fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> + assert(f != -1);
> + f = fcntl(fd, F_SETFD, f | FD_CLOEXEC);
> + assert(f != -1);Seems reasonable to me, but I don't know if anyone else would object. Changes semantics if someone ever calls qemu_set_cloexec(-1) (previously it would ignore the EBADF failures, now it will abort) - such callers are arguably broken, so that's okay by me. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
