[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging
From: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging |
Date: |
Mon, 18 Jul 2011 10:18:18 +0200 |
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 <address@hidden>
>>
>> ---
>>
>> 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