Patchwork [01/12] monitor: use monitor_handle_fd_param for non-Error-friendly users of named fds

login
register
mail settings
Submitter Paolo Bonzini
Date Sept. 19, 2012, 2:31 p.m.
Message ID <1348065078-5139-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/185057/
State New
Headers show

Comments

Paolo Bonzini - Sept. 19, 2012, 2:31 p.m.
monitor_handle_fd_param and monitor_get_fd are mostly the same, except
that monitor_handle_fd_param does error reporting wrong.  Use it in all
other places that do it wrong, instead of reinventing it.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/kvm/pci-assign.c | 4 +---
 migration-fd.c      | 3 +--
 monitor.c           | 6 +++---
 3 file modificati, 5 inserzioni(+), 8 rimozioni(-)
Luiz Capitulino - Sept. 19, 2012, 8:42 p.m.
On Wed, 19 Sep 2012 16:31:04 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> monitor_handle_fd_param and monitor_get_fd are mostly the same, except
> that monitor_handle_fd_param does error reporting wrong.  Use it in all
> other places that do it wrong, instead of reinventing it.

Hmm, why do we want to do this?

As far as I understand it the main difference between the two functions
is that if fdname is a number (for a weak definition of number),
monitor_handle_fd_param() assumes that the fd already exists in qemu
(eg. it was passed by the parent process). 

I don't like much the idea of spreading its usage because if it's
used by a qmp command (and this patch does just that) then clients
using that command won't be able to set the first character of fdname
to a number, IOW it's an incompatible change (unless this doesn't work
today for some reason, but I believe it does).

