Luiz Capitulino <[email protected]> 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 <[email protected]> > --- > 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.
