| Message ID | 20190115145256.9593-5-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | chardev: refactoring & many bugfixes related tcp_chr_wait_connected | expand |
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 >
[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.
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
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
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) {
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(-)