On 10/09/2017 04:49 PM, Dawid Zamirski wrote:

From: Dawid Zamirski <dzamirski@datto.com>

When registering a VM we call OpenMedium on each disk image which adds it to vbox's global media registry. Therefore, we should make sure to call Close when unregistering VM so we cleanup the media registry entries after ourselves - this does not remove disk image files. This follows the behaviour of the VBoxManage unregistervm command. --- src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)

I'm not a vbox expert by any stretch, looking at this mostly because it's been sitting on list unreviewed...

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index dffeabde0..ac3b8fa00 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, IMachine **machine) { nsresult rc; vboxArray media = VBOX_ARRAY_INITIALIZER; + size_t i; + rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid->value, machine); if (NS_FAILED(rc)) { virReportError(VIR_ERR_NO_DOMAIN, "%s", @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data, vboxIID *iid, IMachine **machine) return rc; }

- /* We're not interested in the array returned by the Unregister method, - * but in the side effect of unregistering the virtual machine. In order - * to call the Unregister method correctly we need to use the vboxArray - * wrapper here. */

I think you should keep the second sentence here - "In order to call..." - IDC either way, but that would seem to still be important.

rc = vboxArrayGetWithUintArg(&media, *machine, (*machine)->vtbl->Unregister, - CleanupMode_DetachAllReturnNone); + CleanupMode_DetachAllReturnHardDisksOnly); + + if (NS_FAILED(rc)) + goto cleanup; + + /* close each medium attached to VM to remove from media registry */ + for (i = 0; i < media.count; i++) { + IMedium *medium = media.items[i]; + + if (!medium) + continue; +

So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go through some great pain to determine whether it's a "fake" disk or not - would this code need the same kind of logic? /me wonders how much the existing code could be made more common - beyond the "isCurrent" in the latter function the code appears to be the same. If this code needs it, then it might be worth the effort to pass 'isCurrent' to some common helper and for the other caller, just pass true instead... Which would be similar if this code needed that.

+ /* it's ok to ignore failure here - e.g. it may be used by another VM */ + medium->vtbl->Close(medium);

You could place an ignore_value(); around this. Again see the two API's above they have a detailed explanation. John

+ } + + cleanup: vboxArrayUnalloc(&media); return rc; }

On 10/17/2017 05:28 PM, Dawid Zamirski wrote:
> On Tue, 2017-10-17 at 15:44 -0400, John Ferlan wrote:
>>
>> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
>>> From: Dawid Zamirski <dzamirski@datto.com>
>>>
>>> When registering a VM we call OpenMedium on each disk image which
>>> adds it
>>> to vbox's global media registry. Therefore, we should make sure to
>>> call
>>> Close when unregistering VM so we cleanup the media registry
>>> entries
>>> after ourselves - this does not remove disk image files. This
>>> follows
>>> the behaviour of the VBoxManage unregistervm command.
>>> ---
>>>  src/vbox/vbox_tmpl.c | 24 +++++++++++++++++++-----
>>>  1 file changed, 19 insertions(+), 5 deletions(-)
>>>
>>
>> I'm not a vbox expert by any stretch, looking at this mostly because
>> it's been sitting on list unreviewed...
> 
> Thank you :-)
> 
>>
>>> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
>>> index dffeabde0..ac3b8fa00 100644
>>> --- a/src/vbox/vbox_tmpl.c
>>> +++ b/src/vbox/vbox_tmpl.c
>>> @@ -378,6 +378,8 @@ _unregisterMachine(vboxDriverPtr data, vboxIID
>>> *iid, IMachine **machine)
>>>  {
>>>      nsresult rc;
>>>      vboxArray media = VBOX_ARRAY_INITIALIZER;
>>> +    size_t i;
>>> +
>>>      rc = data->vboxObj->vtbl->FindMachine(data->vboxObj, iid-
>>>> value, machine);
>>>      if (NS_FAILED(rc)) {
>>>          virReportError(VIR_ERR_NO_DOMAIN, "%s",
>>> @@ -385,12 +387,24 @@ _unregisterMachine(vboxDriverPtr data,
>>> vboxIID *iid, IMachine **machine)
>>>          return rc;
>>>      }
>>>  
>>> -    /* We're not interested in the array returned by the
>>> Unregister method,
>>> -     * but in the side effect of unregistering the virtual
>>> machine. In order
>>> -     * to call the Unregister method correctly we need to use the
>>> vboxArray
>>> -     * wrapper here. */
>>
>> I think you should keep the second sentence here - "In order to
>> call..."
>>  - IDC either way, but that would seem to still be important.
>>
> 
> Since this patch adds a code that loops over this array to close each
> IMedium instance it's self-documenting so no there's need for code
> comments here.
> 

That's fine - for you I'm assuming it's obvious why the wrapper is
needed to call Unregister, but for me - less so.

>>>      rc = vboxArrayGetWithUintArg(&media, *machine, (*machine)-
>>>> vtbl->Unregister,
>>> -                                 CleanupMode_DetachAllReturnNone);
>>> +                                 CleanupMode_DetachAllReturnHardDi
>>> sksOnly);
>>> +
>>> +    if (NS_FAILED(rc))
>>> +        goto cleanup;
>>> +
>>> +    /* close each medium attached to VM to remove from media
>>> registry */
>>> +    for (i = 0; i < media.count; i++) {
>>> +        IMedium *medium = media.items[i];
>>> +
>>> +        if (!medium)
>>> +            continue;
>>> +
>>
>> So the vboxSnapshotRedefine and vboxDomainSnapshotDeleteMetadataOnly
>> APIs that use CleanupMode_DetachAllReturnHardDisksOnly seem to go
>> through some great pain to determine whether it's a "fake" disk or
>> not -
>> would this code need the same kind of logic?
>>
>> /me wonders how much the existing code could be made more common -
>> beyond the "isCurrent" in the latter function the code appears to be
>> the
>> same. If this code needs it, then it might be worth the effort to
>> pass
>> 'isCurrent' to some common helper and for the other caller, just pass
>> true instead... Which would be similar if this code needed that.
> 
> I intentionally tried to stay away from touching the snapshot related
> functions even though it seems that they could share some code. Since
> I've been working on this on and off between projects at work, at
> initial stages it seemed like a couple of lines of code "here and
> there" would let me set/change vbox storage controller models via
> libvirtxml. Unfortunately, as I dug deeper and tested all sorts of
> combinations more bugs (even in master branch) seemed to pop out and
> the series grew to be much bigger than I originally intended. However,
> since I'll have to reorganize the commits for V2 anyway, I'll take a
> look at those functions and see what I can do.
> 

I try to avoid snapshot too ;-)  I understand how making adjustments in
libvirt can be like peeling back layers of an onion that cause you to
cry each time you have to enter some other piece of code because either
you discover some new bug/issue or what you're doing affects some other
piece of code. That is though one reason why we encourage using "small
enough" functionality changing/fixing patches rather than some massive
patch doing many different things. Another reason for smaller patches
has to do with git bisect and being able to determine which patch broke
some functionality - you bisect back to some 800 line patch bomb and
attempt to figure out what caused the failure...  Many times what I find
myself doing when I trip across some existing issue is stashing work and
rebasing to the patch before I've stashed, generating a patch for the
issue I uncover and then dealing with the merge conflicts after that.

John

>>
>>> +        /* it's ok to ignore failure here - e.g. it may be used by
>>> another VM */
>>> +        medium->vtbl->Close(medium);
>>
>> You could place an ignore_value(); around this. Again see the two
>> API's
>> above they have a detailed explanation.
>>
>>
>> John
>>
>>> +    }
>>> +
>>> + cleanup:
>>>      vboxArrayUnalloc(&media);
>>>      return rc;
>>>  }
>>>
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:

From: Dawid Zamirski <dzamirski@datto.com>

The optional values are 'piix3', 'piix4' or 'ich6'. Those will be needed to allow setting IDE controller model in VirtualBox driver. --- docs/schemas/domaincommon.rng | 18 ++++++++++++++++-- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 2 ++ 4 files changed, 36 insertions(+), 2 deletions(-)

