diff mbox

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

Message ID 1490879707-6060-8-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster March 30, 2017, 1:15 p.m. UTC 
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 <armbru@redhat.com>
---
 include/qemu/sockets.h | 11 +++++++++++
 util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

Comments

Eric Blake March 30, 2017, 2:42 p.m. UTC | #1 
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 <armbru@redhat.com>
> ---
>  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/

> +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.

> +    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?

> +        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;
> +}
>
Markus Armbruster March 30, 2017, 3:03 p.m. UTC | #2 
Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>> ---
>>  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;
}
Eric Blake March 30, 2017, 3:13 p.m. UTC | #3 
On 03/30/2017 10:03 AM, Markus Armbruster wrote:

>>> +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.
> 

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

Yes, that's better, and no tweaks to the .json file needed.
Max Reitz March 30, 2017, 4:20 p.m. UTC | #4 
On 30.03.2017 17:03, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>>> ---
>>>  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
diff mbox

Patch

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
+ *
+ * Convert SocketAddressFlat to SocketAddress.  Caller is responsible
+ * for freeing with qapi_free_SocketAddress().
+ *
+ * Returns: the argument converted to SocketAddress.
+ */
+SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat);
+
 #endif /* QEMU_SOCKETS_H */
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 4ae37bd..03a2a47 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -25,6 +25,7 @@ 
 #include "qapi/error.h"
 #include "qemu/sockets.h"
 #include "qemu/main-loop.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qobject-output-visitor.h"
 #include "qapi-visit.h"
@@ -1341,3 +1342,31 @@  char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
     }
     return buf;
 }
+
+SocketAddress *socket_address_crumple(SocketAddressFlat *addr_flat)
+{
+    SocketAddress *addr = g_new(SocketAddress, 1);
+
+    addr->type = addr_flat->type;
+    switch (addr->type) {
+    case SOCKET_ADDRESS_FLAT_TYPE_INET:
+        addr->u.inet.data = QAPI_CLONE(InetSocketAddress,
+                                        &addr_flat->u.inet);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_UNIX:
+        addr->u.q_unix.data = QAPI_CLONE(UnixSocketAddress, &
+                                          addr_flat->u.q_unix);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_VSOCK:
+        addr->u.vsock.data = QAPI_CLONE(VsockSocketAddress,
+                                         &addr_flat->u.vsock);
+        break;
+    case SOCKET_ADDRESS_FLAT_TYPE_FD:
+        addr->u.fd.data = QAPI_CLONE(String, &addr_flat->u.fd);
+        break;
+    default:
+        abort();
+    }
+
+    return addr;
+}