From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:51201) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QhKkS-0007hT-Tz for qemu-devel@nongnu.org; Thu, 14 Jul 2011 08:10:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QhKkN-0006i2-UR for qemu-devel@nongnu.org; Thu, 14 Jul 2011 08:10:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49205) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QhKkN-0006hq-FX for qemu-devel@nongnu.org; Thu, 14 Jul 2011 08:10:27 -0400 Message-ID: <4E1EDD5F.5080104@redhat.com> Date: Thu, 14 Jul 2011 14:13:19 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1310455297-27436-1-git-send-email-agraf@suse.de> <1310455297-27436-4-git-send-email-agraf@suse.de> In-Reply-To: <1310455297-27436-4-git-send-email-agraf@suse.de> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: "qemu-devel@nongnu.org Developers" , armbru@redhat.com Am 12.07.2011 09:21, schrieb Alexander Graf: > The monitor command for hotplugging is in i386 specific code. This is just > plain wrong, as S390 just learned how to do hotplugging too and needs to > get drives for that. > > So let's add a generic copy to generic code that handles drive_add in a > way that doesn't have pci dependencies. All pci specific code can then > be handled in a pci specific function. > > Signed-off-by: Alexander Graf > > --- > > v1 -> v2: > > - align generic drive_add to pci specific one > - rework to split between generic and pci code > --- > hw/device-hotplug.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/pci-hotplug.c | 24 ++++-------------------- > sysemu.h | 6 +++++- > 3 files changed, 60 insertions(+), 21 deletions(-) > > diff --git a/hw/device-hotplug.c b/hw/device-hotplug.c > index 8b2ed7a..eb6dd0e 100644 > --- a/hw/device-hotplug.c > +++ b/hw/device-hotplug.c > @@ -26,6 +26,9 @@ > #include "boards.h" > #include "net.h" > #include "blockdev.h" > +#include "qemu-config.h" > +#include "sysemu.h" > +#include "monitor.h" > > DriveInfo *add_init_drive(const char *optstr) > { > @@ -44,3 +47,51 @@ DriveInfo *add_init_drive(const char *optstr) > > return dinfo; > } > + > +#if !defined(TARGET_I386) > +int pci_drive_hot_add(Monitor *mon, const QDict *qdict, > + DriveInfo *dinfo, int type) > +{ > + /* On non-x86 we don't do PCI hotplug */ > + monitor_printf(mon, "Can't hot-add drive to type %d\n", type); > + return -1; > +} > +#endif This assumes that only x86 can do PCI hotplug. If someone added it to another target, the error should be obvious enough, so I guess it's okay. > + > +/* > + * This version of drive_hot_add does not know anything about PCI specifics. > + * It is used as fallback on architectures that don't implement pci-hotplug. > + */ Maybe it was true in v1, don't know, but now it's not a fallback, but a common implementation that is used by both x86 and s390 and calls a more specific one in some cases. It also doesn't make too much sense to have a comment that says what a function is _not_. Without knowing the context of this patch, I probably wouldn't understand what the comment is trying to say. > +void drive_hot_add(Monitor *mon, const QDict *qdict) > +{ > + int type; > + DriveInfo *dinfo = NULL; > + const char *opts = qdict_get_str(qdict, "opts"); > + > + dinfo = add_init_drive(opts); > + if (!dinfo) { > + goto err; > + } > + if (dinfo->devaddr) { > + monitor_printf(mon, "Parameter addr not supported\n"); > + goto err; > + } > + type = dinfo->type; > + > + switch (type) { > + case IF_NONE: > + monitor_printf(mon, "OK\n"); > + break; > + default: > + if (pci_drive_hot_add(mon, qdict, dinfo, type)) { > + goto err; > + } > + } > + return; > + > +err: > + if (dinfo) { > + drive_put_ref(dinfo); > + } > + return; > +} > diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c > index b59be2a..f08d367 100644 > --- a/hw/pci-hotplug.c > +++ b/hw/pci-hotplug.c > @@ -103,24 +103,13 @@ static int scsi_hot_add(Monitor *mon, DeviceState *adapter, > return 0; > } > > -void drive_hot_add(Monitor *mon, const QDict *qdict) > +int pci_drive_hot_add(Monitor *mon, const QDict *qdict, > + DriveInfo *dinfo, int type) > { > int dom, pci_bus; > unsigned slot; > - int type; > PCIDevice *dev; > - DriveInfo *dinfo = NULL; > const char *pci_addr = qdict_get_str(qdict, "pci_addr"); > - const char *opts = qdict_get_str(qdict, "opts"); > - > - dinfo = add_init_drive(opts); > - if (!dinfo) > - goto err; > - if (dinfo->devaddr) { > - monitor_printf(mon, "Parameter addr not supported\n"); > - goto err; > - } > - type = dinfo->type; > > switch (type) { > case IF_SCSI: > @@ -137,19 +126,14 @@ void drive_hot_add(Monitor *mon, const QDict *qdict) > goto err; > } > break; > - case IF_NONE: > - monitor_printf(mon, "OK\n"); > - break; > default: > monitor_printf(mon, "Can't hot-add drive to type %d\n", type); > goto err; > } > - return; > > + return 0; > err: > - if (dinfo) > - drive_put_ref(dinfo); > - return; > + return -1; > } Now that there isn't any error handling any more, "goto err;" could be replaced by "return -1;". The general approach looks okay. Kevin