Patches 2 & 3 should be combined and you'll need to provide an xml2xml example here, modifying tests/qemuxml2xmltest.c in order to add your test. Search for controller type='scsi' and model= being set in any of the tests/*/*.xml files. Then generate your test that would have controller type='ide' ... model='xxx' (just one example is fine). You may also need to alter virDomainControllerDefValidate to ensure perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE aren't lost if ->model is filled in. I am far from being an expert on IDE controllers and their existing assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE references you will find the existing ones. My assumption has been that the current model assumption is piix3 - so by adding piix4 and ich6 you'll need to alter code in order to handle any assumptions those models have; otherwise, once code finds IDE it assumes piix3 and those assumptions may not work well for piix4 and ich6.

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4dbda6932..c3f1557f0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1927,12 +1927,11 @@ <ref name="address"/> </optional> <choice> - <!-- fdc/ide/sata/ccid have only the common attributes --> + <!-- fdc/sata/ccid have only the common attributes --> <group> <attribute name="type"> <choice> <value>fdc</value> - <value>ide</value> <value>sata</value> <value>ccid</value> </choice> @@ -1993,6 +1992,21 @@ </attribute> </optional> </group> + <!-- ide has an optional attribute "model" --> + <group> + <attribute name="type"> + <value>ide</value> + </attribute> + <optional> + <attribute name="model"> + <choice> + <value>piix3</value> + <value>piix4</value> + <value>ich6</value> + </choice> + </attribute> + </optional> + </group> <!-- pci has an optional attribute "model" --> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54be9028d..493bf83ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, "qemu-xhci", "none")

+VIR_ENUM_IMPL(virDomainControllerModelIDE, VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST, + "piix3", + "piix4", + "ich6") + VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "mount", "block", @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def, return virDomainControllerModelUSBTypeFromString(model); else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeFromString(model); + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + return virDomainControllerModelIDETypeFromString(model);

return -1; } @@ -9482,6 +9489,8 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def, return virDomainControllerModelUSBTypeToString(model); else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeToString(model); + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + return virDomainControllerModelIDETypeToString(model);

return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a42efcfa6..d7f4c3f1e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -748,6 +748,14 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST } virDomainControllerModelUSB;

+typedef enum { + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3, + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4, + VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6, + + VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST +} virDomainControllerModelIDE; + # define IS_USB2_CONTROLLER(ctrl) \ (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \ ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \ @@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI) VIR_ENUM_DECL(virDomainControllerPCIModelName) VIR_ENUM_DECL(virDomainControllerModelSCSI) VIR_ENUM_DECL(virDomainControllerModelUSB) +VIR_ENUM_DECL(virDomainControllerModelIDE) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainFSDriver) VIR_ENUM_DECL(virDomainFSAccessMode) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9243c5591..616b14f82 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex; virDomainControllerInsert; virDomainControllerInsertPreAlloced; virDomainControllerIsPSeriesPHB; +virDomainControllerModelIDETypeFromString; +virDomainControllerModelIDETypeToString; virDomainControllerModelPCITypeToString; virDomainControllerModelSCSITypeFromString; virDomainControllerModelSCSITypeToString;

"Currently" you probably don't need to export the *IDEType{To|From}* functions since domain_conf is the only consumer; however, seeing how the *SCSIType{To|From}* has multiple/external consumers makes me wonder if more consumers are needed for IDE. In particular, in qemuBuildControllerDevStr and some assumptions in src/qemu/qemu_domain_address.c related to where controllers live on the bus for piix3 (FWIW: This was written before the above last two paragraphs - it's what prompted me to consider the needs). John

On 10/18/2017 12:23 PM, Dawid Zamirski wrote:

On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:

...

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:

...

From: Dawid Zamirski <dzamirski@datto.com>

The optional values are 'piix3', 'piix4' or 'ich6'. Those will be needed to allow setting IDE controller model in VirtualBox driver. --- docs/schemas/domaincommon.rng | 18 ++++++++++++++++-- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 2 ++ 4 files changed, 36 insertions(+), 2 deletions(-)

Patches 2 & 3 should be combined and you'll need to provide an xml2xml example here, modifying tests/qemuxml2xmltest.c in order to add your test. Search for controller type='scsi' and model= being set in any of the tests/*/*.xml files. Then generate your test that would have controller type='ide' ... model='xxx' (just one example is fine).

I understand that the patch with test cases should be separated, corrent?

Hmmm.... Typically when one makes .rng and domain_conf changes, then I expect to also see *.html.in and tests/qemuxml2xmltest.c changes as well as including the new .xml files in the input/output directories (tests/qemuxml2argvdata and tests/qemuxml2xmloutdata). But this isn't qemu.... So I guess I'm too used to reviewing qemu changes! Since you're adding this for vbox and there isn't the same type xml validation test for that (I see only tests/vboxsnapshotxmltest.c) that just means you have less to do now. BTW: You will note there are bhyvexml2xmltest and lxcxml2xmltests, but nothing for vbox, so I guess it's a moot point since there's no way to test. Hey it is something to add some day in the future if you or someone else is so inclined. FWIW: If you made qemu driver changes, such as qemu_command.c - then one would change the qemuxml2argvtest.c and create a .args output file.

...

You may also need to alter virDomainControllerDefValidate to ensure perhaps some existing assumptions on VIR_DOMAIN_CONTROLLER_TYPE_IDE aren't lost if ->model is filled in.

That's clear, no problem...

Of course perhaps a moot point too ...

...

I am far from being an expert on IDE controllers and their existing assumptions, but if you search on VIR_DOMAIN_CONTROLLER_TYPE_IDE references you will find the existing ones. My assumption has been that the current model assumption is piix3 - so by adding piix4 and ich6 you'll need to alter code in order to handle any assumptions those models have; otherwise, once code finds IDE it assumes piix3 and those assumptions may not work well for piix4 and ich6.

...though, I'm a little concerned with this part. While I'm somewhat familiar with libvirt qemu driver and can confirm that it implies PIIX3 for IDE (at least libvirt does by checking if emulated machine is 440FX although qemu itself seems to support PIIX4 as well [1]), I'm not so sure I could easily determine "defaults" for other drivers like ESX (especially this one being closed-source) - AFAIK those drivers basically allow to attach "an IDE controller" without any specifics on what particular model of the southbridge the hypervisor should emulate - I guess it's likely determined by the "machine" type or hwVersion (ESX) and I suppose vbox is an "oddball" here allowing to set IDE controller model independently. Would it be acceptable to update each driver to check that ->model == -1 and error out if it's not? In other words, qemu would allow model {-1,piix3}, vbox {-1,piix3,piix4,ich6} everything else would accept just -1 - guess those could be enforced by implementing virDomainDeviceDefValidateCallback in each driver.

[1] https://github.com/qemu/qemu/blob/master/hw/ide/piix.c#L285

Well, again, it's my qemu review bias clearly showing through. The qemu code will know nothing about models so it'll just happily ignore the model type (at least for now until someday someone decides differently). So, how to handle this? Hmmm... Well it seems there's perhaps nothing to do unless there is some need/desire to ensure the <address> has specific values in which case you'd probably need that device def callback specific to vbox similarly to the qemu device def callback. One other thought - thinking about patch3 and formatdomain.html.in - you should indicate there that model types are only supported and handled for the vbox hypervisor only - there are other examples of that already. For example, <dd><span class="since">Since 3.9.0</span> for the vbox driver, the <code>ide</code> controller has an optional attribute <code>model</code>, which is one of "piix3", "piix4", or "ich6".</dd> I assume if the model='xxx' isn't provided, then some default is chosen. Sorry for the confusion - John

...

...

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4dbda6932..c3f1557f0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1927,12 +1927,11 @@ <ref name="address"/> </optional> <choice> - <!-- fdc/ide/sata/ccid have only the common attributes --> + <!-- fdc/sata/ccid have only the common attributes --> <group> <attribute name="type"> <choice> <value>fdc</value> - <value>ide</value> <value>sata</value> <value>ccid</value> </choice> @@ -1993,6 +1992,21 @@ </attribute> </optional> </group> + <!-- ide has an optional attribute "model" --> + <group> + <attribute name="type"> + <value>ide</value> + </attribute> + <optional> + <attribute name="model"> + <choice> + <value>piix3</value> + <value>piix4</value> + <value>ich6</value> + </choice> + </attribute> + </optional> + </group> <!-- pci has an optional attribute "model" --> <group> <attribute name="type"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 54be9028d..493bf83ff 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -378,6 +378,11 @@ VIR_ENUM_IMPL(virDomainControllerModelUSB, VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST, "qemu-xhci", "none")

