Re: [Qemu-devel] [PATCH 04/12] chardev: remove many local variables in qemu_chr_parse_socket

[Eric Blake]

[adding Markus in relation to a QemuOpts observation]

On 1/15/19 8:52 AM, Daniel P. Berrangé wrote:
> Now that all validation is separated off into a separate method,
> we can directly populate the ChardevSocket struct from the
> QemuOpts values, avoiding many local variables.
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  chardev/char-socket.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
> 

> @@ -1216,26 +1208,30 @@ static void qemu_chr_parse_socket(QemuOpts *opts, 
> ChardevBackend *backend,
>      sock = backend->u.socket.data = g_new0(ChardevSocket, 1);
>      qemu_chr_parse_common(opts, qapi_ChardevSocket_base(sock));
>  
> -    sock->has_nodelay = true;
> -    sock->nodelay = do_nodelay;
> +    sock->has_nodelay = qemu_opt_get(opts, "delay");
> +    sock->nodelay = !qemu_opt_get_bool(opts, "delay", true);

Unrelated to this patch, but my recent proposal to make QemuOpts be a
bit more typesafe:

https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg02042.html

does not like users calling qemu_opt_get() of an option that has an
integer default (as opposed to a string default), even if the only
reason the caller was doing it was to learn if the option is present.  I
wonder if we should introduce

bool qemu_opt_has(QemuOpts *opts, const char *name)

and then replace existing callers which merely call qemu_opt_get() to
learn whether an optional option was present/defaulted but without
caring about its string value.  I also wonder if Coccinelle can be
employed to determine which callers fit that bill (that is, detect when
a const char * result is ignored or solely used in a bool context, vs.
when it is actually used as a string parameter to another function
and/or saved in a const char * variable).

Now, on to actual review,

> +    /*
> +     * We have different default to QMP for 'server', hence
> +     * we can't just check for existance of 'server'

s/existance/existence/

> +     */
>      sock->has_server = true;
> -    sock->server = is_listen;
> -    sock->has_telnet = true;
> -    sock->telnet = is_telnet;
> -    sock->has_tn3270 = true;
> -    sock->tn3270 = is_tn3270;
> -    sock->has_websocket = true;
> -    sock->websocket = is_websock;
> +    sock->server = qemu_opt_get_bool(opts, "server", false);
> +    sock->has_telnet = qemu_opt_get(opts, "telnet");
> +    sock->telnet = qemu_opt_get_bool(opts, "telnet", false);
> +    sock->has_tn3270 = qemu_opt_get(opts, "tn3270");
> +    sock->tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> +    sock->has_websocket = qemu_opt_get(opts, "websocket");
> +    sock->websocket = qemu_opt_get_bool(opts, "websocket", false);
>      /*
>       * We have different default to QMP for 'wait' when 'server'
>       * is set, hence we can't just check for existance of 'wait'

but then again, you were copy-pasting an existing typo.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature