On Wed, 19 Sep 2012 16:31:04 +0200
Paolo Bonzini <pbonz...@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 <pbonz...@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;
