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

[Markus Armbruster]

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;
}