[PATCH 1/2] hyperv: XML parsing of storage volumes

[Laine Stump]

On 11/19/20 8:38 AM, Daniel P. Berrangé wrote:
> On Wed, Nov 18, 2020 at 01:48:24PM -0500, Matt Coleman wrote:
>> dumpxml can now serialize:
>> * floppy drives
>> * file-backed and device-backed disk drives
>> * images mounted to virtual CD/DVD drives
>> * IDE and SCSI controllers
>>
>> Co-authored-by: Sri Ramanujam <sramanujam at datto.com>
>> Signed-off-by: Matt Coleman <matt at datto.com>
>> ---
>>   src/hyperv/hyperv_driver.c            | 408 +++++++++++++++++++++++++-
>>   src/hyperv/hyperv_driver.h            |   3 +
>>   src/hyperv/hyperv_wmi.c               |  45 +++
>>   src/hyperv/hyperv_wmi.h               |   8 +
>>   src/hyperv/hyperv_wmi_classes.h       |  19 ++
>>   src/hyperv/hyperv_wmi_generator.input | 134 +++++++++
>>   6 files changed, 616 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/hyperv/hyperv_driver.c b/src/hyperv/hyperv_driver.c
>> index 40739595ac..ce9ba2a02a 100644
>> --- a/src/hyperv/hyperv_driver.c
>> +++ b/src/hyperv/hyperv_driver.c
>> @@ -293,6 +293,385 @@ hypervCapsInit(hypervPrivate *priv)
>>       return NULL;
>>   }
>>   
>> +/*
>> + * Virtual device functions
>> + */
>> +static int
>> +hypervGetDeviceParentRasdFromDeviceId(const char *parentDeviceId,
>> +                                      Msvm_ResourceAllocationSettingData *list,
>> +                                      Msvm_ResourceAllocationSettingData **out)
>> +{
>> +    Msvm_ResourceAllocationSettingData *entry = list;
>> +    g_autofree char *escapedDeviceId = NULL;
>> +    *out = NULL;
>> +
>> +    while (entry) {
>> +        escapedDeviceId = g_strdup_printf("%s\"", entry->data->InstanceID);
>> +        escapedDeviceId = virStringReplace(escapedDeviceId, "\\", "\\\\");
> This leaks the original escapedDeviceId pointer.  The autofree only
> runs when variables go out of scope, not when you overwrite pointers.
>
> It'll also leak on every loop iteration.
>
>> +
>> +        if (g_str_has_suffix(parentDeviceId, escapedDeviceId)) {
>> +            *out = entry;
>> +            break;
>> +        }
>> +
>> +        entry = entry->next;
>> +    }
>> +
>> +    if (*out)
>> +        return 0;
>> +
>> +    return -1;
>> +}
>
>> diff --git a/src/hyperv/hyperv_driver.h b/src/hyperv/hyperv_driver.h
>> index 8099b5714b..e49550a2c8 100644
>> --- a/src/hyperv/hyperv_driver.h
>> +++ b/src/hyperv/hyperv_driver.h
>> @@ -22,4 +22,7 @@
>>   
>>   #pragma once
>>   
>> +#define HYPERV_MAX_SCSI_CONTROLLERS 4
>> +#define HYPERV_MAX_IDE_CONTROLLERS 2
> Is it really possible to have 2 IDE controllers ?


I know nothing about hyperv, but qemu does allow multiple IDE 
controllers, and I even once wrote patches to support it, but then we 
(or maybe it was just "me"?) decided that allowing lots of IDE disks 
would just be encouraging people to use the oldest, slowest type of disk 
in large configurations that would most benefit from going to the 
trouble of getting the proper virtio drivers installed in the guest.


So in the end, the qemu driver in libvirt only supports a single IDE 
controller, and only if it is built into the basic machinetype. That 
won't prevent any other driver from supporting multiple IDE controllers, 
but I would think twice about doing it.


> Most cases there is a single IDE controller, with two channels,
> and each channel has two devices, giving a total of 4 disks.
>
> I'm just wondering if the code is mistakenly creating separate
> libvirt controllers for the 2 IDE channels