Re: [Qemu-devel] [PULL v2 00/28] QAPI patches for 2017-05-04

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 05/08/2017 01:09 PM, Eric Blake wrote:
>> On 05/08/2017 11:42 AM, Stefan Hajnoczi wrote:
>>> On Fri, May 05, 2017 at 08:34:42AM +0200, Markus Armbruster wrote:
>>>> [v2]: Fix trailing space, note tweaks to PATCH 12 properly in the
>>>>
>>>> commit message 
>>>> The following changes since commit 
>>>> e619b14746e5d8c0e53061661fd0e1da01fd4d60:
>>>>
>>>>   Merge remote-tracking branch 'sthibault/tags/samuel-thibault' into 
>>>> staging (2017-05-02 15:16:29 +0100)
>>>>
>>>> are available in the git repository at:
>>>>
>>>>   git://repo.or.cz/qemu/armbru.git tags/pull-qapi-2017-05-04-v2
>>>
>>> make check fails when I apply this series:
>>>
>>>   GTESTER tests/test-char
>>>   GTester: last random seed: R02S47a681e3e741223084711f7aacacb19a
>>>   GTester: last random seed: R02S739de30ffeb0f93caedc22b2863f60a7
>> 
>> I tried bisecting, and I hit compilation failures during:
>> 
>>     sockets: Rename SocketAddress to SocketAddressLegacy
>> 
>> chardev/char-socket.c: In function ‘qmp_chardev_open_socket’:
>> chardev/char-socket.c:948:13: error: implicit declaration of function
>> ‘qapi_free_SocketAddress’ [-Werror=implicit-function-declaration]
>>              qapi_free_SocketAddress(s->addr);
>>              ^~~~~~~~~~~~~~~~~~~~~~~
>
> For this one, I got past by squashing in:
>
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 6b89209..1b99344 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -945,7 +945,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>                  goto error;
>              }
>
> -            qapi_free_SocketAddress(s->addr);
> +            qapi_free_SocketAddressLegacy(s->addr);
>              s->addr = socket_local_address(sioc->fd, errp);
>              update_disconnected_filename(s);
>
> @@ -1051,7 +1051,7 @@ char_socket_get_addr(Object *obj, Visitor *v,
> const char *name,
>  {
>      SocketChardev *s = SOCKET_CHARDEV(obj);
>
> -    visit_type_SocketAddress(v, name, &s->addr, errp);
> +    visit_type_SocketAddressLegacy(v, name, &s->addr, errp);
>  }
>
>  static bool

For completeness:

  @@ -1078,7 +1078,7 @@ static void char_socket_class_init(ObjectClass *oc, 
void *data)
       cc->chr_add_watch = tcp_chr_add_watch;
       cc->chr_update_read_handler = tcp_chr_update_read_handler;

  -    object_class_property_add(oc, "addr", "SocketAddress",
  +    object_class_property_add(oc, "addr", "SocketAddressLegacy",
                                 char_socket_get_addr, NULL,
                                 NULL, NULL, &error_abort);


To be reverted in the next patch, like the other two.

>
>
>> 
>> 
>> and
>>     sockets: Rename SocketAddressFlat to SocketAddress
>> 
>> chardev/char-socket.c: In function ‘qmp_chardev_open_socket’:
>> chardev/char-socket.c:948:37: error: passing argument 1 of
>> ‘qapi_free_SocketAddress’ from incompatible pointer type
>> [-Werror=incompatible-pointer-types]
>>              qapi_free_SocketAddress(s->addr);
>>                                      ^
>
> For this one, I squashed in:
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 69e6de6..8a321a1 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -945,7 +945,7 @@ static void qmp_chardev_open_socket(Chardev *chr,
>                  goto error;
>              }
>
> -            qapi_free_SocketAddressLegacy(s->addr);
> +            qapi_free_SocketAddress(s->addr);
>              s->addr = socket_local_address(sioc->fd, errp);
>              update_disconnected_filename(s);
>
> @@ -1051,7 +1051,7 @@ char_socket_get_addr(Object *obj, Visitor *v,
> const char *name,
>  {
>      SocketChardev *s = SOCKET_CHARDEV(obj);
>
> -    visit_type_SocketAddressLegacy(v, name, &s->addr, errp);
> +    visit_type_SocketAddress(v, name, &s->addr, errp);
>  }
>
>  static bool

Matching revert here.

> diff --git a/tests/test-char.c b/tests/test-char.c
> index 773a1c3..124d0c5 100644
> --- a/tests/test-char.c
> +++ b/tests/test-char.c
> @@ -291,7 +291,7 @@ static void char_socket_test(void)
>      Chardev *chr = qemu_chr_new("server", "tcp:127.0.0.1:0,server,nowait");
>      Chardev *chr_client;
>      QObject *addr;
> -    QDict *qdict, *data;
> +    QDict *qdict;
>      const char *port;
>      SocketIdleData d = { .chr = chr };
>      CharBackend be;
> @@ -306,8 +306,7 @@ static void char_socket_test(void)
>
>      addr = object_property_get_qobject(OBJECT(chr), "addr", &error_abort);
>      qdict = qobject_to_qdict(addr);
> -    data = qdict_get_qdict(qdict, "data");
> -    port = qdict_get_str(data, "port");
> +    port = qdict_get_str(qdict, "port");
>      tmp = g_strdup_printf("tcp:127.0.0.1:%s", port);
>      QDECREF(qdict);
>

This one demonstrates why direct use of QDict sucks: neither grep nor
the compiler help you with refactoring, only the debugger does.
Visitors are much friendlier there.

Going forward, I'd like to see less manual rummaging in QDicts.  If you
need efficiency, use C structs.  If you need the malleability of
QObject, use visitors.

> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index ad3ce65..bee3646 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1369,7 +1369,12 @@ SocketAddressLegacy
> *socket_address_crumple(SocketAddress *addr_flat)
>
>  SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
>  {
> -    SocketAddress *addr = g_new(SocketAddress, 1);
> +    SocketAddress *addr;
> +
> +    if (!addr_legacy) {
> +        return NULL;
> +    }
> +    addr = g_new(SocketAddress, 1);
>
>      switch (addr_legacy->type) {
>      case SOCKET_ADDRESS_LEGACY_KIND_INET:
>
>
> That was enough to get me back to a working 'make check'.

Needed because ChardevUdp member local is optional.

I searched the changes between this pull request's base and current
master for additional traps, but couldn't find any.

New pull request coming.  Thanks!