On Tue, Jan 15, 2019 at 01:33:25PM -0600, Eric Blake wrote: > [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).
Yes, if you're going to enforce type safety on qemu_opt_get, then then we definitely need to have qemu_opt_has() function to check for existance, as this is needed in other places too. > > 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. Both typos were from this series Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
