[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 v3 0/3] qapi: convert add_client

[Luiz Capitulino]

v3

 - qmp_add_client(): don't use errp to test for error

Luiz Capitulino (1):
  qapi: convert add_client

Paolo Bonzini (2):
  pci-assign: use monitor_handle_fd_param
  monitor: add Error * argument to monitor_get_fd

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

-- 
1.7.12.315.g682ce8b

[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

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

[Markus Armbruster]

Luiz Capitulino <lcapitulino@redhat.com> writes:

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

Silent change: no longer accepts file descriptors in octal and hex.

Silent fix: now rejects junk after numeric file descriptor.  

Both are fine with me, but perhaps worth mentioning in the commit
message.

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
>
>> 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);
>
> Silent change: no longer accepts file descriptors in octal and hex.
>
> Silent fix: now rejects junk after numeric file descriptor.  
>
> Both are fine with me, but perhaps worth mentioning in the commit
> message.

One more thought: this parses qdev "kvm-pci-assign" property "configfd".
A real "fd" property (qdev_prop_fd & friends) would be cleaner, but as
long as this is the only instance, it's not worth the trouble.

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

[Luiz Capitulino]

v4

 - Fix misworded error message in monitor_get_fd() [Markus]
 - small doc & commit log improvements [Markus]

Luiz Capitulino (1):
  qapi: convert add_client

Paolo Bonzini (2):
  pci-assign: use monitor_handle_fd_param
  monitor: add Error * argument to monitor_get_fd

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

-- 
1.7.12.315.g682ce8b

[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