Another side effect is that you add the possibility of functions
changing from monitor_get_fd() to monitor_handle_fd_param() to also take
fds passed by the parent process. Might be positive, but I wonder if that's
useful for the commands you're changing.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/kvm/pci-assign.c | 4 +---
>  migration-fd.c      | 3 +--
>  monitor.c           | 6 +++---
>  3 file modificati, 5 inserzioni(+), 8 rimozioni(-)
> 
> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
> index 05b93d9..8b96a57 100644
> --- a/hw/kvm/pci-assign.c
> +++ b/hw/kvm/pci-assign.c
> @@ -582,10 +582,8 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
>          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);
> +            dev->config_fd = monitor_handle_fd_param(cur_mon, pci_dev->configfd_name);
>              if (dev->config_fd < 0) {
> -                error_report("%s: (%s) unkown", __func__,
> -                             pci_dev->configfd_name);
>                  return 1;
>              }
>          }
> diff --git a/migration-fd.c b/migration-fd.c
> index 50138ed..3eb53d9 100644
> --- a/migration-fd.c
> +++ b/migration-fd.c
> @@ -75,9 +75,8 @@ 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_handle_fd_param(cur_mon, fdname);
>      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..4901600 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_handle_fd_param(mon, fdname);
>          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_handle_fd_param(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);
> +	int fd = monitor_handle_fd_param(mon, fdname);
>  	if (qemu_chr_add_client(s, fd) < 0) {
>  	    qerror_report(QERR_ADD_CLIENT_FAILED);
>  	    return -1;
Paolo Bonzini - Sept. 20, 2012, 8:09 a.m.
Il 19/09/2012 22:42, Luiz Capitulino ha scritto:
> On Wed, 19 Sep 2012 16:31:04 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> monitor_handle_fd_param and monitor_get_fd are mostly the same, except
>> that monitor_handle_fd_param does error reporting wrong.  Use it in all
>> other places that do it wrong, instead of reinventing it.
> 
> Hmm, why do we want to do this?
> 
> As far as I understand it the main difference between the two functions
> is that if fdname is a number (for a weak definition of number),
> monitor_handle_fd_param() assumes that the fd already exists in qemu
> (eg. it was passed by the parent process). 

Oops, right, I remembered it was the other way round (i.e. first search
for a named file descriptor, and fall back to a numeric one).

> Another side effect is that you add the possibility of functions
> changing from monitor_get_fd() to monitor_handle_fd_param() to also take
> fds passed by the parent process. Might be positive, but I wonder if that's
> useful for the commands you're changing.

Making behavior more uniform may be a useful thing on its own.  We might
even consider moving it to monitor_get_fd (with the above tweak for
compatibility).  BTW, pci-assign.c is open-coding
monitor_handle_fd_param (including the numeric file descriptor behavior)
and we can remove the surrounding if.

Paolo

>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/kvm/pci-assign.c | 4 +---
>>  migration-fd.c      | 3 +--
>>  monitor.c           | 6 +++---
>>  3 file modificati, 5 inserzioni(+), 8 rimozioni(-)
>>
>> diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
>> index 05b93d9..8b96a57 100644
>> --- a/hw/kvm/pci-assign.c
>> +++ b/hw/kvm/pci-assign.c
>> @@ -582,10 +582,8 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
>>          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);
>> +            dev->config_fd = monitor_handle_fd_param(cur_mon, pci_dev->configfd_name);
>>              if (dev->config_fd < 0) {
>> -                error_report("%s: (%s) unkown", __func__,
>> -                             pci_dev->configfd_name);
>>                  return 1;
>>              }
>>          }
>> diff --git a/migration-fd.c b/migration-fd.c
>> index 50138ed..3eb53d9 100644
>> --- a/migration-fd.c
>> +++ b/migration-fd.c
>> @@ -75,9 +75,8 @@ 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_handle_fd_param(cur_mon, fdname);
>>      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..4901600 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_handle_fd_param(mon, fdname);
>>          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_handle_fd_param(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);
>> +	int fd = monitor_handle_fd_param(mon, fdname);
>>  	if (qemu_chr_add_client(s, fd) < 0) {
>>  	    qerror_report(QERR_ADD_CLIENT_FAILED);
>>  	    return -1;
>
Luiz Capitulino - Sept. 20, 2012, 1:47 p.m.
On Thu, 20 Sep 2012 10:09:54 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 19/09/2012 22:42, Luiz Capitulino ha scritto:
> > On Wed, 19 Sep 2012 16:31:04 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> monitor_handle_fd_param and monitor_get_fd are mostly the same, except
> >> that monitor_handle_fd_param does error reporting wrong.  Use it in all
> >> other places that do it wrong, instead of reinventing it.
> > 
> > Hmm, why do we want to do this?
> > 
> > As far as I understand it the main difference between the two functions
> > is that if fdname is a number (for a weak definition of number),
> > monitor_handle_fd_param() assumes that the fd already exists in qemu
> > (eg. it was passed by the parent process). 
> 
> Oops, right, I remembered it was the other way round (i.e. first search
> for a named file descriptor, and fall back to a numeric one).

That's a lot better, IMO.

> > Another side effect is that you add the possibility of functions
> > changing from monitor_get_fd() to monitor_handle_fd_param() to also take
> > fds passed by the parent process. Might be positive, but I wonder if that's
> > useful for the commands you're changing.
> 
> Making behavior more uniform may be a useful thing on its own.  We might
> even consider moving it to monitor_get_fd (with the above tweak for
> compatibility).

Yes, although I can think of another issue: suppose an mngt app passes an
fd with fdname=9, but the fd doesn't reach qemu for some reason. Then the
mngt app executes a qmp command with fdname=9 and fd 9 turns out to exist
in qemu... Actually, a mngt app could do this even w/o passing an fd to
qemu.

Not sure how relevant this issue is though, as this can happen today
with qmp commands using monitor_handle_fd_param().

> BTW, pci-assign.c is open-coding
> monitor_handle_fd_param (including the numeric file descriptor behavior)
> and we can remove the surrounding if.

Yes, that's fine.
Paolo Bonzini - Sept. 20, 2012, 2:33 p.m.
[snip]

Ok, I'll drop this patch and do everything in a single patch adding Error *.

Paolo

Patch

diff --git a/hw/kvm/pci-assign.c b/hw/kvm/pci-assign.c
index 05b93d9..8b96a57 100644
--- a/hw/kvm/pci-assign.c
+++ b/hw/kvm/pci-assign.c
@@ -582,10 +582,8 @@  static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
         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);
+            dev->config_fd = monitor_handle_fd_param(cur_mon, pci_dev->configfd_name);
             if (dev->config_fd < 0) {
-                error_report("%s: (%s) unkown", __func__,
-                             pci_dev->configfd_name);
                 return 1;
             }
         }
diff --git a/migration-fd.c b/migration-fd.c
index 50138ed..3eb53d9 100644
--- a/migration-fd.c
+++ b/migration-fd.c
@@ -75,9 +75,8 @@  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_handle_fd_param(cur_mon, fdname);
     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..4901600 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_handle_fd_param(mon, fdname);
         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_handle_fd_param(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);
+	int fd = monitor_handle_fd_param(mon, fdname);
 	if (qemu_chr_add_client(s, fd) < 0) {
 	    qerror_report(QERR_ADD_CLIENT_FAILED);
 	    return -1;