| Message ID | 1310455297-27436-4-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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 <agraf@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. > + > +/* > + * 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
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 <agraf@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
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 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. + */ +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; } static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon, diff --git a/sysemu.h b/sysemu.h index d3013f5..9c8e50f 100644 --- a/sysemu.h +++ b/sysemu.h @@ -144,9 +144,13 @@ extern unsigned int nb_prom_envs; /* pci-hotplug */ void pci_device_hot_add(Monitor *mon, const QDict *qdict); -void drive_hot_add(Monitor *mon, const QDict *qdict); +int pci_drive_hot_add(Monitor *mon, const QDict *qdict, + DriveInfo *dinfo, int type); void do_pci_device_hot_remove(Monitor *mon, const QDict *qdict); +/* generic hotplug */ +void drive_hot_add(Monitor *mon, const QDict *qdict); + /* pcie aer error injection */ void pcie_aer_inject_error_print(Monitor *mon, const QObject *data); int do_pcie_aer_inejct_error(Monitor *mon,
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 <agraf@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(-)