Re: [Qemu-devel] [PATCH 3/4] qemu-char: Convert udp char backend to parse/kind

[Peter Maydell]

On 19 August 2014 14:16, Markus Armbruster <address@hidden> wrote:
> Peter Maydell <address@hidden> writes:
>
>> Convert the udp char backend to the new style QAPI framework.
>>
>> Signed-off-by: Peter Maydell <address@hidden>
>> ---
>>  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