From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47220) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qij24-0007rb-Vh for qemu-devel@nongnu.org; Mon, 18 Jul 2011 04:18:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Qij1y-0001mz-8k for qemu-devel@nongnu.org; Mon, 18 Jul 2011 04:18:28 -0400 Received: from cantor2.suse.de ([195.135.220.15]:49820 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Qij1x-0001mS-Mp for qemu-devel@nongnu.org; Mon, 18 Jul 2011 04:18:22 -0400 Mime-Version: 1.0 (Apple Message framework v1084) Content-Type: text/plain; charset=us-ascii From: Alexander Graf In-Reply-To: <4E1EDD5F.5080104@redhat.com> Date: Mon, 18 Jul 2011 10:18:18 +0200 Content-Transfer-Encoding: quoted-printable Message-Id: References: <1310455297-27436-1-git-send-email-agraf@suse.de> <1310455297-27436-4-git-send-email-agraf@suse.de> <4E1EDD5F.5080104@redhat.com> Subject: Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: "qemu-devel@nongnu.org Developers" , armbru@redhat.com On 14.07.2011, at 14:13, Kevin Wolf wrote: > 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. >>=20 >> 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. >>=20 >> Signed-off-by: Alexander Graf >>=20 >> --- >>=20 >> v1 -> v2: >>=20 >> - 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(-) >>=20 >> 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" >>=20 >> DriveInfo *add_init_drive(const char *optstr) >> { >> @@ -44,3 +47,51 @@ DriveInfo *add_init_drive(const char *optstr) >>=20 >> 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 >=20 > 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. Yes, I tried to keep it functionally the same. If any other targets want = to add PCI hotplug support, it's as simple as an #ifdef change. For now, = none does :). Next time someone adds PCI hotplug support for another = arch, we might want to consider cleaning this whole thing up a bit = though. >=20 >> + >> +/* >> + * 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. >> + */ >=20 > 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. >=20 > 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. Yep. Will just remove the comment. >=20 >> +void drive_hot_add(Monitor *mon, const QDict *qdict) >> +{ >> + int type; >> + DriveInfo *dinfo =3D NULL; >> + const char *opts =3D qdict_get_str(qdict, "opts"); >> + >> + dinfo =3D add_init_drive(opts); >> + if (!dinfo) { >> + goto err; >> + } >> + if (dinfo->devaddr) { >> + monitor_printf(mon, "Parameter addr not supported\n"); >> + goto err; >> + } >> + type =3D 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; >> } >>=20 >> -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 =3D NULL; >> const char *pci_addr =3D qdict_get_str(qdict, "pci_addr"); >> - const char *opts =3D qdict_get_str(qdict, "opts"); >> - >> - dinfo =3D add_init_drive(opts); >> - if (!dinfo) >> - goto err; >> - if (dinfo->devaddr) { >> - monitor_printf(mon, "Parameter addr not supported\n"); >> - goto err; >> - } >> - type =3D dinfo->type; >>=20 >> 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; >>=20 >> + return 0; >> err: >> - if (dinfo) >> - drive_put_ref(dinfo); >> - return; >> + return -1; >> } >=20 > Now that there isn't any error handling any more, "goto err;" could be > replaced by "return -1;". I actually changed it at first and then changed it back to the goto err = :). I find this more readable tbh as "err" somehow jumps into the eye = more easily than "return -1". And with such a huge amount of error = cases, I really wanted to stress them :). Alex