[Qemu-devel] [PATCH] qapi: convert add_client

[Qemu-devel] [PATCH] qapi: convert add_client

[Luiz Capitulino]

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(-)

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);
+}
-- 
1.7.12.315.g682ce8b

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1919 bytes --]

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.

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

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

[Luiz Capitulino]

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.

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

[Markus Armbruster]

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.

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

[Paolo Bonzini]

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

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

[Luiz Capitulino]

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).

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

[Luiz Capitulino]

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.