Re: [Qemu-devel] [PATCH 03/12] chardev: forbid 'wait' option with client sockets

[Thomas Huth]

On 2019-01-15 15:52, Daniel P. Berrangé wrote:
> The 'wait'/'nowait' parameter is used to tell server sockets whether to
> block until a client is accepted during initialization. Client chardevs
> have always silently ignored this option. Various tests were mistakenly
> passing this option for their client chardevs.
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  chardev/char-socket.c          | 12 +++++++++++-
>  tests/ivshmem-test.c           |  2 +-
>  tests/libqtest.c               |  4 ++--
>  tests/test-filter-redirector.c |  4 ++--
>  4 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 4570755adf..233f16f72d 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -1047,6 +1047,12 @@ static bool qmp_chardev_validate_socket(ChardevSocket 
> *sock,
>              error_setg(errp, "%s", "Websocket client is not implemented");
>              return false;
>          }
> +        if (sock->has_wait) {
> +            error_setg(errp, "%s",
> +                       "'wait' option is incompatible with "
> +                       "socket in client connect mode");
> +            return false;
> +        }
>      }
>  
>      return true;
> @@ -1220,7 +1226,11 @@ static void qemu_chr_parse_socket(QemuOpts *opts, 
> ChardevBackend *backend,
>      sock->tn3270 = is_tn3270;
>      sock->has_websocket = true;
>      sock->websocket = is_websock;
> -    sock->has_wait = true;
> +    /*
> +     * 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_reconnect = qemu_opt_find(opts, "reconnect");
>      sock->reconnect = reconnect;
> diff --git a/tests/ivshmem-test.c b/tests/ivshmem-test.c
> index fe5eb304b1..faffc1c624 100644
> --- a/tests/ivshmem-test.c
> +++ b/tests/ivshmem-test.c
> @@ -293,7 +293,7 @@ static void *server_thread(void *data)
>  
>  static void setup_vm_with_server(IVState *s, int nvectors, bool msi)
>  {
> -    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s,nowait "
> +    char *cmd = g_strdup_printf("-chardev socket,id=chr0,path=%s "
>                                  "-device ivshmem%s,chardev=chr0,vectors=%d",
>                                  tmpserver,
>                                  msi ? "-doorbell" : ",size=1M,msi=off",

I think there will be a conflict with the patch 'Remove deprecated
"ivshmem" legacy device' that is currently in Michael's PULL request ...
could you please rebase this patch in a day or two once that PULL has
been merged?

Apart from that, patch looks fine to me.

Acked-by: Thomas Huth <address@hidden>