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

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

[Luiz Capitulino]

The first two patches work on adding an Error * argument to monitor_get_fd().
The second patch is the actual conversion of add_client.

v2

 o add Error * argument to monitor_get_fd()
 o fix schema doc mistakes

 dump.c              |  3 +--
 hw/kvm/pci-assign.c | 12 +++---------
 migration-fd.c      |  2 +-
 monitor.c           | 48 ++++++------------------------------------------
 monitor.h           |  2 +-
 qapi-schema.json    | 23 +++++++++++++++++++++++
 qmp-commands.hx     |  5 +----
 qmp.c               | 43 +++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 79 insertions(+), 59 deletions(-)

[Qemu-devel] [PATCH 1/3] pci-assign: use monitor_handle_fd_param

[Luiz Capitulino]

From: Paolo Bonzini <pbonzini@redhat.com>

There is no need to open-code the choice between a file descriptor
number or a named one.  Just use monitor_handle_fd_param, which
also takes care of printing the error message.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 hw/kvm/pci-assign.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
index 05b93d9..7a0998c 100644
--- a/hw/kvm/pci-assign.c
+++ b/hw/kvm/pci-assign.c
@@ -579,15 +579,9 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
     snprintf(name, sizeof(name), "%sconfig", dir);
 
     if (pci_dev->configfd_name && *pci_dev->configfd_name) {
-        if (qemu_isdigit(pci_dev->configfd_name[0])) {
-            dev->config_fd = strtol(pci_dev->configfd_name, NULL, 0);
-        } else {
-            dev->config_fd = monitor_get_fd(cur_mon, pci_dev->configfd_name);
-            if (dev->config_fd < 0) {
-                error_report("%s: (%s) unkown", __func__,
-                             pci_dev->configfd_name);
-                return 1;
-            }
+        dev->config_fd = monitor_handle_fd_param(cur_mon, pci_dev->configfd_name);
+        if (dev->config_fd < 0) {
+            return 1;
         }
     } else {
         dev->config_fd = open(name, O_RDWR);
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 2/3] monitor: add Error * argument to monitor_get_fd

[Luiz Capitulino]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 dump.c         |  3 +--
 migration-fd.c |  2 +-
 monitor.c      | 15 +++++++++------
 monitor.h      |  2 +-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/dump.c b/dump.c
index 2bf8d8d..1a3c716 100644
--- a/dump.c
+++ b/dump.c
@@ -836,9 +836,8 @@ void qmp_dump_guest_memory(bool paging, const char *file, bool has_begin,
 
 #if !defined(WIN32)
     if (strstart(file, "fd:", &p)) {
-        fd = monitor_get_fd(cur_mon, p);
+        fd = monitor_get_fd(cur_mon, p, errp);
         if (fd == -1) {
-            error_set(errp, QERR_FD_NOT_FOUND, p);
             return;
         }
     }
diff --git a/migration-fd.c b/migration-fd.c
index 50138ed..7335167 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -75,7 +75,7 @@ static int fd_close(MigrationState *s)
 
 int fd_start_outgoing_migration(MigrationState *s, const char *fdname)
 {
-    s->fd = monitor_get_fd(cur_mon, fdname);
+    s->fd = monitor_get_fd(cur_mon, fdname, NULL);
     if (s->fd == -1) {
         DPRINTF("fd_migration: invalid file descriptor identifier\n");
         goto err_after_get_fd;
diff --git a/monitor.c b/monitor.c
index 67064e2..645f8f4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -951,7 +951,7 @@ static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d
     CharDriverState *s;
 
     if (strcmp(protocol, "spice") == 0) {
-        int fd = monitor_get_fd(mon, fdname);
+        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) {
@@ -965,13 +965,13 @@ static int add_graphics_client(Monitor *mon, const QDict *qdict, QObject **ret_d
         return 0;
 #ifdef CONFIG_VNC
     } else if (strcmp(protocol, "vnc") == 0) {
-	int fd = monitor_get_fd(mon, fdname);
+	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);
+	int fd = monitor_get_fd(mon, fdname, NULL);
 	if (qemu_chr_add_client(s, fd) < 0) {
 	    qerror_report(QERR_ADD_CLIENT_FAILED);
 	    return -1;
@@ -2118,7 +2118,7 @@ static void do_loadvm(Monitor *mon, const QDict *qdict)
     }
 }
 
-int monitor_get_fd(Monitor *mon, const char *fdname)
+int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp)
 {
     mon_fd_t *monfd;
 
@@ -2139,6 +2139,7 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
         return fd;
     }
 
+    error_setg(errp, "File description named '%s' has not been found", fdname);
     return -1;
 }
 
@@ -2410,12 +2411,14 @@ int monitor_fdset_dup_fd_remove(int dup_fd)
 int monitor_handle_fd_param(Monitor *mon, const char *fdname)
 {
     int fd;
+    Error *local_err = NULL;
 
     if (!qemu_isdigit(fdname[0]) && mon) {
 
-        fd = monitor_get_fd(mon, fdname);
+        fd = monitor_get_fd(mon, fdname, &local_err);
         if (fd == -1) {
-            error_report("No file descriptor named %s found", fdname);
+            qerror_report_err(local_err);
+            error_free(local_err);
             return -1;
         }
     } else {
diff --git a/monitor.h b/monitor.h
index 64c1561..e240c3f 100644
--- a/monitor.h
+++ b/monitor.h
@@ -66,7 +66,7 @@ int monitor_read_block_device_key(Monitor *mon, const char *device,
                                   BlockDriverCompletionFunc *completion_cb,
                                   void *opaque);
 
-int monitor_get_fd(Monitor *mon, const char *fdname);
+int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
 int monitor_handle_fd_param(Monitor *mon, const char *fdname);
 
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
-- 
1.7.12.315.g682ce8b

[Qemu-devel] [PATCH 3/3] 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            | 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)
+#
+# @fdname: file descriptor name previously passed via 'getfd' command
+#
+# @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' } }
+
+##
 # @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..973beff 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 (error_is_set(errp)) {
+        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

[Qemu-devel] [PATCH 3/3] 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            | 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)
+#
+# @fdname: file descriptor name previously passed via 'getfd' command
+#
+# @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' } }
+
+##
 # @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;
+        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 3/3] qapi: convert add_client

[Markus Armbruster]

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.

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

[Luiz Capitulino]

On Tue, 25 Sep 2012 13:29:32 +0200
Markus Armbruster <armbru@redhat.com> wrote:

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

By "uses up" you mean that the fd will be consumed from the monitor's
poll? I guess that's true for every command that accepts fds.

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

Agreed, I think a better design would be to have separate commands.

> > +#
> > +# @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?

Yes, as I'll have to respin 2/3 anyway...

> > +{ '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.

I think so.

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

I actually tried to find information whether spice_server_add_ssl_client()
and spice_server_add_client() set errno on failure, but I remember I didn't
find any official doc. Maybe I didn't look for it correctly, will try again.

But in case no doc clearly states that we can read errno on failure, then
I'll prefer the generic error.

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

Today, this can't fail. The only chardev that defines this method
is the socket one and it can't fail.

> 
> > +            close(fd);
> > +            return;
> > +        }
> > +        return;
> > +    }
> > +
> > +    error_setg(errp, "protocol '%s' is invalid", protocol);
> > +    close(fd);
> > +}
> 
> Just comments, no objections.
> 

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Tue, 25 Sep 2012 13:29:32 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> 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.
>
> By "uses up" you mean that the fd will be consumed from the monitor's
> poll? I guess that's true for every command that accepts fds.

Yes, that's how these commands should work.  Before your patch,
add_graphics_client() doesn't call when protocol is invalid.  Your patch
fixes that.  Worth mentioning in the commit message.

[...]

[Qemu-devel] [PATCH 3/3] 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
 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 | 25 +++++++++++++++++++++++++
 qmp-commands.hx  |  5 +----
 qmp.c            | 43 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 43 deletions(-)

diff --git a/monitor.c b/monitor.c
index c24235e..c9f460a 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..191d921 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -33,6 +33,31 @@
             '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)
+#
+# @fdname: file descriptor name previously passed via 'getfd' command
+#
+# @skipauth: #optional whether to skip authentication. Only applies
+#            to "vnc" and "spice" protocols
+#
+# @tls: #optional whether to perform TLS. Only applies to the "spice"
+#       protocol
+#
+# Returns: nothing on success.
+#
+# Since: 0.14.0
+##
+{ '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;
+        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