Re: [Qemu-devel] [PATCH 3/4] Add generic drive hotplugging

[Alexander Graf]

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