From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:51046) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TER5Y-00014P-3w for qemu-devel@nongnu.org; Wed, 19 Sep 2012 16:41:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TER5W-0001Ky-Sr for qemu-devel@nongnu.org; Wed, 19 Sep 2012 16:41:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:64844) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TER5W-0001Ks-JQ for qemu-devel@nongnu.org; Wed, 19 Sep 2012 16:41:38 -0400 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q8JKfbYf023691 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 19 Sep 2012 16:41:37 -0400 Date: Wed, 19 Sep 2012 17:42:29 -0300 From: Luiz Capitulino Message-ID: <20120919174229.28597314@doriath.home> In-Reply-To: <1348065078-5139-2-git-send-email-pbonzini@redhat.com> References: <1348065078-5139-1-git-send-email-pbonzini@redhat.com> <1348065078-5139-2-git-send-email-pbonzini@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 01/12] monitor: use monitor_handle_fd_param for non-Error-friendly users of named fds List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: qemu-devel@nongnu.org On Wed, 19 Sep 2012 16:31:04 +0200 Paolo Bonzini 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 > --- > 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;