Re: [Qemu-devel] [PATCH 3/3] qapi: convert add_client

[Markus Armbruster <armbru@redhat.com>]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> Also fixes a few issues while there:
>
>  1. The fd returned by monitor_get_fd() leaks in most error conditions
>  2. monitor_get_fd() return value is not checked. Best case we get
>     an error that is not correctly reported, worse case one of the
>     functions using the fd (with value of -1) will explode
>  3. A few error conditions aren't reported

4. We now "use up" @fdname always.  Before, it was left alone for
   invalid @protocol.

>
> Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
> ---
>  monitor.c        | 39 ---------------------------------------
>  qapi-schema.json | 23 +++++++++++++++++++++++
>  qmp-commands.hx  |  5 +----
>  qmp.c            | 43 +++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 67 insertions(+), 43 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 645f8f4..e18df38 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -944,45 +944,6 @@ static void do_trace_print_events(Monitor *mon)
>      trace_print_events((FILE *)mon, &monitor_fprintf);
>  }
>  
> -static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_data)
> -{
> -    const char *protocol  = qdict_get_str(qdict, "protocol");
> -    const char *fdname = qdict_get_str(qdict, "fdname");
> -    CharDriverState *s;
> -
> -    if (strcmp(protocol, "spice") == 0) {
> -        int fd = monitor_get_fd(mon, fdname, NULL);
> -        int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> -        int tls = qdict_get_try_bool(qdict, "tls", 0);
> -        if (!using_spice) {
> -            /* correct one? spice isn't a device ,,, */
> -            qerror_report(QERR_DEVICE_NOT_ACTIVE, "spice");
> -            return -1;
> -        }
> -        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
> -            close(fd);
> -        }
> -        return 0;
> -#ifdef CONFIG_VNC
> -    } else if (strcmp(protocol, "vnc") == 0) {
> -	int fd = monitor_get_fd(mon, fdname, NULL);
> -        int skipauth = qdict_get_try_bool(qdict, "skipauth", 0);
> -	vnc_display_add_client(NULL, fd, skipauth);
> -	return 0;
> -#endif
> -    } else if ((s = qemu_chr_find(protocol)) != NULL) {
> -	int fd = monitor_get_fd(mon, fdname, NULL);
> -	if (qemu_chr_add_client(s, fd) < 0) {
> -	    qerror_report(QERR_ADD_CLIENT_FAILED);
> -	    return -1;
> -	}
> -	return 0;
> -    }
> -
> -    qerror_report(QERR_INVALID_PARAMETER, "protocol");
> -    return -1;
> -}
> -
>  static int client_migrate_info(Monitor *mon, const QDict *qdict,
>                                 MonitorCompletion cb, void *opaque)
>  {
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 14e4419..c977ec7 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -33,6 +33,29 @@
>              'MigrationExpected' ] }
>  
>  ##
> +# @add_client
> +#
> +# Allow client connections for VNC, Spice and socket based
> +# character devices to be passed in to QEMU via SCM_RIGHTS.
> +#
> +# @protocol: protocol name. Valid names are "vnc", "spice" or the
> +#            name of a character device (eg. from -chardev id=XXXX)

Not this patch's fault, but here goes anyway: rotten design, breaks when
you name your character device "vnc" or "spice".  I think we shouldn't
overload commands like that.

> +#
> +# @fdname: file descriptor name previously passed via 'getfd' command
> +#
> +# @skipauth: #optional whether to skip authentication

Should we document that this applies only to vnc and spice?

> +#
> +# @tls: #optional whether to perform TLS

Should we document that this applies only to spice?

> +#
> +# Returns: nothing on success.
> +#
> +# Since: 0.14.0
> +##

Should we document that this always "uses up" @fdname, i.e. even when it
fails?

> +{ 'command': 'add_client',
> +  'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool',
> +            '*tls': 'bool' } }
> +
> +##
>  # @NameInfo:
>  #
>  # Guest name information.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 6e21ddb..36e08d9 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1231,10 +1231,7 @@ EQMP
>      {
>          .name       = "add_client",
>          .args_type  = "protocol:s,fdname:s,skipauth:b?,tls:b?",
> -        .params     = "protocol fdname skipauth tls",
> -        .help       = "add a graphics client",
> -        .user_print = monitor_user_noop,
> -        .mhandler.cmd_new = add_graphics_client,
> +        .mhandler.cmd_new = qmp_marshal_input_add_client,
>      },
>  
>  SQMP
> diff --git a/qmp.c b/qmp.c
> index 8463922..36c54c5 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -479,3 +479,46 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
>      return arch_query_cpu_definitions(errp);
>  }
>  
> +void qmp_add_client(const char *protocol, const char *fdname,
> +                    bool has_skipauth, bool skipauth, bool has_tls, bool tls,
> +                    Error **errp)
> +{
> +    CharDriverState *s;
> +    int fd;
> +
> +    fd = monitor_get_fd(cur_mon, fdname, errp);
> +    if (fd < 0) {
> +        return;
> +    }
> +
> +    if (strcmp(protocol, "spice") == 0) {
> +        if (!using_spice) {
> +            error_set(errp, QERR_DEVICE_NOT_ACTIVE, "spice");
> +            close(fd);
> +            return;
> +        }
> +        skipauth = has_skipauth ? skipauth : false;
> +        tls = has_tls ? tls : false;

Pattern "has_FOO ? FOO : DEFAULT".  Would it be feasible and useful to
declare the default in the schema, and pass only FOO to the function
then, not has_FOO?  Outside this patch's scope, of course.

> +        if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) {
> +            error_setg(errp, "spice failed to add client");

Error message could use some love, but anything is an improvement over
nothing.

> +            close(fd);
> +        }
> +        return;
> +#ifdef CONFIG_VNC
> +    } else if (strcmp(protocol, "vnc") == 0) {

I hate "return; else", but to each his own :)

> +        skipauth = has_skipauth ? skipauth : false;
> +        vnc_display_add_client(NULL, fd, skipauth);

Amazingly, this can't fail.  Wonder what happens when you pass a bogus
file descriptor...  Not this patch's fault.

> +        return;
> +#endif
> +    } else if ((s = qemu_chr_find(protocol)) != NULL) {
> +        if (qemu_chr_add_client(s, fd) < 0) {
> +            error_setg(errp, "failed to add client");

Error message could use some love, but it's no worse than before.

> +            close(fd);
> +            return;
> +        }
> +        return;
> +    }
> +
> +    error_setg(errp, "protocol '%s' is invalid", protocol);
> +    close(fd);
> +}

Just comments, no objections.