| Message ID | 20120918160652.48fe5baa@doriath.home |
|---|---|
| State | New |
| Headers | show |
On 09/18/2012 01:06 PM, Luiz Capitulino wrote: > 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 > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > monitor.c | 39 --------------------------------------- > qapi-schema.json | 23 +++++++++++++++++++++++ > qmp-commands.hx | 5 +---- > qmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 68 insertions(+), 43 deletions(-) > > { 'command': 'screendump', 'data': {'filename': 'str'} } > + > +## > +# @add_client Are these supposed to be sorted in any particular order? > +# > +# 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) > +# > +# @fdname: file descriptor name passed via SCM_RIGHTS Misleading; isn't this really: file descriptor name previously passed via 'getfd' command since it is only 'getfd' that uses SCM_RIGHTS? > +# > +# skipauth: #optional whether to skip authentication > +# > +# tls: #optional whether to perform TLS Missing leading @ on two lines. > +# > +# Returns: nothing on success. > +# > +# Since: 0.14.0 If this were a new command for 1.3, I'd say to name it 'add-client'; but since QMP has already been exposing it and you are now just documenting it, you can't change the name.
On Tue, 18 Sep 2012 13:13:16 -0600 Eric Blake <eblake@redhat.com> wrote: > On 09/18/2012 01:06 PM, Luiz Capitulino wrote: > > 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 > > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > monitor.c | 39 --------------------------------------- > > qapi-schema.json | 23 +++++++++++++++++++++++ > > qmp-commands.hx | 5 +---- > > qmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 68 insertions(+), 43 deletions(-) > > > > > { 'command': 'screendump', 'data': {'filename': 'str'} } > > + > > +## > > +# @add_client > > Are these supposed to be sorted in any particular order? Honestly, this is something I've paid very little attention to. But it's a good idea to keep the list sorted yes (although I'd like ErroClass to be the first item). > > > +# > > +# 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) > > +# > > +# @fdname: file descriptor name passed via SCM_RIGHTS > > Misleading; isn't this really: > > file descriptor name previously passed via 'getfd' command > > since it is only 'getfd' that uses SCM_RIGHTS? You're right, of course. Stupid mistake from my part :) > > +# > > +# skipauth: #optional whether to skip authentication > > +# > > +# tls: #optional whether to perform TLS > > Missing leading @ on two lines. Will fix. > > +# > > +# Returns: nothing on success. > > +# > > +# Since: 0.14.0 > > If this were a new command for 1.3, I'd say to name it 'add-client'; but > since QMP has already been exposing it and you are now just documenting > it, you can't change the name. Yes, we just have to live with that for all old commands.
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Tue, 18 Sep 2012 13:13:16 -0600 > Eric Blake <eblake@redhat.com> wrote: > >> On 09/18/2012 01:06 PM, Luiz Capitulino wrote: >> > 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 >> > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> >> > --- >> > monitor.c | 39 --------------------------------------- >> > qapi-schema.json | 23 +++++++++++++++++++++++ >> > qmp-commands.hx | 5 +---- >> > qmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> > 4 files changed, 68 insertions(+), 43 deletions(-) >> > >> >> > { 'command': 'screendump', 'data': {'filename': 'str'} } >> > + >> > +## >> > +# @add_client [...] >> If this were a new command for 1.3, I'd say to name it 'add-client'; but >> since QMP has already been exposing it and you are now just documenting >> it, you can't change the name. > > Yes, we just have to live with that for all old commands. If the inconsistency bothers us, we can either * add suitable aliases for every QMP name containing '_', or * fix the QMP names, and fold '_' to '-' in names received from client.
Il 18/09/2012 21:06, Luiz Capitulino ha scritto: > + fd = monitor_get_fd(cur_mon, fdname); > + if (fd < 0) { > + error_setg(errp, "file-description '%s' has not been found", fdname); > + return; > + } Please change this to add an Error * argument to monitor_get_fd, and change non-Error-friendly callers of monitor_get_fd to monitor_handle_fd_param. You can then reuse the error in monitor_handle_fd_param (there is already QERR_FD_NOT_FOUND, by the way). I'll post the patches later in my reviewed NBD series, please take them from there and I'll rebase before my pull request. Paolo
On Wed, 19 Sep 2012 09:08:30 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Tue, 18 Sep 2012 13:13:16 -0600 > > Eric Blake <eblake@redhat.com> wrote: > > > >> On 09/18/2012 01:06 PM, Luiz Capitulino wrote: > >> > 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 > >> > > >> > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > >> > --- > >> > monitor.c | 39 --------------------------------------- > >> > qapi-schema.json | 23 +++++++++++++++++++++++ > >> > qmp-commands.hx | 5 +---- > >> > qmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > >> > 4 files changed, 68 insertions(+), 43 deletions(-) > >> > > >> > >> > { 'command': 'screendump', 'data': {'filename': 'str'} } > >> > + > >> > +## > >> > +# @add_client > [...] > >> If this were a new command for 1.3, I'd say to name it 'add-client'; but > >> since QMP has already been exposing it and you are now just documenting > >> it, you can't change the name. > > > > Yes, we just have to live with that for all old commands. > > If the inconsistency bothers us, we can either > > * add suitable aliases for every QMP name containing '_', or > > * fix the QMP names, and fold '_' to '-' in names received from client. Agreed, maybe we could do both as some commands would profit from having aliases (cont, qmp_capabilities, etc).
On Wed, 19 Sep 2012 13:04:22 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 18/09/2012 21:06, Luiz Capitulino ha scritto: > > + fd = monitor_get_fd(cur_mon, fdname); > > + if (fd < 0) { > > + error_setg(errp, "file-description '%s' has not been found", fdname); > > + return; > > + } > > Please change this to add an Error * argument to monitor_get_fd, and > change non-Error-friendly callers of monitor_get_fd to > monitor_handle_fd_param. Good point. > You can then reuse the error in monitor_handle_fd_param (there is > already QERR_FD_NOT_FOUND, by the way). > > I'll post the patches later in my reviewed NBD series, please take them > from there and I'll rebase before my pull request. Ok, although I wouldn't mind having this patch on top of yours.
diff --git a/monitor.c b/monitor.c index 67064e2..07fee72 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); - 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); - 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); - 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..069906a 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -2619,3 +2619,26 @@ # Since: 0.14.0 ## { 'command': 'screendump', 'data': {'filename': 'str'} } + +## +# @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) +# +# @fdname: file descriptor name passed via SCM_RIGHTS +# +# skipauth: #optional whether to skip authentication +# +# tls: #optional whether to perform TLS +# +# Returns: nothing on success. +# +# Since: 0.14.0 +## +{ 'command': 'add_client', + 'data': { 'protocol': 'str', 'fdname': 'str', '*skipauth': 'bool', + '*tls': 'bool' } } 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..abec611 100644 --- a/qmp.c +++ b/qmp.c @@ -479,3 +479,47 @@ 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); + if (fd < 0) { + error_setg(errp, "file-description '%s' has not been found", fdname); + 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; + if (qemu_spice_display_add_client(fd, skipauth, tls) < 0) { + error_setg(errp, "spice failed to add client"); + close(fd); + } + return; +#ifdef CONFIG_VNC + } else if (strcmp(protocol, "vnc") == 0) { + skipauth = has_skipauth ? skipauth : false; + vnc_display_add_client(NULL, fd, skipauth); + 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"); + close(fd); + return; + } + return; + } + + error_setg(errp, "protocol '%s' is invalid", protocol); + close(fd); +}
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 Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- monitor.c | 39 --------------------------------------- qapi-schema.json | 23 +++++++++++++++++++++++ qmp-commands.hx | 5 +---- qmp.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 43 deletions(-)