[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3] qapi: convert add_client
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 3/3] qapi: convert add_client |
Date: |
Tue, 25 Sep 2012 13:29:32 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Luiz Capitulino <address@hidden> 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 <address@hidden>
> ---
> 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.