+VIR_ENUM_IMPL(virDomainControllerModelIDE, VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST, + "piix3", + "piix4", + "ich6") + VIR_ENUM_IMPL(virDomainFS, VIR_DOMAIN_FS_TYPE_LAST, "mount", "block", @@ -9467,6 +9472,8 @@ virDomainControllerModelTypeFromString(const virDomainControllerDef *def, return virDomainControllerModelUSBTypeFromString(model); else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeFromString(model); + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + return virDomainControllerModelIDETypeFromString(model);

return -1; } @@ -9482,6 +9489,8 @@ virDomainControllerModelTypeToString(virDomainControllerDefPtr def, return virDomainControllerModelUSBTypeToString(model); else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) return virDomainControllerModelPCITypeToString(model); + else if (def->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) + return virDomainControllerModelIDETypeToString(model);

return NULL; } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index a42efcfa6..d7f4c3f1e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -748,6 +748,14 @@ typedef enum { VIR_DOMAIN_CONTROLLER_MODEL_USB_LAST } virDomainControllerModelUSB;

+typedef enum { + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3, + VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4, + VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6, + + VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST +} virDomainControllerModelIDE; + # define IS_USB2_CONTROLLER(ctrl) \ (((ctrl)->type == VIR_DOMAIN_CONTROLLER_TYPE_USB) && \ ((ctrl)->model == VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1 || \ @@ -3231,6 +3239,7 @@ VIR_ENUM_DECL(virDomainControllerModelPCI) VIR_ENUM_DECL(virDomainControllerPCIModelName) VIR_ENUM_DECL(virDomainControllerModelSCSI) VIR_ENUM_DECL(virDomainControllerModelUSB) +VIR_ENUM_DECL(virDomainControllerModelIDE) VIR_ENUM_DECL(virDomainFS) VIR_ENUM_DECL(virDomainFSDriver) VIR_ENUM_DECL(virDomainFSAccessMode) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9243c5591..616b14f82 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -231,6 +231,8 @@ virDomainControllerFindUnusedIndex; virDomainControllerInsert; virDomainControllerInsertPreAlloced; virDomainControllerIsPSeriesPHB; +virDomainControllerModelIDETypeFromString; +virDomainControllerModelIDETypeToString; virDomainControllerModelPCITypeToString; virDomainControllerModelSCSITypeFromString; virDomainControllerModelSCSITypeToString;

"Currently" you probably don't need to export the *IDEType{To|From}* functions since domain_conf is the only consumer; however, seeing how the *SCSIType{To|From}* has multiple/external consumers makes me wonder if more consumers are needed for IDE.

