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.
> +
> +/*
> + * 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
