[04/12] chardev: remove many local variables in qemu_chr_parse_socket

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(-)

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