In particular, in qemuBuildControllerDevStr and some assumptions in src/qemu/qemu_domain_address.c related to where controllers live on the bus for piix3 (FWIW: This was written before the above last two paragraphs - it's what prompted me to consider the needs).

John

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
> 
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > From: Dawid Zamirski <dzamirski@datto.com>
> > 
> Lots of potentially use DEBUG information being lost. IDC, but seems
> a
> shame to not have it. Although it can be overkill when DEBUG is
> enabled
> - it does help sometimes in debugging problems. Your call...

I've removed those because they basically print out the values of the
XML I just passed in and I know them from XML already. I've added a
more useful (at least for me) debug statement in (WIP) V2 that prints
the arguments passed to VBOX's AttachDevice method - those are
'computed' based on XML input by this very code so it's more helpful to
know what those final values are when debugging.

> 
> > -
> > -        if (type == VIR_STORAGE_TYPE_FILE && src) {
> > -            IMedium *medium = NULL;
> > -            vboxIID mediumUUID;
> > -            PRUnichar *mediumFileUtf16 = NULL;
> > -            PRUint32 storageBus = StorageBus_Null;
> > -            PRUint32 deviceType = DeviceType_Null;
> > -            PRUint32 accessMode = AccessMode_ReadOnly;
> > -            PRInt32 deviceInst = 0;
> > -            PRInt32 devicePort = 0;
> > -            PRInt32 deviceSlot = 0;
> > +    for (i = 0; i < def->ndisks; i++) {
> > +        disk = def->disks[i];
> > +        const char *src = virDomainDiskGetSource(disk);
> > +        int type = virDomainDiskGetType(disk);
> >  
> > -            VBOX_IID_INITIALIZE(&mediumUUID);
> > -            VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
> > +        if (type != VIR_STORAGE_TYPE_FILE) {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                           _("Unsupported storage type %s, the
> > only supported "
> > +                             "type is %s"),
> > +                           virStorageTypeToString(type),
> > +                           virStorageTypeToString(VIR_STORAGE_TYPE
> > _FILE));
> > +            return -1;
> > +        }
> >  
> > -            if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_DISK) {
> > -                deviceType = DeviceType_HardDisk;
> > -                accessMode = AccessMode_ReadWrite;
> > -            } else if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_CDROM) {
> > -                deviceType = DeviceType_DVD;
> > -                accessMode = AccessMode_ReadOnly;
> > -            } else if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> > -                deviceType = DeviceType_Floppy;
> > -                accessMode = AccessMode_ReadWrite;
> > -            } else {
> > -                VBOX_UTF16_FREE(mediumFileUtf16);
> > -                continue;
> > -            }
> > +        if (!src && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> 
> I think more typically there are checks against != *_CDROM ||
> *_FLOPPY
> although perhaps this check should go slightly later [1]
> 
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                           _("Missing disk source file path"));
> > +            return -1;
> > +        }
> >  
> > -            gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj,
> > mediumFileUtf16,
> > -                                               deviceType,
> > accessMode, &medium);
> > +        IMedium *medium = NULL;
> > +        vboxIID mediumUUID;
> 
> Since you call vboxIIDUnalloc later on this [2], perhaps this should
> be
> initialized to NULL... Although, I see it only gets set when "if
> (src)",
> so perhaps that becomes the key for Unalloc too.
> 
> > +        PRUnichar *mediumFileUtf16 = NULL;
> > +        PRUint32 deviceType = DeviceType_Null;
> > +        PRUint32 accessMode = AccessMode_ReadOnly;
> > +        PRInt32 deviceSlot = disk->info.addr.drive.bus;
> > +        PRInt32 devicePort = disk->info.addr.drive.unit;
> > +        int model = -1;
> 
> We really prefer to see definitions grouped together at the top
> rather
> than in the middle of code
> 
> > +
> > +        if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> 
> [1]  If you move that !src check here, then you're accomplishing the
> same result as previously but making it more obvious (at least for
> me)
> since _LUN isn't supported...
> 
> > +            deviceType = DeviceType_HardDisk;
> > +            accessMode = AccessMode_ReadWrite;
> > +        } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> > +            deviceType = DeviceType_DVD;
> > +            accessMode = AccessMode_ReadOnly;
> > +        } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY)
> > {
> > +            deviceType = DeviceType_Floppy;
> > +            accessMode = AccessMode_ReadWrite;
> > +        } else {
> > +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                           _("Unsupported disk device type %s"),
> > +                           virDomainDiskDeviceTypeToString(disk-
> > >device));
> > +            ret = -1;
> > +            goto cleanup;
> > +        }
> 
> Also if you change this to a :
> 
>     switch ((virDomainDiskDevice) disk->device) {
>         case VIR_DOMAIN_DISK_DEVICE_DISK:
> ...
>         case VIR_DOMAIN_DISK_DEVICE_LUN:
>              VIR_DOMAIN_DISK_DEVICE_LAST:
>             virReportError()...
> ...
>     }
> 
> Then you avoid the chance that someone adds a new virDomainDiskDevice
> and doesn't address that in the vbox code.
> 
> >  
> > -            if (!medium) {
> > -                PRUnichar *mediumEmpty = NULL;
> > +        if (src) {
> > +            VBOX_IID_INITIALIZE(&mediumUUID);
> > +            VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
> >  
> > -                VBOX_UTF8_TO_UTF16("", &mediumEmpty);
> > +            rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, 
> > mediumFileUtf16,
> > +                                                    deviceType,
> > accessMode, &medium);
> >  
> > -                rc = gVBoxAPI.UIVirtualBox.OpenMedium(data-
> > >vboxObj,
> > -                                                      mediumFileUt
> > f16,
> > -                                                      deviceType,
> > accessMode,
> > -                                                      &medium);
> > -                VBOX_UTF16_FREE(mediumEmpty);
> > +            /* The following is not needed for vbox 4.2+ but older
> > versions have
> > +             * distinct find and open operations where former
> > looks in vbox media
> > +             * registry and latter at storage location. In 4.2+,
> > the OpenMedium call
> > +             * takes care of both cases internally.
> > +             */
> > +            if (!medium) {
> > +                rc = gVBoxAPI.UIVirtualBox.OpenMedium(data-
> > >vboxObj, mediumFileUtf16,
> > +                                                      deviceType,
> > accessMode, &medium);
> >              }
> >  
> >              if (!medium) {
> >                  virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                               _("Failed to attach the following
> > disk/dvd/floppy "
> > -                                 "to the machine: %s, rc=%08x"),
> > +                               _("Failed to open the following
> > disk/dvd/floppy "
> > +                                 "image file: %s, rc=%08x"),
> >                                 src, (unsigned)rc);
> > -                VBOX_UTF16_FREE(mediumFileUtf16);
> > -                continue;
> > +                ret = -1;
> > +                goto cleanup;
> >              }
> >  
> >              rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
> >              if (NS_FAILED(rc)) {
> >                  virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                               _("can't get the uuid of the file
> > to be attached "
> > +                               _("Can't get the uuid of the file
> > to be attached "
> >                                   "as harddisk/dvd/floppy: %s,
> > rc=%08x"),
> >                                 src, (unsigned)rc);
> > -                VBOX_MEDIUM_RELEASE(medium);
> > -                VBOX_UTF16_FREE(mediumFileUtf16);
> > -                continue;
> > +                ret = -1;
> > +                goto cleanup;
> >              }
> > +        }
> >  
> > -            if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_DISK) {
> > -                if (def->disks[i]->src->readonly) {
> > -                    gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Immutable);
> > -                    VIR_DEBUG("setting harddisk to immutable");
> > -                } else if (!def->disks[i]->src->readonly) {
> > -                    gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Normal);
> > -                    VIR_DEBUG("setting harddisk type to normal");
> > -                }
> > +        if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> > +            if (disk->src->readonly) {
> > +                gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Immutable);
> > +            } else if (!disk->src->readonly) {
> > +                gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Normal);
> >              }
> > +        }
> >  
> > -            if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> > -                VBOX_UTF8_TO_UTF16("IDE Controller",
> > &storageCtlName);
> > -                storageBus = StorageBus_IDE;
> > -            } else if (def->disks[i]->bus ==
> > VIR_DOMAIN_DISK_BUS_SATA) {
> > -                VBOX_UTF8_TO_UTF16("SATA Controller",
> > &storageCtlName);
> > -                storageBus = StorageBus_SATA;
> > -            } else if (def->disks[i]->bus ==
> > VIR_DOMAIN_DISK_BUS_SCSI) {
> > -                VBOX_UTF8_TO_UTF16("SCSI Controller",
> > &storageCtlName);
> > -                storageBus = StorageBus_SCSI;
> > -            } else if (def->disks[i]->bus ==
> > VIR_DOMAIN_DISK_BUS_FDC) {
> > -                VBOX_UTF8_TO_UTF16("Floppy Controller",
> > &storageCtlName);
> > -                storageBus = StorageBus_Floppy;
> > -            }
> > +        /* asssociate <disc> bus to controller */
> 
> <disk>
> 
> >  
> > -            /* get the device details i.e instance, port and slot
> > */
> > -            if (!vboxGetDeviceDetails(def->disks[i]->dst,
> > -                                      maxPortPerInst,
> > -                                      maxSlotPerPort,
> > -                                      storageBus,
> > -                                      &deviceInst,
> > -                                      &devicePort,
> > -                                      &deviceSlot)) {
> > -                virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                               _("can't get the port/slot number
> > of "
> > -                                 "harddisk/dvd/floppy to be
> > attached: "
> > -                                 "%s, rc=%08x"),
> > -                               src, (unsigned)rc);
> > -                VBOX_MEDIUM_RELEASE(medium);
> > -                vboxIIDUnalloc(&mediumUUID);
> > -                VBOX_UTF16_FREE(mediumFileUtf16);
> > -                continue;
> > -            }
> > +        switch (disk->bus) {
> > +        case VIR_DOMAIN_DISK_BUS_IDE:
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> > &storageCtlName);
> >  
> > -            /* attach the harddisk/dvd/Floppy to the storage
> > controller */
> > -            rc = gVBoxAPI.UIMachine.AttachDevice(machine,
> > -                                                 storageCtlName,
> > -                                                 devicePort,
> > -                                                 deviceSlot,
> > -                                                 deviceType,
> > -                                                 medium);
> > +            break;
> >  
> > -            if (NS_FAILED(rc)) {
> > -                virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                               _("could not attach the file as "
> > -                                 "harddisk/dvd/floppy: %s,
> > rc=%08x"),
> > -                               src, (unsigned)rc);
> > -            } else {
> > -                DEBUGIID("Attached HDD/DVD/Floppy with UUID",
> > &mediumUUID);
> > +        case VIR_DOMAIN_DISK_BUS_SATA:
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &storageCtlName);
> > +
> > +            break;
> > +
> > +        case VIR_DOMAIN_DISK_BUS_SCSI:
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &storageCtlName);
> > +
> > +            model = virDomainDeviceFindControllerModel(def, &disk-
> > >info,
> > +                                                       VIR_DOMAIN_
> > CONTROLLER_TYPE_SCSI);
> > +
> > +            /* if the model is lsisas1068, set vbox bus type to
> > SAS */
> > +            if (model ==
> > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> > +                VBOX_UTF16_FREE(storageCtlName);
> > +                VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &storageCtlName);
> >              }
> >  
> > -            VBOX_MEDIUM_RELEASE(medium);
> > -            vboxIIDUnalloc(&mediumUUID);
> > -            VBOX_UTF16_FREE(mediumFileUtf16);
> > -            VBOX_UTF16_FREE(storageCtlName);
> > +            break;
> > +
> > +        case VIR_DOMAIN_DISK_BUS_FDC:
> > +            VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> > &storageCtlName);
> > +            devicePort = 0;
> > +            deviceSlot = disk->info.addr.drive.unit;
> > +
> > +            break;
> >          }
> > +
> > +        /* attach the harddisk/dvd/Floppy to the storage
> > controller */
> > +        rc = gVBoxAPI.UIMachine.AttachDevice(machine,
> > +                                             storageCtlName,
> > +                                             devicePort,
> > +                                             deviceSlot,
> > +                                             deviceType,
> > +                                             medium);
> > +
> > +        if (NS_FAILED(rc)) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("Could not attach the file as "
> > +                             "harddisk/dvd/floppy: %s, rc=%08x"),
> > +                           src, (unsigned)rc);
> > +            ret = -1;
> > +        }
> 
> you could keep the "} else { DEBUGIID("Attached HDD/DVD/Floppy with
> UUID", &mediumUUID); |	" - your call
> 
> 
> > +
> > + cleanup:
> > +        vboxIIDUnalloc(&mediumUUID);
> 
> [2] This only gets set on "if (src)", so you probably need that
> condition here too - just to be safe...
> 
> > +        VBOX_MEDIUM_RELEASE(medium);
> > +        VBOX_UTF16_FREE(mediumFileUtf16);
> > +        VBOX_UTF16_FREE(storageCtlName);
> > +
> > +        if (ret < 0)
> > +            break;
> 
> Not a normal way to have cleanup: label inside a for loop, but it
> does
> avoid having duplicated logic...
> 
> >      }
> > +
> > +    return ret;
> >  }
> >  
> >  static void
> > @@ -1853,6 +1875,7 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> > const char *xml, unsigned int flags
> >      char uuidstr[VIR_UUID_STRING_BUFLEN];
> >      virDomainPtr ret = NULL;
> >      unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE;
> > +    int success = 0;
> 
> So this is more of a "bool machineReady = false"
> 
> I don't like @success, but naming is hard and whatever you pick will
> cause someone else to say - why not use something else...
> 
> >  
> >      virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
> >  
> > @@ -1948,7 +1971,10 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> > const char *xml, unsigned int flags
> >      gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
> >  
> >      vboxSetBootDeviceOrder(def, data, machine);
> > -    vboxAttachDrives(def, data, machine);
> > +    if (vboxAttachStorageControllers(def, data, machine) < 0)
> > +        goto cleanup;
> > +    if (vboxAttachDrives(def, data, machine) < 0)
> > +        goto cleanup;
> >      vboxAttachSound(def, machine);
> >      if (vboxAttachNetwork(def, data, machine) < 0)
> >          goto cleanup;
> > @@ -1959,30 +1985,40 @@ vboxDomainDefineXMLFlags(virConnectPtr
> > conn, const char *xml, unsigned int flags
> >      vboxAttachUSB(def, data, machine);
> >      vboxAttachSharedFolder(def, data, machine);
> >  
> > -    /* Save the machine settings made till now and close the
> > -     * session. also free up the mchiid variable used.
> > -     */
> > +    success = 1;
> > +
> > + cleanup:
> > +    /* Save the machine settings made till now, even if incomplete
> > */
> >      rc = gVBoxAPI.UIMachine.SaveSettings(machine);
> > -    if (NS_FAILED(rc)) {
> > +    if (NS_FAILED(rc))
> >          virReportError(VIR_ERR_INTERNAL_ERROR,
> > -                       _("failed no saving settings, rc=%08x"),
> > (unsigned)rc);
> > -        goto cleanup;
> > +                       _("failed to save VM settings, rc=%08x"),
> > +                       (unsigned) rc);
> 
> So if you fail to save the settings, then unlike previously you're
> going
> to call virGetDomain() which doesn't seem right.
> 
> Is the (unsigned) typecast really necessary on a 08x ?
> 
> > +
> > +    if (success) {
> > +        ret = virGetDomain(conn, def->name, def->uuid, -1);
> > +    } else {
> > +        /* Unregister incompletely configured VM to not leave
> > garbage behind */
> > +        gVBoxAPI.UISession.Close(data->vboxSession);> +        rc
> > = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
> > +        if (NS_SUCCEEDED(rc)) {
> > +            gVBoxAPI.deleteConfig(machine);
> > +        } else {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR,
> > +                           _("could not cleanup partial domain
> > after failure "
> > +                             "to define, rc=%08x"),
> > +                           (unsigned) rc);
> > +        }
> >      }
> >  
> >      gVBoxAPI.UISession.Close(data->vboxSession);
> 
> Prior logic allowed gVBoxAPI.UISession.Close(data->vboxSession);
> prior
> to  virGetDomain(conn, def->name, def->uuid, -1); - keeping this here
> would mean in the } else { path from above we'd call this twice.
> 
> Is that right?  Can/Should it move before the "if (success) {" above?
> 
> >      vboxIIDUnalloc(&mchiid);
> >  
> > -    ret = virGetDomain(conn, def->name, def->uuid, -1);
> >      VBOX_RELEASE(machine);
> >  
> >      virDomainDefFree(def);
> >  
> >      return ret;
> > -
> > - cleanup:
> > -    VBOX_RELEASE(machine);
> > -    virDomainDefFree(def);
> > -    return NULL;
> >  }
> >  
> >  static virDomainPtr
> > @@ -3057,8 +3093,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >      bool error = false;
> >      int diskCount = 0;
> >      size_t i;
> > -    PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> > -    PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> > +    PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {};
> > +    PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {};
> >  
> >      def->ndisks = 0;
> >      gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
> > @@ -3151,7 +3187,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >              def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
> >          } else if (storageBus == StorageBus_SATA) {
> >              def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
> > -        } else if (storageBus == StorageBus_SCSI) {
> > +        } else if (storageBus == StorageBus_SCSI ||
> > +                   storageBus == StorageBus_SAS) {
> >              def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> >          } else if (storageBus == StorageBus_Floppy) {
> >              def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
> > diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> > index b08ad1e3e..23940fc63 100644
> > --- a/src/vbox/vbox_common.h
> > +++ b/src/vbox/vbox_common.h
> > @@ -326,6 +326,14 @@ enum HardDiskVariant
> >  # define VBOX_E_INVALID_SESSION_STATE 0x80BB000B
> >  # define VBOX_E_OBJECT_IN_USE 0x80BB000C
> >  
> > +/* VBOX storage controller name definitions */
> > +# define VBOX_CONTROLLER_IDE_NAME "IDE Controller"
> > +# define VBOX_CONTROLLER_FLOPPY_NAME "Floppy Controller"
> > +# define VBOX_CONTROLLER_SATA_NAME "SATA Controller"
> > +# define VBOX_CONTROLLER_SCSI_NAME "SCSI Controller"
> > +# define VBOX_CONTROLLER_SAS_NAME "SAS Controller"
> > +
> > +
> >  /* Simplied definitions in vbox_CAPI_*.h */
> >  
> >  typedef void const *PCVBOXXPCOM;
> > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> > index 6592cbd63..b7ced62dc 100644
> > --- a/src/vbox/vbox_tmpl.c
> > +++ b/src/vbox/vbox_tmpl.c
> 
> I think this should be a separate patch prior to this one.  You're
> adjusting format and removing the ATTRIBUTE_UNUSED for arguments that
> are actually used.
> 
> > @@ -684,7 +684,9 @@ _virtualboxCreateHardDisk(IVirtualBox *vboxObj,
> > PRUnichar *format,
> >  #if VBOX_API_VERSION < 5000000
> >      return vboxObj->vtbl->CreateHardDisk(vboxObj, format,
> > location, medium);
> >  #elif VBOX_API_VERSION >= 5000000 /*VBOX_API_VERSION >= 5000000*/
> > -    return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
> > AccessMode_ReadWrite, DeviceType_HardDisk, medium);
> > +    return vboxObj->vtbl->CreateMedium(vboxObj, format, location,
> > +                                       AccessMode_ReadWrite,
> > +                                       DeviceType_HardDisk,
> > medium);
> >  #endif /*VBOX_API_VERSION >= 5000000*/
> >  }
> >  
> > @@ -696,37 +698,28 @@ _virtualboxRegisterMachine(IVirtualBox
> > *vboxObj, IMachine *machine)
> >  
> >  static nsresult
> >  _virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location,
> > -                        PRUint32 deviceType ATTRIBUTE_UNUSED,
> > -                        PRUint32 accessMode ATTRIBUTE_UNUSED,
> > -                        IMedium **medium)
> > +                        PRUint32 deviceType,
> > +                        PRUint32 accessMode ATTRIBUTE_UNUSED,
> > IMedium **medium)
> 
> Keep IMedium **medium on it's own line.
> 
> It's the preferred way at least for libvirt code on function args.
> 
> >  {
> >  #if VBOX_API_VERSION < 4002000
> > -    return vboxObj->vtbl->FindMedium(vboxObj, location,
> > -                                     deviceType, medium);
> > +    return vboxObj->vtbl->FindMedium(vboxObj, location,
> > deviceType, medium);
> >  #else /* VBOX_API_VERSION >= 4002000 */
> > -    return vboxObj->vtbl->OpenMedium(vboxObj, location,
> > -                                     deviceType, accessMode,
> > PR_FALSE, medium);
> > +    return vboxObj->vtbl->OpenMedium(vboxObj, location,
> > deviceType, accessMode,
> > +                                     PR_FALSE, medium);
> >  #endif /* VBOX_API_VERSION >= 4002000 */
> >  }
> >  
> >  static nsresult
> > -_virtualboxOpenMedium(IVirtualBox *vboxObj ATTRIBUTE_UNUSED,
> > -                      PRUnichar *location ATTRIBUTE_UNUSED,
> > -                      PRUint32 deviceType ATTRIBUTE_UNUSED,
> > -                      PRUint32 accessMode ATTRIBUTE_UNUSED,
> > -                      IMedium **medium ATTRIBUTE_UNUSED)
> > +_virtualboxOpenMedium(IVirtualBox *vboxObj, PRUnichar *location,
> > +                      PRUint32 deviceType, PRUint32 accessMode,
> > +                      IMedium **medium)
> 
> For this one - keep each argument on it's own line and just remove
> the
> ATTRIBUTE_UNUSED
> 
> >  {
> >  #if VBOX_API_VERSION == 4000000
> > -    return vboxObj->vtbl->OpenMedium(vboxObj,
> > -                                     location,
> > -                                     deviceType, accessMode,
> > +    return vboxObj->vtbl->OpenMedium(vboxObj, location,
> > deviceType, accessMode,
> >                                       medium);
> >  #elif VBOX_API_VERSION >= 4001000
> > -    return vboxObj->vtbl->OpenMedium(vboxObj,
> > -                                     location,
> > -                                     deviceType, accessMode,
> > -                                     false,
> > -                                     medium);
> > +    return vboxObj->vtbl->OpenMedium(vboxObj, location,
> > deviceType, accessMode,
> > +                                     false, medium);
> >  #endif
> >  }
> >  
> > @@ -778,12 +771,8 @@ _machineGetStorageControllerByName(IMachine
> > *machine, PRUnichar *name,
> >  }
> >  
> >  static nsresult
> > -_machineAttachDevice(IMachine *machine ATTRIBUTE_UNUSED,
> > -                     PRUnichar *name ATTRIBUTE_UNUSED,
> > -                     PRInt32 controllerPort ATTRIBUTE_UNUSED,
> > -                     PRInt32 device ATTRIBUTE_UNUSED,
> > -                     PRUint32 type ATTRIBUTE_UNUSED,
> > -                     IMedium * medium ATTRIBUTE_UNUSED)
> > +_machineAttachDevice(IMachine *machine, PRUnichar *name, PRInt32
> > controllerPort,
> > +                     PRInt32 device, PRUint32 type, IMedium *
> > medium)
> 
> Same - one line per argument is preferred
> 
> 
> John
> >  {
> >      return machine->vtbl->AttachDevice(machine, name,
> > controllerPort,
> >                                         device, type, medium);
> > 

On 10/09/2017 04:49 PM, Dawid Zamirski wrote:

From: Dawid Zamirski <dzamirski@datto.com>

* added vboxDumpStorageControllers * replaced vboxDumpIDEHDDs with vboxDumpDisks which has been refectored quite a bit to handle removable devices, the new SAS controller support etc. * align the logic in vboxSnapshotGetReadWriteDisks and vboxSnapshotGetReadOnlyDisks to more closely resemble vboxDumpDisks. * vboxGenerateMediumName was simplified to no longer "compute" the device name value as deviePort/deviceSlot and their upper bound values are no longer enough to do this accurately due to the added SAS bus support which does not "map" directly into libvirt XML semantics * vboxGetMaxPortSlotValues is now removed as it's no longer used --- src/vbox/vbox_common.c | 691 ++++++++++++++++++++++++++++--------------------- 1 file changed, 393 insertions(+), 298 deletions(-)

Nny way to break this up into smaller more reviewably manageable chunks? Certainly as I pointed out before, separating out the SAS into its own patch could include whatever changes would happen here to support it. It's too bad vboxDumpIDEHDDs couldn't be more easily split using the current logic then updates applied to use the controller fetches. I've kind of lost steam and think there's enough up to this point that needs to be redone that I'll wait for the followup to look more in depth. I noted a couple of things below.

diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 7645b29a0..dd876645a 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -290,126 +290,44 @@ static int openSessionForMachine(vboxDriverPtr data, const unsigned char *dom_uu return 0; }

-/** - * function to get the values for max port per - * instance and max slots per port for the devices - * - * @returns true on Success, false on failure. - * @param vbox Input IVirtualBox pointer - * @param maxPortPerInst Output array of max port per instance - * @param maxSlotPerPort Output array of max slot per port - * - */ - -static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox, - PRUint32 *maxPortPerInst, - PRUint32 *maxSlotPerPort) -{ - ISystemProperties *sysProps = NULL; - - if (!vbox) - return false; - - gVBoxAPI.UIVirtualBox.GetSystemProperties(vbox, &sysProps); - - if (!sysProps) - return false; - - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps, - StorageBus_IDE, - &maxPortPerInst[StorageBus_IDE]); - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps, - StorageBus_SATA, - &maxPortPerInst[StorageBus_SATA]); - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps, - StorageBus_SCSI, - &maxPortPerInst[StorageBus_SCSI]); - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps, - StorageBus_SAS, - &maxPortPerInst[StorageBus_SAS]); - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps, - StorageBus_Floppy, - &maxPortPerInst[StorageBus_Floppy]); - - gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps, - StorageBus_IDE, - &maxSlotPerPort[StorageBus_IDE]); - gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps, - StorageBus_SATA, - &maxSlotPerPort[StorageBus_SATA]); - gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps, - StorageBus_SCSI, - &maxSlotPerPort[StorageBus_SCSI]); - gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps, - StorageBus_SAS, - &maxSlotPerPort[StorageBus_SAS]); - gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps, - StorageBus_Floppy, - &maxSlotPerPort[StorageBus_Floppy]); - - VBOX_RELEASE(sysProps); - - return true; -} - /** * function to generate the name for medium, * for e.g: hda, sda, etc * * @returns null terminated string with device name or NULL * for failures - * @param conn Input Connection Pointer * @param storageBus Input storage bus type - * @param deviceInst Input device instance number * @param devicePort Input port number * @param deviceSlot Input slot number - * @param aMaxPortPerInst Input array of max port per device instance - * @param aMaxSlotPerPort Input array of max slot per device port - * + * @param sdCount Running total of disk devices with "sd" prefix */ -static char *vboxGenerateMediumName(PRUint32 storageBus, - PRInt32 deviceInst, - PRInt32 devicePort, - PRInt32 deviceSlot, - PRUint32 *aMaxPortPerInst, - PRUint32 *aMaxSlotPerPort) +static char * +vboxGenerateMediumName(PRUint32 storageBus, PRInt32 devicePort, + PRInt32 deviceSlot, size_t sdCount)

