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é <berrange@redhat.com>
---
chardev/char-socket.c | 40 ++++++++++++++++++----------------------
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 233f16f72d..ba86284ea9 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -1186,18 +1186,10 @@ error:
static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
Error **errp)
{
- bool is_listen = qemu_opt_get_bool(opts, "server", false);
- bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
- bool is_telnet = qemu_opt_get_bool(opts, "telnet", false);
- bool is_tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
- bool is_websock = qemu_opt_get_bool(opts, "websocket", false);
- bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true);
- int64_t reconnect = qemu_opt_get_number(opts, "reconnect", 0);
const char *path = qemu_opt_get(opts, "path");
const char *host = qemu_opt_get(opts, "host");
const char *port = qemu_opt_get(opts, "port");
const char *fd = qemu_opt_get(opts, "fd");
- const char *tls_creds = qemu_opt_get(opts, "tls-creds");
SocketAddressLegacy *addr;
ChardevSocket *sock;
@@ -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);
+ /*
+ * We have different default to QMP for 'server', hence
+ * we can't just check for existance of 'server'
+ */
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'
*/
- sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
- sock->wait = is_waitconnect;
+ sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
+ sock->wait = qemu_opt_get_bool(opts, "wait", true);
sock->has_reconnect = qemu_opt_find(opts, "reconnect");
- sock->reconnect = reconnect;
- sock->has_tls_creds = tls_creds;
- sock->tls_creds = g_strdup(tls_creds);
+ sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
+ sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
+ sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
addr = g_new0(SocketAddressLegacy, 1);
if (path) {
--
2.20.1
Hi
On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> ---
> chardev/char-socket.c | 40 ++++++++++++++++++----------------------
> 1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 233f16f72d..ba86284ea9 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1186,18 +1186,10 @@ error:
> static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> Error **errp)
> {
> - bool is_listen = qemu_opt_get_bool(opts, "server", false);
> - bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> - bool is_telnet = qemu_opt_get_bool(opts, "telnet", false);
> - bool is_tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> - bool is_websock = qemu_opt_get_bool(opts, "websocket", false);
> - bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true);
> - int64_t reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> const char *path = qemu_opt_get(opts, "path");
> const char *host = qemu_opt_get(opts, "host");
> const char *port = qemu_opt_get(opts, "port");
> const char *fd = qemu_opt_get(opts, "fd");
> - const char *tls_creds = qemu_opt_get(opts, "tls-creds");
> SocketAddressLegacy *addr;
> ChardevSocket *sock;
>
> @@ -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);
> + /*
> + * We have different default to QMP for 'server', hence
> + * we can't just check for existance of 'server'
> + */
> 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'
> */
> - sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
> - sock->wait = is_waitconnect;
> + sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
> + sock->wait = qemu_opt_get_bool(opts, "wait", true);
That changes slightly the behaviour, since wait was not parsed before
when !is_listen.
Since we are "correcting" (breaking?) things anyway in previous patches too:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> sock->has_reconnect = qemu_opt_find(opts, "reconnect");
> - sock->reconnect = reconnect;
> - sock->has_tls_creds = tls_creds;
> - sock->tls_creds = g_strdup(tls_creds);
> + sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> + sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
> + sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
>
> addr = g_new0(SocketAddressLegacy, 1);
> if (path) {
> --
> 2.20.1
>
On Tue, Jan 15, 2019 at 11:18:21PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Tue, Jan 15, 2019 at 6:53 PM Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> > ---
> > chardev/char-socket.c | 40 ++++++++++++++++++----------------------
> > 1 file changed, 18 insertions(+), 22 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 233f16f72d..ba86284ea9 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -1186,18 +1186,10 @@ error:
> > static void qemu_chr_parse_socket(QemuOpts *opts, ChardevBackend *backend,
> > Error **errp)
> > {
> > - bool is_listen = qemu_opt_get_bool(opts, "server", false);
> > - bool is_waitconnect = is_listen && qemu_opt_get_bool(opts, "wait", true);
> > - bool is_telnet = qemu_opt_get_bool(opts, "telnet", false);
> > - bool is_tn3270 = qemu_opt_get_bool(opts, "tn3270", false);
> > - bool is_websock = qemu_opt_get_bool(opts, "websocket", false);
> > - bool do_nodelay = !qemu_opt_get_bool(opts, "delay", true);
> > - int64_t reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> > const char *path = qemu_opt_get(opts, "path");
> > const char *host = qemu_opt_get(opts, "host");
> > const char *port = qemu_opt_get(opts, "port");
> > const char *fd = qemu_opt_get(opts, "fd");
> > - const char *tls_creds = qemu_opt_get(opts, "tls-creds");
> > SocketAddressLegacy *addr;
> > ChardevSocket *sock;
> >
> > @@ -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);
> > + /*
> > + * We have different default to QMP for 'server', hence
> > + * we can't just check for existance of 'server'
> > + */
> > 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'
> > */
> > - sock->has_wait = qemu_opt_find(opts, "wait") || is_listen;
> > - sock->wait = is_waitconnect;
> > + sock->has_wait = qemu_opt_find(opts, "wait") || sock->server;
> > + sock->wait = qemu_opt_get_bool(opts, "wait", true);
>
> That changes slightly the behaviour, since wait was not parsed before
> when !is_listen.
>
> Since we are "correcting" (breaking?) things anyway in previous patches too:
Yes, in the !is_listen case, the previous patch will see has_wait == true
and raise an error. So this patch is not a change in behaviour wrt the
previous patch, but the series as a whole is a change in behaviour due to
reporting errors for bad "wait" usage in clients.
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> > sock->has_reconnect = qemu_opt_find(opts, "reconnect");
> > - sock->reconnect = reconnect;
> > - sock->has_tls_creds = tls_creds;
> > - sock->tls_creds = g_strdup(tls_creds);
> > + sock->reconnect = qemu_opt_get_number(opts, "reconnect", 0);
> > + sock->has_tls_creds = qemu_opt_get(opts, "tls-creds");
> > + sock->tls_creds = g_strdup(qemu_opt_get(opts, "tls-creds"));
> >
> > addr = g_new0(SocketAddressLegacy, 1);
> > if (path) {
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 :|
[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é <berrange@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
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é <berrange@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 :|
© 2016 - 2026 Red Hat, Inc.