[3/4] qemu-char: Convert udp char backend to parse/kind

Convert the udp char backend to the new style QAPI framework.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qemu-char.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 54 insertions(+), 15 deletions(-)

Peter Maydell <peter.maydell@linaro.org> writes:

> Convert the udp char backend to the new style QAPI framework.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  qemu-char.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 54 insertions(+), 15 deletions(-)
>
> diff --git a/qemu-char.c b/qemu-char.c
> index cac7edb..a01ccdc 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -2383,20 +2383,6 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
>      return chr;
>  }
>  
> -static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
> -{
> -    Error *local_err = NULL;
> -    int fd = -1;
> -
> -    fd = inet_dgram_opts(opts, &local_err);
> -    if (fd < 0) {
> -        qerror_report_err(local_err);
> -        error_free(local_err);
> -        return NULL;
> -    }
> -    return qemu_chr_open_udp_fd(fd);
> -}
> -
>  /***********************************************************/
>  /* TCP Net console */
>  
> @@ -3449,6 +3435,58 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>      backend->socket->addr = addr;
>  }
>  
> +static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
> +                               Error **errp)
> +{
> +    const char *host = qemu_opt_get(opts, "host");
> +    const char *port = qemu_opt_get(opts, "port");
> +    bool ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
> +    bool ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
> +    const char *localaddr = qemu_opt_get(opts, "localaddr");
> +    const char *localport = qemu_opt_get(opts, "localport");
> +    bool has_local = false;
> +    SocketAddress *addr;
> +
> +    if (host == NULL || strlen(host) == 0) {
> +        host = "localhost";
> +    }

We have to make up *some* value, because the host is mandatory for
InetSocketAddress in the schema.

The value We make up matches inet_dgram_opts().  Duplicating its choice
here isn't so hot.  At least it's an obvious choice.

Perhaps host = "" would do.  Your choice.

> +    if (port == NULL || strlen(port) == 0) {
> +        error_setg(errp, "chardev: udp: remote port not specified");
> +        return;
> +    }
> +    if (localport == NULL || strlen(localport) == 0) {
> +        localport = "0";
> +    } else {
> +        has_local = true;
> +    }
> +    if (localaddr == NULL || strlen(localaddr) == 0) {
> +        localaddr = "";
> +    } else {
> +        has_local = true;
> +    }

I neither localaddr not localport are specified, has_local remains
false.  Good.

If one of them is specified, we need to make up a value for the other,
because they're mandatory for InetSocketAddress in the schema.  We make
up values the eventual consumer inet_dgram_opts() treates the same as
"not specified".  Not nice, but it works.

> +
> +    backend->udp = g_new0(ChardevUdp, 1);
> +
> +    addr = g_new0(SocketAddress, 1);
> +    addr->kind = SOCKET_ADDRESS_KIND_INET;
> +    addr->inet = g_new0(InetSocketAddress, 1);
> +    addr->inet->host = g_strdup(host);
> +    addr->inet->port = g_strdup(port);
> +    addr->inet->has_ipv4 = addr->inet->ipv4 = ipv4;

This turns "ipv4=off" into "ipv4 not specified".  Works because the
eventual consumer inet_dgram_opts() treats them the same.  Still,
setting ->has_to = qemu_opt_get(opts, "ipv4") would be more obviously
correct.

> +    addr->inet->has_ipv6 = addr->inet->ipv6 = ipv6;

Likewise.

> +    backend->udp->remote = addr;
> +
> +    if (has_local) {
> +        backend->udp->has_local = true;
> +        addr = g_new0(SocketAddress, 1);
> +        addr->kind = SOCKET_ADDRESS_KIND_INET;
> +        addr->inet = g_new0(InetSocketAddress, 1);
> +        addr->inet->host = g_strdup(localaddr);
> +        addr->inet->port = g_strdup(localport);
> +        backend->udp->local = addr;
> +    }
> +}
> +

Like for backend=socket [PATCH 1], we're converting from QemuOpts to
QAPI-generated data structures, only to convert it right back, because
the internal interfaces haven't been qapified.  Ugly, but out of scope
for this series.