As w/ previous patch - one argument per line is the normal libvirt preference

{ const char *prefix = NULL; char *name = NULL; int total = 0; - PRUint32 maxPortPerInst = 0; - PRUint32 maxSlotPerPort = 0; - - if (!aMaxPortPerInst || - !aMaxSlotPerPort) - return NULL;

if ((storageBus < StorageBus_IDE) || (storageBus > StorageBus_SAS)) return NULL;

- maxPortPerInst = aMaxPortPerInst[storageBus]; - maxSlotPerPort = aMaxSlotPerPort[storageBus]; - total = (deviceInst * maxPortPerInst * maxSlotPerPort) - + (devicePort * maxSlotPerPort) - + deviceSlot; - if (storageBus == StorageBus_IDE) { prefix = "hd"; + total = devicePort * 2 + deviceSlot; } else if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || storageBus == StorageBus_SAS) { prefix = "sd"; + total = sdCount; } else if (storageBus == StorageBus_Floppy) { + total = deviceSlot; prefix = "fd"; }

name = virIndexToDiskName(total, prefix);

- VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, " - "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u", - NULLSTR(name), total, storageBus, deviceInst, devicePort, - deviceSlot, maxPortPerInst, maxSlotPerPort); return name; }

@@ -3086,162 +3004,300 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr def, IMachine *mach }

