| Message ID | 1348065078-5139-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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;
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; >
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.
[snip] Ok, I'll drop this patch and do everything in a single patch adding Error *. Paolo
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;
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(-)