Re: [Qemu-devel] [RFC v2 for-2.9 07/10] sockets: New helper socket_address_crumple()

[Max Reitz]

On 30.03.2017 17:03, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 03/30/2017 08:15 AM, Markus Armbruster wrote:
>>> SocketAddress is a simple union, and simple unions are awkward: they
>>> have their variant members wrapped in a "data" object on the wire, and
>>> require additional indirections in C.  I intend to limit its use to
>>> existing external interfaces.  New ones should use SocketAddressFlat.
>>> I further intend to convert all internal interfaces to
>>> SocketAddressFlat.  This helper should go away then.
>>>
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>> ---
>>>  include/qemu/sockets.h | 11 +++++++++++
>>>  util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
>>>  2 files changed, 40 insertions(+)
>>>
>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>> index 5f1bab9..cef05a5 100644
>>> --- a/include/qemu/sockets.h
>>> +++ b/include/qemu/sockets.h
>>> @@ -119,4 +119,15 @@ SocketAddress *socket_remote_address(int fd, Error 
>>> **errp);
>>>   */
>>>  char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
>>>  
>>> +/**
>>> + * socket_address_crumple:
>>> + * @addr_flat: the socket addres to crumple
>>
>> s/addres/address/
> 
> Fixing...
> 
>>> +SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
>>> +{
>>> +    SocketAddress *addr = g_new(SocketAddress, 1);
>>> +
>>> +    addr->type = addr_flat->type;
>>
>> Works only because our enum is defined in the same order as the simple
>> union's members.  A bit fragile, so maybe we want to comment in the
>> .json file that we can't reorder members of either the enum or the
>> simple union's 'data'?  Or it might even tickle a picky compiler to warn
>> about assignment between incompatible enum types.  Another option would
>> be making it robust by instead doing switch(addr_flat->type) and
>> assigning to addr->type in each branch of the switch.
> 
> Sold.
> 
>>> +    switch (addr->type) {
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_INET:
>>> +        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
>>> +                                        &addr_flat->u.inet);
>>
>> Indentation is off.
> 
>>> +        break;
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
>>> +        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
>>> +                                          addr_flat->u.q_unix);
>>
>> Why is the unary & split from its argument?
> 
> Editing accident.
> 
>>> +        break;
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
>>> +        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
>>> +                                         &addr_flat->u.vsock);
>>
>> More indentation problems.
>>
>>> +        break;
>>> +    case SOCKET_ADDRESS_FLAT_TYPE_FD:
>>> +        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
>>> +        break;
>>> +    default:
>>> +        abort();
>>> +    }
>>> +
>>> +    return addr;
>>> +}
>>>
> 
> Now looks like this:
> 
> SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
> {
>     SocketAddress *addr = g_new(SocketAddress, 1);
> 
>     switch (addr_flat->type) {
>     case SOCKET_ADDRESS_FLAT_TYPE_INET:
>         addr->type = SOCKET_ADDRESS_KIND_INET;
>         addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
>                                        &addr_flat->u.inet);
>         break;
>     case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
>         addr->type = SOCKET_ADDRESS_KIND_UNIX;
>         addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress,
>                                          &addr_flat->u.q_unix);
>         break;
>     case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
>         addr->type = SOCKET_ADDRESS_KIND_VSOCK;
>         addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
>                                         &addr_flat->u.vsock);
>         break;
>     case SOCKET_ADDRESS_FLAT_TYPE_FD:
>         addr->type = SOCKET_ADDRESS_KIND_FD;
>         addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
>         break;
>     default:
>         abort();
>     }
> 
>     return addr;
> }

Looks good!

Max

signature.asc
Description: OpenPGP digital signature