static void -vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) +vboxDumpStorageControllers(virDomainDefPtr def, + vboxDriverPtr data ATTRIBUTE_UNUSED,

Is this ever going to be used? Why pass it then to a newly defined function? Is it because all the callers pass it whether or not it's used? Seems strange, especially considering it's the driver pointer.

+ IMachine *machine) { - /* dump IDE hdds if present */ - vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; - bool error = false; - int diskCount = 0; - size_t i; - PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {}; + vboxArray storageControllers = VBOX_ARRAY_INITIALIZER; + virDomainControllerDefPtr cont = NULL; + size_t i = 0;

- def->ndisks = 0; - gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, - gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine)); + gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine, + gVBoxAPI.UArray.handleMachineGetStorageControllers(machine));

- /* get the number of attachments */ - for (i = 0; i < mediumAttachments.count; i++) { - IMediumAttachment *imediumattach = mediumAttachments.items[i]; - if (imediumattach) { - IMedium *medium = NULL; + for (i = 0; i < storageControllers.count; i++) { + IStorageController *controller = storageControllers.items[i]; + PRUint32 storageBus = StorageBus_Null; + PRUint32 controllerType = StorageControllerType_Null; + + gVBoxAPI.UIStorageController.GetBus(controller, &storageBus); + gVBoxAPI.UIStorageController.GetControllerType(controller, + &controllerType); + + switch (storageBus) { + case StorageBus_IDE: + { + int model = -1; + switch (controllerType) { + case StorageControllerType_PIIX3: + model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3; + break; + case StorageControllerType_PIIX4: + model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4; + break; + case StorageControllerType_ICH6: + model = VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6; + break; + }

- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); - if (medium) { - def->ndisks++; - VBOX_RELEASE(medium); + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_IDE, + -1, model); + + break; + } + case StorageBus_SCSI: + { + int model = -1; + switch (controllerType) { + case StorageControllerType_BusLogic: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC; + break; + case StorageControllerType_LsiLogic: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC; + break; } + + cont = virDomainDefAddController(def, VIR_DOMAIN_CONTROLLER_TYPE_SCSI, + -1, model); + + break; + } + case StorageBus_SAS: + cont = virDomainDefAddController(def, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI, -1, + VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068); + break; + case StorageBus_SATA: + cont = virDomainDefAddController(def, + VIR_DOMAIN_CONTROLLER_TYPE_SATA, -1, -1); + break; + case StorageBus_Floppy: + cont = virDomainDefAddController(def, + VIR_DOMAIN_CONTROLLER_TYPE_FDC, -1, -1); + break; } }

- /* Allocate mem, if fails return error */ - if (VIR_ALLOC_N(def->disks, def->ndisks) >= 0) { - for (i = 0; i < def->ndisks; i++) { - virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); - if (!disk) { - error = true; - break; + if (!cont) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Failed to add controller definition")); + } +} + +static void +vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)

One argument per line

+{ + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *mediumattach = NULL; + IMedium *medium = NULL; + nsresult rc; + size_t sdCount = 0, i, j; + + def->ndisks = 0;> + rc = gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, + gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine)); + + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not get machine's medium attachments")); + goto error; + } + + /* get the number of attachments */ + for (i = 0; i < mediumAttachments.count; i++) { + mediumattach = mediumAttachments.items[i]; + if (mediumattach) { + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumattach, &medium); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get medium")); + goto error; } - def->disks[i] = disk; + + def->ndisks++; + VBOX_RELEASE(medium); } - } else { - error = true; }

- if (!error) - error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) + goto error; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + if (!disk) + goto error; + + def->disks[i] = disk; + }

