Re: [Qemu-devel] [PATCH] Fix issues affecting Xen 9pfs discovered by Coverity

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]