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. >> >> 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 <ag...@suse.de> >> >> --- >> >> 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.
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. > >> + >> +/* >> + * 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. Yep. Will just remove the comment. > >> +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;". 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