/* get the attachment details here */ - for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) { - IMediumAttachment *imediumattach = mediumAttachments.items[i]; + for (i = 0; i < mediumAttachments.count; i++) { + mediumattach = mediumAttachments.items[i]; IStorageController *storageController = NULL; PRUnichar *storageControllerName = NULL; PRUint32 deviceType = DeviceType_Null; PRUint32 storageBus = StorageBus_Null; PRBool readOnly = PR_FALSE; - IMedium *medium = NULL; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; + virDomainDiskDefPtr disk = def->disks[i];

- if (!imediumattach) - continue; + if (!mediumattach) + goto skip;

- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); - if (!medium) - continue; + rc = gVBoxAPI.UIMediumAttachment.GetController(mediumattach, + &storageControllerName); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get storage controller name for medium " + "attachment, rc=%08x", (unsigned) rc); + goto skip; + }

- gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); - if (!storageControllerName) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, + storageControllerName, + &storageController); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get storage controller for medium attachment, " + "rc=%08x", (unsigned) rc); + goto skip; }

- gVBoxAPI.UIMachine.GetStorageControllerByName(machine, - storageControllerName, - &storageController); - VBOX_UTF16_FREE(storageControllerName); - if (!storageController) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIStorageController.GetBus(storageController, + &storageBus); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get storage controller bus type, rc=%08x", + (unsigned) rc); + goto skip; }

- gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); - VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); - VBOX_UTF16_FREE(mediumLocUtf16); - ignore_value(virDomainDiskSetSource(def->disks[diskCount], - mediumLocUtf8)); - VBOX_UTF8_FREE(mediumLocUtf8); + rc = gVBoxAPI.UIMediumAttachment.GetType(mediumattach, &deviceType); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get device type for medium attachment, rc=%08x", + (unsigned) rc); + goto skip; + }

- if (!virDomainDiskGetSource(def->disks[diskCount])) { - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); - error = true; - break; + rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumattach, &devicePort); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get device port for medium attachment, rc=%08x", + (unsigned) rc); + goto skip; + } + + rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumattach, &deviceSlot); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get device slot for medium attachment, rc=%08x", + (unsigned) rc); + goto skip; }

- gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumattach, &medium); + if (NS_FAILED(rc)) { + VIR_WARN("Could not get medium for medium attachment, rc=%08x", + (unsigned) rc); + goto skip; + } + + /* removable drives might not have mediums */ + if (medium) { + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); + if (NS_FAILED(rc)) { + VIR_WARN("Clould not get storage location for medium, rc=%08x", + (unsigned) rc); + goto skip; + } + + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); + if (NS_FAILED(rc)) { + VIR_WARN("Clould not get read-only status for medium, rc=%08x", + (unsigned) rc); + goto skip; + } + + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { + VIR_WARN("Could not set disk source for medium %s", + mediumLocUtf8); + goto skip; + } + } + + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + disk->info.addr.drive.bus = 0; + disk->info.addr.drive.unit = devicePort; + + disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot, + sdCount); if (storageBus == StorageBus_IDE) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + disk->bus = VIR_DOMAIN_DISK_BUS_IDE; + disk->info.addr.drive.bus = devicePort; /* primary, secondary */ + disk->info.addr.drive.unit = deviceSlot; /* master, slave */ + disk->dst = virIndexToDiskName(devicePort * 2 + deviceSlot, "hd"); + disk->info.addr.drive.controller = + virDomainControllerFindByType(def, + VIR_DOMAIN_CONTROLLER_TYPE_IDE); } else if (storageBus == StorageBus_SATA) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + disk->bus = VIR_DOMAIN_DISK_BUS_SATA; + disk->info.addr.drive.controller = + virDomainControllerFindByType(def, + VIR_DOMAIN_CONTROLLER_TYPE_SATA); + sdCount++; } else if (storageBus == StorageBus_SCSI || storageBus == StorageBus_SAS) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; + sdCount++; + /* find scsi controller index with matching model for vbox bus type */ + for (j = 0; j < def->ncontrollers; j++) { + virDomainControllerDefPtr ctrl = def->controllers[j]; + + if (ctrl->type != VIR_DOMAIN_CONTROLLER_TYPE_SCSI) + continue; + + if (storageBus == StorageBus_SAS && + ctrl->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { + disk->info.addr.drive.controller = ctrl->idx; + } else if (storageBus == StorageBus_SCSI && + ctrl->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { + disk->info.addr.drive.controller = ctrl->idx; + } + } } else if (storageBus == StorageBus_Floppy) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + disk->bus = VIR_DOMAIN_DISK_BUS_FDC; + disk->info.addr.drive.unit = deviceSlot; }

- gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType); if (deviceType == DeviceType_HardDisk) - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK; + disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; else if (deviceType == DeviceType_Floppy) - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; + disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; else if (deviceType == DeviceType_DVD) - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM; - - gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); - gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); - def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, - deviceInst, - devicePort, - deviceSlot, - maxPortPerInst, - maxSlotPerPort); - if (!def->disks[diskCount]->dst) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not generate medium name for the disk " - "at: controller instance:%u, port:%d, slot:%d"), - deviceInst, devicePort, deviceSlot); - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); - error = true; - break; - } + disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;

- gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) - def->disks[diskCount]->src->readonly = true; + disk->src->readonly = true;

- virDomainDiskSetType(def->disks[diskCount], - VIR_STORAGE_TYPE_FILE); + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);

+ skip: + VBOX_UTF16_FREE(storageControllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); VBOX_RELEASE(medium); VBOX_RELEASE(storageController); - diskCount++; }

gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);

- /* cleanup on error */ - if (error) { - for (i = 0; i < def->ndisks; i++) - VIR_FREE(def->disks[i]); - VIR_FREE(def->disks); - def->ndisks = 0; - } + return; + + error: + for (i = 0; i < def->ndisks; i++) + VIR_FREE(def->disks[i]); + VIR_FREE(def->disks); + def->ndisks = 0; + gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); }