>  typedef struct CharDriver {
>      const char *name;
>      /* old, pre qapi */
> @@ -4139,7 +4177,8 @@ static void register_types(void)
>      register_char_driver_qapi("null", CHARDEV_BACKEND_KIND_NULL, NULL);
>      register_char_driver_qapi("socket", CHARDEV_BACKEND_KIND_SOCKET,
>                                qemu_chr_parse_socket);
> -    register_char_driver("udp", qemu_chr_open_udp);
> +    register_char_driver_qapi("udp", CHARDEV_BACKEND_KIND_UDP,
> +                              qemu_chr_parse_udp);
>      register_char_driver_qapi("ringbuf", CHARDEV_BACKEND_KIND_RINGBUF,
>                                qemu_chr_parse_ringbuf);
>      register_char_driver_qapi("file", CHARDEV_BACKEND_KIND_FILE,

On 19 August 2014 14:16, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Convert the udp char backend to the new style QAPI framework.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>>  qemu-char.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--------------
>>  1 file changed, 54 insertions(+), 15 deletions(-)
>>
>> diff --git a/qemu-char.c b/qemu-char.c
>> index cac7edb..a01ccdc 100644
>> --- a/qemu-char.c
>> +++ b/qemu-char.c
>> @@ -2383,20 +2383,6 @@ static CharDriverState *qemu_chr_open_udp_fd(int fd)
>>      return chr;
>>  }
>>
>> -static CharDriverState *qemu_chr_open_udp(QemuOpts *opts)
>> -{
>> -    Error *local_err = NULL;
>> -    int fd = -1;
>> -
>> -    fd = inet_dgram_opts(opts, &local_err);
>> -    if (fd < 0) {
>> -        qerror_report_err(local_err);
>> -        error_free(local_err);
>> -        return NULL;
>> -    }
>> -    return qemu_chr_open_udp_fd(fd);
>> -}
>> -
>>  /***********************************************************/
>>  /* TCP Net console */
>>
>> @@ -3449,6 +3435,58 @@ static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
>>      backend->socket->addr = addr;
>>  }
>>
>> +static void qemu_chr_parse_udp(QemuOpts *opts, ChardevBackend *backend,
>> +                               Error **errp)
>> +{
>> +    const char *host = qemu_opt_get(opts, "host");
>> +    const char *port = qemu_opt_get(opts, "port");
>> +    bool ipv4 = qemu_opt_get_bool(opts, "ipv4", 0);
>> +    bool ipv6 = qemu_opt_get_bool(opts, "ipv6", 0);
>> +    const char *localaddr = qemu_opt_get(opts, "localaddr");
>> +    const char *localport = qemu_opt_get(opts, "localport");
>> +    bool has_local = false;
>> +    SocketAddress *addr;
>> +
>> +    if (host == NULL || strlen(host) == 0) {
>> +        host = "localhost";
>> +    }
>
> We have to make up *some* value, because the host is mandatory for
> InetSocketAddress in the schema.
>
> The value We make up matches inet_dgram_opts().  Duplicating its choice
> here isn't so hot.  At least it's an obvious choice.
>
> Perhaps host = "" would do.  Your choice.

I think I'll leave this as is, then. (Here seemed the
obvious point to handle the required semantics for
command lines with an unspecified value. Eventually
the QemuOpts parsing code in inet_dgram_opts()
will disappear, hopefully.)

>> +    backend->udp = g_new0(ChardevUdp, 1);
>> +
>> +    addr = g_new0(SocketAddress, 1);
>> +    addr->kind = SOCKET_ADDRESS_KIND_INET;
>> +    addr->inet = g_new0(InetSocketAddress, 1);
>> +    addr->inet->host = g_strdup(host);
>> +    addr->inet->port = g_strdup(port);
>> +    addr->inet->has_ipv4 = addr->inet->ipv4 = ipv4;
>
> This turns "ipv4=off" into "ipv4 not specified".  Works because the
> eventual consumer inet_dgram_opts() treats them the same.  Still,
> setting ->has_to = qemu_opt_get(opts, "ipv4") would be more obviously
> correct.

Agreed (and ditto the similar remarks you made on patch 1).

>
>> +    addr->inet->has_ipv6 = addr->inet->ipv6 = ipv6;
>
> Likewise.
>
>> +    backend->udp->remote = addr;
>> +
>> +    if (has_local) {
>> +        backend->udp->has_local = true;
>> +        addr = g_new0(SocketAddress, 1);
>> +        addr->kind = SOCKET_ADDRESS_KIND_INET;
>> +        addr->inet = g_new0(InetSocketAddress, 1);
>> +        addr->inet->host = g_strdup(localaddr);
>> +        addr->inet->port = g_strdup(localport);
>> +        backend->udp->local = addr;
>> +    }
>> +}
>> +
>
> Like for backend=socket [PATCH 1], we're converting from QemuOpts to
> QAPI-generated data structures, only to convert it right back, because
> the internal interfaces haven't been qapified.  Ugly, but out of scope
> for this series.

Yep, the obvious next cleanup is to fold the
inet_dgram_opts() code into socket_dgram() and
avoid the double-conversion. As you say, separate
series.

-- PMM