From: Markus Armbruster <armbru@redhat.com>
References: <20171221155905.3793-1-berrange@redhat.com>
	<20171221155905.3793-2-berrange@redhat.com>
	<cf30232b-8674-134a-38fa-0c6cb9f9af3e@redhat.com>
Date: Fri, 22 Dec 2017 09:55:29 +0100
In-Reply-To: <cf30232b-8674-134a-38fa-0c6cb9f9af3e@redhat.com> (Eric Blake's
	message of "Thu, 21 Dec 2017 12:47:17 -0600")
Message-ID: <87o9mrrvjy.fsf@dusky.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [Qemu-devel] [PATCH v2 1/2] io: move fd_is_socket() into common
 sockets code
To: Eric Blake <eblake@redhat.com>
Cc: "Daniel P. Berrange" <berrange@redhat.com>, qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, =?utf-8?Q?Marc-Andr=C3=A9?= Lureau <marcandre.lureau@redhat.com>

Eric Blake <eblake@redhat.com> writes:

> On 12/21/2017 09:59 AM, Daniel P. Berrange wrote:
>> The fd_is_socket() helper method is useful in a few places, so put it in
>> the common sockets code.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>>   include/qemu/sockets.h |  1 +
>>   io/channel-util.c      | 13 -------------
>>   util/qemu-sockets.c    | 13 +++++++++++++
>>   3 files changed, 14 insertions(+), 13 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> But while you are touching this...
>
>> +++ b/util/qemu-sockets.c
>> @@ -91,6 +91,19 @@ NetworkAddressFamily inet_netfamily(int family)
>>       return NETWORK_ADDRESS_FAMILY_UNKNOWN;
>>   }
>>   +bool fd_is_socket(int fd)
>> +{
>> +    int optval;
>> +    socklen_t optlen;
>> +    optlen = sizeof(optval);
>> +    return qemu_getsockopt(fd,
>> +                           SOL_SOCKET,
>> +                           SO_TYPE,
>> +                           (char *)&optval,
>
> This cast is pointless (although you are just moving it from the old
> code).  qemu_getsockopt() already takes care of casting for mingw
> (where the signature is not POSIX-compliant), and on all other
> platforms, the argument is already prototyped as void*; and since
> void* accepts anything, you don't have to go through an intermediate
> char*.

If we're tweaking, then I'd like it tweaked so:

    bool fd_is_socket(int fd)
    {
        int optval;
        socklen_t optlen = sizeof(optval);

        return !qemu_getsockopt(fd, SOL_SOCKET, SO_TYPE, &optval, &optlen);
    }

With or without such tweaks:
Reviewed-by: Markus Armbruster <armbru@redhat.com>