[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é <berra...@redhat.com> > --- > 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