static int @@ -3934,8 +3990,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (vboxDumpDisplay(def, data, machine) < 0) goto cleanup;

- vboxDumpIDEHDDs(def, data, machine); - + vboxDumpStorageControllers(def, data, machine); + vboxDumpDisks(def, data, machine);

As an aside - it's really strange to me how GetXMLDesc works for a running machine here - querying the live system to get everything... I'm so used to the QEMU model to save the live definition... John

vboxDumpSharedFolders(def, data, machine); vboxDumpNetwork(def, data, machine, networkAdapterCount); vboxDumpAudio(def, data, machine); @@ -5490,8 +5546,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data, return snapshot; }

-static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, - virDomainSnapshotPtr snapshot) +static int +vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, + virDomainSnapshotPtr snapshot) { virDomainPtr dom = snapshot->domain; vboxDriverPtr data = dom->conn->privateData; @@ -5500,9 +5557,7 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, ISnapshot *snap = NULL; IMachine *snapMachine = NULL; vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; - int diskCount = 0; + size_t diskCount = 0, sdCount = 0; nsresult rc; vboxIID snapIid; char *snapshotUuidStr = NULL; @@ -5571,9 +5626,6 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, goto cleanup; }

- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort)) - goto cleanup; - /* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { IStorageController *storageController = NULL; @@ -5583,7 +5635,6 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, IMedium *disk = NULL; PRUnichar *childLocUtf16 = NULL; char *childLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; vboxArray children = VBOX_ARRAY_INITIALIZER; @@ -5592,31 +5643,76 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, void *handle; size_t j = 0; size_t k = 0; + if (!imediumattach) continue; - rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk); + + rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, + &storageControllerName); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium")); + _("cannot get controller")); goto cleanup; } - if (!disk) + + if (!storageControllerName) continue; - rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); + + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, + storageControllerName, + &storageController); + VBOX_UTF16_FREE(storageControllerName); + if (!storageController) + continue; + + rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get controller")); + _("cannot get storage controller bus")); + VBOX_RELEASE(storageController); goto cleanup; } - if (!storageControllerName) { - VBOX_RELEASE(disk); - continue; + rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get medium attachment type")); + VBOX_RELEASE(storageController); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get medium attachment type")); + VBOX_RELEASE(storageController); + goto cleanup; } + rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get medium attachment device")); + VBOX_RELEASE(storageController); + goto cleanup; + } + + rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get medium")); + VBOX_RELEASE(storageController); + goto cleanup; + } + + /* skip removable disk */ + if (!disk) + goto skip; + handle = gVBoxAPI.UArray.handleMediumGetChildren(disk); rc = gVBoxAPI.UArray.vboxArrayGet(&children, disk, handle); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get children disk")); + VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); goto cleanup; } handle = gVBoxAPI.UArray.handleMediumGetSnapshotIds(disk); @@ -5625,8 +5721,11 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get snapshot ids")); + VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); goto cleanup; } + for (j = 0; j < children.count; ++j) { IMedium *child = children.items[j]; for (k = 0; k < snapshotIids.count; ++k) { @@ -5634,18 +5733,13 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, char *diskSnapIdStr = NULL; VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr); if (STREQ(diskSnapIdStr, snapshotUuidStr)) { - rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, - storageControllerName, - &storageController); - VBOX_UTF16_FREE(storageControllerName); - if (!storageController) { - VBOX_RELEASE(child); - break; - } rc = gVBoxAPI.UIMedium.GetLocation(child, &childLocUtf16); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get disk location")); + VBOX_RELEASE(child); + VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); goto cleanup; } VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8); @@ -5653,52 +5747,37 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, if (VIR_STRDUP(def->disks[diskCount].src->path, childLocUtf8) < 0) { VBOX_RELEASE(child); VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); goto cleanup; } VBOX_UTF8_FREE(childLocUtf8);

- rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get storage controller bus")); - goto cleanup; - } - rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium attachment type")); - goto cleanup; - } - rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium attachment type")); - goto cleanup; - } - rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium attachment device")); - goto cleanup; - } def->disks[diskCount].src->type = VIR_STORAGE_TYPE_FILE; def->disks[diskCount].name = vboxGenerateMediumName(storageBus, - deviceInst, devicePort, deviceSlot, - maxPortPerInst, - maxSlotPerPort); + sdCount); } VBOX_UTF8_FREE(diskSnapIdStr); } } + + diskCount++; + + skip: VBOX_RELEASE(storageController); VBOX_RELEASE(disk); - diskCount++; + + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) + sdCount++; + } + gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);

ret = 0; + cleanup: if (ret < 0) { for (i = 0; i < def->ndisks; i++) @@ -5710,9 +5789,9 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, return ret; }

-static -int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, - virDomainSnapshotDefPtr def) +static int +vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, + virDomainSnapshotPtr snapshot) { virDomainPtr dom = snapshot->domain; vboxDriverPtr data = dom->conn->privateData; @@ -5725,9 +5804,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, nsresult rc; vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; size_t i = 0; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; - int diskCount = 0; + size_t diskCount = 0, sdCount = 0; int ret = -1;

if (!data->vboxObj) @@ -5790,9 +5867,6 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, goto cleanup; }

- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort)) - goto cleanup; - /* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->dom->ndisks; i++) { PRUnichar *storageControllerName = NULL; @@ -5805,16 +5879,17 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; IMediumAttachment *imediumattach = mediumAttachments.items[i]; + if (!imediumattach) continue; + rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cannot get medium")); goto cleanup; } - if (!disk) - continue; + rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -5834,41 +5909,85 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, VBOX_UTF16_FREE(storageControllerName); if (!storageController) continue; - rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16); + + rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get disk location")); + _("cannot get storage controller bus")); goto cleanup; } - VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); - VBOX_UTF16_FREE(mediumLocUtf16); - if (VIR_STRDUP(def->dom->disks[diskCount]->src->path, mediumLocUtf8) < 0) + + rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get medium attachment port")); goto cleanup; + }

- VBOX_UTF8_FREE(mediumLocUtf8); + rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get device")); + goto cleanup; + }

- rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); + + rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get storage controller bus")); + _("cannot get medium attachment type")); goto cleanup; } + + if (disk) { + rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get disk location")); + goto cleanup; + } + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + if (VIR_STRDUP(def->dom->disks[diskCount]->src->path, mediumLocUtf8) < 0) + goto cleanup; + + VBOX_UTF8_FREE(mediumLocUtf8); + + rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot get read only attribute")); + goto cleanup; + } + } else { + /* for removable drives, bump sdCount according to type so that + * device names are properly generated, and then skip it + */ + if (storageBus == StorageBus_SATA || + storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) + sdCount++; + + continue; + } + + def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + devicePort, + deviceSlot, + sdCount); if (storageBus == StorageBus_IDE) { def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; - } else if (storageBus == StorageBus_SCSI) { + sdCount++; + } else if (storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) { def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + sdCount++; } else if (storageBus == StorageBus_Floppy) { def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; }

- rc = gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium attachment type")); - goto cleanup; - } if (deviceType == DeviceType_HardDisk) def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK; else if (deviceType == DeviceType_Floppy) @@ -5876,33 +5995,9 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, else if (deviceType == DeviceType_DVD) def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;

- rc = gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium attachment port")); - goto cleanup; - } - rc = gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get device")); - goto cleanup; - } - rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get read only attribute")); - goto cleanup; - } if (readOnly == PR_TRUE) def->dom->disks[diskCount]->src->readonly = true; def->dom->disks[diskCount]->src->type = VIR_STORAGE_TYPE_FILE; - def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, - deviceInst, - devicePort, - deviceSlot, - maxPortPerInst, - maxSlotPerPort); if (!def->dom->disks[diskCount]->dst) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not generate medium name for the disk " @@ -5995,7 +6090,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0) VIR_DEBUG("Could not get read write disks for snapshot");

- if (vboxSnapshotGetReadOnlyDisks(snapshot, def) < 0) + if (vboxSnapshotGetReadOnlyDisks(def, snapshot) < 0) VIR_DEBUG("Could not get Readonly disks for snapshot"); }