[libvirt] [PATCH v2 00/15] vbox: Improve handling of storage devices
From: Dawid Zamirski <dzrudy@gmail.com> v1: https://www.redhat.com/archives/libvir-list/2017-October/msg00381.html Changes since v1: * split out the original patches into smaller ones, with each bugfix or cleanup task is in its own isolated and compilable patch * make sure switch statements are type casted and all cases are handled * fixed implementation of partial VM cleanup code and isolated into separate patch (patch #3) * squashed doc and rng updates into a single patch (#9), updated doc wording as suggested * addressed other minor issues (add debug statements, code formatting, better comments, commit message spelling etc) Dawid Zamirski (15): vbox: Update ATTRIBUTE_UNUSED usage vbox: Close media when undefining domains vbox: Cleanup partially-defined VM on failure vbox: vboxAttachDrives now relies on address info vbox: Cleanup vboxAttachDrives implementation vbox: Errors in vboxAttachDrives are now critical vbox: Support empty removable drives. vbox: Add more IStorageController API mappings domain: Allow 'model' attribute for ide controller vbox: Process <controller> element in domain XML vbox: Add vboxDumpStorageControllers vbox: Correctly generate drive name in dumpxml vbox: Cleanup vboxDumpDisks implementation vbox: Process empty removable disks in dumpxml vbox: Generate disk address element in dumpxml docs/formatdomain.html.in | 4 + docs/schemas/domaincommon.rng | 18 +- src/conf/domain_conf.c | 9 + src/conf/domain_conf.h | 9 + src/libvirt_private.syms | 2 + src/vbox/vbox_common.c | 1422 +++++++++++++++++++++++++---------------- src/vbox/vbox_common.h | 21 + src/vbox/vbox_tmpl.c | 93 ++- src/vbox/vbox_uniformed_api.h | 3 + 9 files changed, 983 insertions(+), 598 deletions(-) -- 2.14.2
Since the removal of VBOX <= 3x, the function arguments are actually used so they should not be marked with ATTRIBUTE_UNUSED anymore. --- src/vbox/vbox_tmpl.c | 49 +++++++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index dffeabde0..4aa5e9a63 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -665,7 +665,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*/ } @@ -676,38 +678,33 @@ _virtualboxRegisterMachine(IVirtualBox *vboxObj, IMachine *machine) } static nsresult -_virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location, - PRUint32 deviceType ATTRIBUTE_UNUSED, +_virtualboxFindHardDisk(IVirtualBox *vboxObj, + PRUnichar *location, + PRUint32 deviceType, PRUint32 accessMode ATTRIBUTE_UNUSED, IMedium **medium) { #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) { #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 } @@ -759,12 +756,12 @@ _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) { return machine->vtbl->AttachDevice(machine, name, controllerPort, device, type, medium); -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Since the removal of VBOX <= 3x, the function arguments are actually used so they should not be marked with ATTRIBUTE_UNUSED anymore. --- src/vbox/vbox_tmpl.c | 49 +++++++++++++++++++++++-------------------------- 1 file changed, 23 insertions(+), 26 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John [I can push this before a v3 would be necessary - more later]
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(-) diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 4aa5e9a63..2679b60f7 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. */ 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; + + /* it's ok to ignore failure here - e.g. it may be used by another VM */ + ignore_value(medium->vtbl->Close(medium)); + } + + cleanup: vboxArrayUnalloc(&media); return rc; } -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
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(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John [I can push this before a v3 would be necessary - more later]
Since the VBOX API requires to register an initial VM before proceeding to attach any remaining devices to it, any failure to attach such devices should result in automatic cleanup of the initially registered VM so that the state of VBOX registry remains clean without any leftover "aborted" VMs in it. Failure to cleanup of such partial VMs results in a warning log so that actual define error stays on the top of the error stack. --- src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 92ee37164..812c940e6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -1853,6 +1853,8 @@ 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; + bool machineReady = false; + virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); @@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags if (!data->vboxObj) return ret; - VBOX_IID_INITIALIZE(&mchiid); if (!(def = virDomainDefParseString(xml, data->caps, data->xmlopt, NULL, parse_flags))) { - goto cleanup; + return ret; } + VBOX_IID_INITIALIZE(&mchiid); virUUIDFormat(def->uuid, uuidstr); rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr); @@ -1959,30 +1961,41 @@ 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. + machineReady = true; + + cleanup: + /* Save the machine settings made till now, even when jumped here on error, + * as otherwise unregister won't cleanup properly. For example, it won't + * close media that were partially attached. The VBOX SDK docs say that + * unregister implicitly calls saveSettings but evidently it's not so... */ rc = gVBoxAPI.UIMachine.SaveSettings(machine); 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"), rc); + machineReady = false; } gVBoxAPI.UISession.Close(data->vboxSession); - vboxIIDUnalloc(&mchiid); - - ret = virGetDomain(conn, def->name, def->uuid, -1); - VBOX_RELEASE(machine); - virDomainDefFree(def); + if (machineReady) { + ret = virGetDomain(conn, def->name, def->uuid, -1); + } else { + /* Unregister incompletely configured VM to not leave garbage behind */ + rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine); - return ret; + if (NS_SUCCEEDED(rc)) + gVBoxAPI.deleteConfig(machine); + else + VIR_WARN("Could not cleanup partially created VM after failure, " + "rc=%08x", rc); + } - cleanup: + vboxIIDUnalloc(&mchiid); VBOX_RELEASE(machine); virDomainDefFree(def); - return NULL; + + return ret; } static virDomainPtr -- 2.14.2
On Tue, 2017-10-24 at 15:35 -0400, Dawid Zamirski wrote:
> Since the VBOX API requires to register an initial VM before
> proceeding
> to attach any remaining devices to it, any failure to attach such
> devices should result in automatic cleanup of the initially
> registered
> VM so that the state of VBOX registry remains clean without any
> leftover
> "aborted" VMs in it. Failure to cleanup of such partial VMs results
> in a
> warning log so that actual define error stays on the top of the error
> stack.
> ---
> src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++-------------
> -
> 1 file changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..812c940e6 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -1853,6 +1853,8 @@ 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;
> + bool machineReady = false;
> +
>
> virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
>
> @@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> const char *xml, unsigned int flags
> if (!data->vboxObj)
> return ret;
>
> - VBOX_IID_INITIALIZE(&mchiid);
> if (!(def = virDomainDefParseString(xml, data->caps, data-
> >xmlopt,
> NULL, parse_flags))) {
> - goto cleanup;
> + return ret;
> }
>
> + VBOX_IID_INITIALIZE(&mchiid);
> virUUIDFormat(def->uuid, uuidstr);
>
> rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine,
> uuidstr);
> @@ -1959,30 +1961,41 @@ 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.
> + machineReady = true;
> +
> + cleanup:
> + /* Save the machine settings made till now, even when jumped
> here on error,
> + * as otherwise unregister won't cleanup properly. For example,
> it won't
> + * close media that were partially attached. The VBOX SDK docs
> say that
> + * unregister implicitly calls saveSettings but evidently it's
> not so...
> */
> rc = gVBoxAPI.UIMachine.SaveSettings(machine);
There's one more code path in this function that causes a segfault here
due to machine == NULL (e.g. when CreateMachine fails). I already have
patched this but I'll hold off with sending V3 until this series is
reviewed and feedback is available.
>
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Since the VBOX API requires to register an initial VM before proceeding to attach any remaining devices to it, any failure to attach such devices should result in automatic cleanup of the initially registered VM so that the state of VBOX registry remains clean without any leftover "aborted" VMs in it. Failure to cleanup of such partial VMs results in a warning log so that actual define error stays on the top of the error stack. --- src/vbox/vbox_common.c | 41 +++++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 92ee37164..812c940e6 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -1853,6 +1853,8 @@ 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; + bool machineReady = false; +
virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL);
@@ -1862,12 +1864,12 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags if (!data->vboxObj) return ret;
- VBOX_IID_INITIALIZE(&mchiid); if (!(def = virDomainDefParseString(xml, data->caps, data->xmlopt, NULL, parse_flags))) { - goto cleanup; + return ret; }
Existing, but the { } here aren't necessary since there's only one statement after the if. It escapes the syntax-check check because there's two lines in the function call...
+ VBOX_IID_INITIALIZE(&mchiid); virUUIDFormat(def->uuid, uuidstr);
rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr);
And as I assume you know - having this go to cleanup is going to cause a problem since it's designed to clean up on failure assuming that this succeeded, right? I would have been OK with a posted diff to squash in... In any case, at this point only @def and @mchiid would need to be cleaned up. If you move the mchiid generation to after this line, then the failure scenario here could be virDomainDefFree(def) and return ret; (or NULL). Whichever way works - the rest of the code looks OK, but I can wait for the next version. Since Patches 1 & 2 are OK and for the most part Patches 4-9 are fine and would seem to be "movable" to just before current patch 10. That patch also adjusts vboxDomainDefineXMLFlags, so for me it became a logic point to at least be able to push some of the patches before you post a a v3. For the nits found in patch 4-9 - I can make the simple adjustments noted before pushing. That way the next series is smaller. John
@@ -1959,30 +1961,41 @@ 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. + machineReady = true; + + cleanup: + /* Save the machine settings made till now, even when jumped here on error, + * as otherwise unregister won't cleanup properly. For example, it won't + * close media that were partially attached. The VBOX SDK docs say that + * unregister implicitly calls saveSettings but evidently it's not so... */ rc = gVBoxAPI.UIMachine.SaveSettings(machine); 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"), rc); + machineReady = false; }
gVBoxAPI.UISession.Close(data->vboxSession); - vboxIIDUnalloc(&mchiid); - - ret = virGetDomain(conn, def->name, def->uuid, -1); - VBOX_RELEASE(machine);
- virDomainDefFree(def); + if (machineReady) { + ret = virGetDomain(conn, def->name, def->uuid, -1); + } else { + /* Unregister incompletely configured VM to not leave garbage behind */ + rc = gVBoxAPI.unregisterMachine(data, &mchiid, &machine);
- return ret; + if (NS_SUCCEEDED(rc)) + gVBoxAPI.deleteConfig(machine); + else + VIR_WARN("Could not cleanup partially created VM after failure, " + "rc=%08x", rc); + }
- cleanup: + vboxIIDUnalloc(&mchiid); VBOX_RELEASE(machine); virDomainDefFree(def); - return NULL; + + return ret; }
static virDomainPtr
Previously, the driver was computing VBOX's devicePort/deviceSlot values based on device name and max port/slot values. While this worked, it completely ignored <address> values. Additionally, libvirt's built-in virDomainDiskDefAssignAddress already does a good job setting default values on virDomainDeviceDriveAddress struct which we can use to set devicePort and deviceSlot and accomplish the same result while allowing the cusomizing those via XML. Also, this allows to remove some code which will make further patches smaller. --- src/vbox/vbox_common.c | 104 ++++--------------------------------------------- 1 file changed, 7 insertions(+), 97 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 812c940e6..fa8471e68 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -346,68 +346,6 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox, return true; } -/** - * function to get the StorageBus, Port number - * and Device number for the given devicename - * e.g: hda has StorageBus = IDE, port = 0, - * device = 0 - * - * @returns true on Success, false on failure. - * @param deviceName Input device name - * @param aMaxPortPerInst Input array of max port per device instance - * @param aMaxSlotPerPort Input array of max slot per device port - * @param storageBus Input storage bus type - * @param deviceInst Output device instance number - * @param devicePort Output port number - * @param deviceSlot Output slot number - * - */ -static bool vboxGetDeviceDetails(const char *deviceName, - PRUint32 *aMaxPortPerInst, - PRUint32 *aMaxSlotPerPort, - PRUint32 storageBus, - PRInt32 *deviceInst, - PRInt32 *devicePort, - PRInt32 *deviceSlot) -{ - int total = 0; - PRUint32 maxPortPerInst = 0; - PRUint32 maxSlotPerPort = 0; - - if (!deviceName || - !deviceInst || - !devicePort || - !deviceSlot || - !aMaxPortPerInst || - !aMaxSlotPerPort) - return false; - - if ((storageBus < StorageBus_IDE) || - (storageBus > StorageBus_Floppy)) - return false; - - total = virDiskNameToIndex(deviceName); - - maxPortPerInst = aMaxPortPerInst[storageBus]; - maxSlotPerPort = aMaxSlotPerPort[storageBus]; - - if (!maxPortPerInst || - !maxSlotPerPort || - (total < 0)) - return false; - - *deviceInst = total / (maxPortPerInst * maxSlotPerPort); - *devicePort = (total % (maxPortPerInst * maxSlotPerPort)) / maxSlotPerPort; - *deviceSlot = (total % (maxPortPerInst * maxSlotPerPort)) % maxSlotPerPort; - - VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, " - "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u", - deviceName, total, storageBus, *deviceInst, *devicePort, - *deviceSlot, maxPortPerInst, maxSlotPerPort); - - return true; -} - /** * function to generate the name for medium, * for e.g: hda, sda, etc @@ -1022,14 +960,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; nsresult rc = 0; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; PRUnichar *storageCtlName = NULL; - bool error = false; - - /* get the max port/slots/etc for the given storage bus */ - error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, - maxSlotPerPort); /* add a storage controller for the mediums to be attached */ /* this needs to change when multiple controller are supported for @@ -1071,7 +1002,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) VBOX_RELEASE(storageCtl); } - for (i = 0; i < def->ndisks && !error; i++) { + for (i = 0; i < def->ndisks; i++) { const char *src = virDomainDiskGetSource(def->disks[i]); int type = virDomainDiskGetType(def->disks[i]); int format = virDomainDiskGetFormat(def->disks[i]); @@ -1095,12 +1026,10 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) 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; + PRInt32 devicePort = def->disks[i]->info.addr.drive.unit; + PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus; VBOX_IID_INITIALIZE(&mediumUUID); VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16); @@ -1166,35 +1095,16 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) { VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); - storageBus = StorageBus_IDE; + devicePort = def->disks[i]->info.addr.drive.bus; + deviceSlot = def->disks[i]->info.addr.drive.unit; } 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; - } - - /* 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; + devicePort = 0; + deviceSlot = def->disks[i]->info.addr.drive.unit; } /* attach the harddisk/dvd/Floppy to the storage controller */ -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Previously, the driver was computing VBOX's devicePort/deviceSlot values based on device name and max port/slot values. While this worked, it completely ignored <address> values. Additionally, libvirt's built-in virDomainDiskDefAssignAddress already does a good job setting default values on virDomainDeviceDriveAddress struct which we can use to set devicePort and deviceSlot and accomplish the same result while allowing the cusomizing those via XML. Also, this allows to remove some code
customizing
which will make further patches smaller. --- src/vbox/vbox_common.c | 104 ++++--------------------------------------------- 1 file changed, 7 insertions(+), 97 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
This commit primes vboxAttachDrives for further changes so when they are made, the diff is less noisy: * move variable declarations to the top of the function * add disk variable to replace all the def->disks[i] instances * add cleanup at the end of the loop body, so it's all in one place rather than scattered through the loop body. It's purposefully called 'cleanup' rather than 'skip' or 'continue' because future commit will treat errors as hard-failures. --- src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++-------------------------- 1 file changed, 46 insertions(+), 49 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index fa8471e68..b949c4db7 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -959,8 +959,17 @@ static void vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; + int type, format; + const char *src = NULL; nsresult rc = 0; + virDomainDiskDefPtr disk = NULL; PRUnichar *storageCtlName = NULL; + IMedium *medium = NULL; + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL; + PRUint32 devicePort, deviceSlot, deviceType, accessMode; + vboxIID mediumUUID; + + VBOX_IID_INITIALIZE(&mediumUUID); /* add a storage controller for the mediums to be attached */ /* this needs to change when multiple controller are supported for @@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) } for (i = 0; i < def->ndisks; i++) { - const char *src = virDomainDiskGetSource(def->disks[i]); - int type = virDomainDiskGetType(def->disks[i]); - int format = virDomainDiskGetFormat(def->disks[i]); + disk = def->disks[i]; + src = virDomainDiskGetSource(disk); + type = virDomainDiskGetType(disk); + format = virDomainDiskGetFormat(disk); + deviceType = DeviceType_Null; + accessMode = AccessMode_ReadOnly; + devicePort = disk->info.addr.drive.unit; + deviceSlot = disk->info.addr.drive.bus; VIR_DEBUG("disk(%zu) type: %d", i, type); - VIR_DEBUG("disk(%zu) device: %d", i, def->disks[i]->device); - VIR_DEBUG("disk(%zu) bus: %d", i, def->disks[i]->bus); + VIR_DEBUG("disk(%zu) device: %d", i, disk->device); + VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus); VIR_DEBUG("disk(%zu) src: %s", i, src); - VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst); + VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst); VIR_DEBUG("disk(%zu) driverName: %s", i, - virDomainDiskGetDriver(def->disks[i])); + virDomainDiskGetDriver(disk)); VIR_DEBUG("disk(%zu) driverType: %s", i, virStorageFileFormatTypeToString(format)); - VIR_DEBUG("disk(%zu) cachemode: %d", i, def->disks[i]->cachemode); - VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]->src->readonly + VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode); + VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src->readonly ? "True" : "False")); - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->src->shared + VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src->shared ? "True" : "False")); if (type == VIR_STORAGE_TYPE_FILE && src) { - IMedium *medium = NULL; - vboxIID mediumUUID; - PRUnichar *mediumFileUtf16 = NULL; - PRUint32 deviceType = DeviceType_Null; - PRUint32 accessMode = AccessMode_ReadOnly; - PRInt32 devicePort = def->disks[i]->info.addr.drive.unit; - PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus; - - VBOX_IID_INITIALIZE(&mediumUUID); VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16); - if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { deviceType = DeviceType_HardDisk; accessMode = AccessMode_ReadWrite; - } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { deviceType = DeviceType_DVD; accessMode = AccessMode_ReadOnly; - } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { deviceType = DeviceType_Floppy; accessMode = AccessMode_ReadWrite; } else { - VBOX_UTF16_FREE(mediumFileUtf16); - continue; + goto cleanup; } gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16, deviceType, accessMode, &medium); if (!medium) { - PRUnichar *mediumEmpty = NULL; - VBOX_UTF8_TO_UTF16("", &mediumEmpty); rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, @@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!medium) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to attach the following disk/dvd/floppy " - "to the machine: %s, rc=%08x"), - src, (unsigned)rc); - VBOX_UTF16_FREE(mediumFileUtf16); - continue; + "to the machine: %s, rc=%08x"), src, rc); + 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; + src, rc); + goto cleanup; } - if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (def->disks[i]->src->readonly) { + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (disk->src->readonly) { gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable); - VIR_DEBUG("setting harddisk to immutable"); - } else if (!def->disks[i]->src->readonly) { + VIR_DEBUG("Setting harddisk to immutable"); + } else if (!disk->src->readonly) { gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal); - VIR_DEBUG("setting harddisk type to normal"); + VIR_DEBUG("Setting harddisk type to normal"); } } - if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) { VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); devicePort = def->disks[i]->info.addr.drive.bus; deviceSlot = def->disks[i]->info.addr.drive.unit; - } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) { + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) { VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName); - } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) { + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName); - } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) { + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName); devicePort = 0; - deviceSlot = def->disks[i]->info.addr.drive.unit; + deviceSlot = disk->info.addr.drive.unit; } /* attach the harddisk/dvd/Floppy to the storage controller */ @@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not attach the file as " - "harddisk/dvd/floppy: %s, rc=%08x"), - src, (unsigned)rc); + _("Could not attach the file as " + "harddisk/dvd/floppy: %s, rc=%08x"), src, rc); } else { DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID); } - + cleanup: VBOX_MEDIUM_RELEASE(medium); vboxIIDUnalloc(&mediumUUID); VBOX_UTF16_FREE(mediumFileUtf16); -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This commit primes vboxAttachDrives for further changes so when they are made, the diff is less noisy:
* move variable declarations to the top of the function * add disk variable to replace all the def->disks[i] instances * add cleanup at the end of the loop body, so it's all in one place rather than scattered through the loop body. It's purposefully called 'cleanup' rather than 'skip' or 'continue' because future commit will treat errors as hard-failures. --- src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++-------------------------- 1 file changed, 46 insertions(+), 49 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index fa8471e68..b949c4db7 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -959,8 +959,17 @@ static void vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; + int type, format; + const char *src = NULL; nsresult rc = 0; + virDomainDiskDefPtr disk = NULL; PRUnichar *storageCtlName = NULL; + IMedium *medium = NULL; + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL; + PRUint32 devicePort, deviceSlot, deviceType, accessMode; + vboxIID mediumUUID; + + VBOX_IID_INITIALIZE(&mediumUUID);
The cleanup: label would clean this up on the first pass through the loop, so it needs to move outside the last }. I can do this for you; however, I have a question - something I noticed while reviewing patch 7. There's a call: rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID); for which it wasn't clear whether the mediumUUID is overwritten. If it is, then although existing - it probably needs some cleanup and of course would change the flow a bit to doing that initializer each time through the loop. Moving it does have a ripple effect on subsequent patches, but it's manageable. If moving it to the end is acceptable, then Reviewed-by: John Ferlan <jferlan@redhat.com> otherwise let me know and I'll let you repost as part of a v3 John
/* add a storage controller for the mediums to be attached */ /* this needs to change when multiple controller are supported for @@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) }
for (i = 0; i < def->ndisks; i++) { - const char *src = virDomainDiskGetSource(def->disks[i]); - int type = virDomainDiskGetType(def->disks[i]); - int format = virDomainDiskGetFormat(def->disks[i]); + disk = def->disks[i]; + src = virDomainDiskGetSource(disk); + type = virDomainDiskGetType(disk); + format = virDomainDiskGetFormat(disk); + deviceType = DeviceType_Null; + accessMode = AccessMode_ReadOnly; + devicePort = disk->info.addr.drive.unit; + deviceSlot = disk->info.addr.drive.bus;
VIR_DEBUG("disk(%zu) type: %d", i, type); - VIR_DEBUG("disk(%zu) device: %d", i, def->disks[i]->device); - VIR_DEBUG("disk(%zu) bus: %d", i, def->disks[i]->bus); + VIR_DEBUG("disk(%zu) device: %d", i, disk->device); + VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus); VIR_DEBUG("disk(%zu) src: %s", i, src); - VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst); + VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst); VIR_DEBUG("disk(%zu) driverName: %s", i, - virDomainDiskGetDriver(def->disks[i])); + virDomainDiskGetDriver(disk)); VIR_DEBUG("disk(%zu) driverType: %s", i, virStorageFileFormatTypeToString(format)); - VIR_DEBUG("disk(%zu) cachemode: %d", i, def->disks[i]->cachemode); - VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]->src->readonly + VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode); + VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src->readonly ? "True" : "False")); - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->src->shared + VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src->shared ? "True" : "False"));
if (type == VIR_STORAGE_TYPE_FILE && src) { - IMedium *medium = NULL; - vboxIID mediumUUID; - PRUnichar *mediumFileUtf16 = NULL; - PRUint32 deviceType = DeviceType_Null; - PRUint32 accessMode = AccessMode_ReadOnly; - PRInt32 devicePort = def->disks[i]->info.addr.drive.unit; - PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus; - - VBOX_IID_INITIALIZE(&mediumUUID); VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { deviceType = DeviceType_HardDisk; accessMode = AccessMode_ReadWrite; - } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { deviceType = DeviceType_DVD; accessMode = AccessMode_ReadOnly; - } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { deviceType = DeviceType_Floppy; accessMode = AccessMode_ReadWrite; } else { - VBOX_UTF16_FREE(mediumFileUtf16); - continue; + goto cleanup; }
gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16, deviceType, accessMode, &medium);
if (!medium) { - PRUnichar *mediumEmpty = NULL; - VBOX_UTF8_TO_UTF16("", &mediumEmpty);
rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, @@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!medium) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to attach the following disk/dvd/floppy " - "to the machine: %s, rc=%08x"), - src, (unsigned)rc); - VBOX_UTF16_FREE(mediumFileUtf16); - continue; + "to the machine: %s, rc=%08x"), src, rc); + 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; + src, rc); + goto cleanup; }
- if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (def->disks[i]->src->readonly) { + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { + if (disk->src->readonly) { gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable); - VIR_DEBUG("setting harddisk to immutable"); - } else if (!def->disks[i]->src->readonly) { + VIR_DEBUG("Setting harddisk to immutable"); + } else if (!disk->src->readonly) { gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal); - VIR_DEBUG("setting harddisk type to normal"); + VIR_DEBUG("Setting harddisk type to normal"); } }
- if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) { + if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) { VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); devicePort = def->disks[i]->info.addr.drive.bus; deviceSlot = def->disks[i]->info.addr.drive.unit; - } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) { + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) { VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName); - } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) { + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName); - } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) { + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName); devicePort = 0; - deviceSlot = def->disks[i]->info.addr.drive.unit; + deviceSlot = disk->info.addr.drive.unit; }
/* attach the harddisk/dvd/Floppy to the storage controller */ @@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not attach the file as " - "harddisk/dvd/floppy: %s, rc=%08x"), - src, (unsigned)rc); + _("Could not attach the file as " + "harddisk/dvd/floppy: %s, rc=%08x"), src, rc); } else { DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID); } - + cleanup: VBOX_MEDIUM_RELEASE(medium); vboxIIDUnalloc(&mediumUUID); VBOX_UTF16_FREE(mediumFileUtf16);
On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
>
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> > This commit primes vboxAttachDrives for further changes so when
> > they
> > are made, the diff is less noisy:
> >
> > * move variable declarations to the top of the function
> > * add disk variable to replace all the def->disks[i] instances
> > * add cleanup at the end of the loop body, so it's all in one place
> > rather than scattered through the loop body. It's purposefully
> > called 'cleanup' rather than 'skip' or 'continue' because future
> > commit will treat errors as hard-failures.
> > ---
> > src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++------------
> > --------------
> > 1 file changed, 46 insertions(+), 49 deletions(-)
> >
> > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> > index fa8471e68..b949c4db7 100644
> > --- a/src/vbox/vbox_common.c
> > +++ b/src/vbox/vbox_common.c
> > @@ -959,8 +959,17 @@ static void
> > vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> > *machine)
> > {
> > size_t i;
> > + int type, format;
> > + const char *src = NULL;
> > nsresult rc = 0;
> > + virDomainDiskDefPtr disk = NULL;
> > PRUnichar *storageCtlName = NULL;
> > + IMedium *medium = NULL;
> > + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
> > + PRUint32 devicePort, deviceSlot, deviceType, accessMode;
> > + vboxIID mediumUUID;
> > +
> > + VBOX_IID_INITIALIZE(&mediumUUID);
>
> The cleanup: label would clean this up on the first pass through the
> loop, so it needs to move outside the last }.
>
The VBOX_IID_INITIALIZE macro is a simple struct initializer that is
equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact,
src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly
that, but that macro is not available to vbox_common.c This is likely
another remanent of VBOX <= 3 support that needs to be cleaned up - I
will do so in a separate patch as it will touch to many parts of the
driver and when you take a closer look we don't really need vboxIID
struct at all and can simply use PRUnichar directly for VBOX UUIDs.
> I can do this for you; however, I have a question - something I
> noticed
> while reviewing patch 7. There's a call:
>
> rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
>
> for which it wasn't clear whether the mediumUUID is overwritten. If
> it
> is, then although existing - it probably needs some cleanup and of
> course would change the flow a bit to doing that initializer each
> time
> through the loop.
Yes, it is overwritten but since the cleanup: label is at the end of
the loop body it will set mediumUIID to {NULL, true} which is the same
as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in
src/vbox/vbox_tmpl.c)
>
> Moving it does have a ripple effect on subsequent patches, but it's
> manageable.
>
> If moving it to the end is acceptable, then
As per above, I'd rather leave this patch "as is" as the (future)
driver cleanup patches for vboxIID will make this clearer.
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
> otherwise let me know and I'll let you repost as part of a v3
>
> John
>
> >
> > /* add a storage controller for the mediums to be attached */
> > /* this needs to change when multiple controller are supported
> > for
> > @@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > }
> >
> > for (i = 0; i < def->ndisks; i++) {
> > - const char *src = virDomainDiskGetSource(def->disks[i]);
> > - int type = virDomainDiskGetType(def->disks[i]);
> > - int format = virDomainDiskGetFormat(def->disks[i]);
> > + disk = def->disks[i];
> > + src = virDomainDiskGetSource(disk);
> > + type = virDomainDiskGetType(disk);
> > + format = virDomainDiskGetFormat(disk);
> > + deviceType = DeviceType_Null;
> > + accessMode = AccessMode_ReadOnly;
> > + devicePort = disk->info.addr.drive.unit;
> > + deviceSlot = disk->info.addr.drive.bus;
> >
> > VIR_DEBUG("disk(%zu) type: %d", i, type);
> > - VIR_DEBUG("disk(%zu) device: %d", i, def->disks[i]-
> > >device);
> > - VIR_DEBUG("disk(%zu) bus: %d", i, def->disks[i]-
> > >bus);
> > + VIR_DEBUG("disk(%zu) device: %d", i, disk->device);
> > + VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus);
> > VIR_DEBUG("disk(%zu) src: %s", i, src);
> > - VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]-
> > >dst);once vboxIID will (probably) be removed in separate patches
> > at some point in the future, I'd rather leave this patch "as is"
> > for now.
> > + VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst);
> > VIR_DEBUG("disk(%zu) driverName: %s", i,
> > - virDomainDiskGetDriver(def->disks[i]));
> > + virDomainDiskGetDriver(disk));
> > VIR_DEBUG("disk(%zu) driverType: %s", i,
> > virStorageFileFormatTypeToString(format));
> > - VIR_DEBUG("disk(%zu) cachemode: %d", i, def->disks[i]-
> > >cachemode);
> > - VIR_DEBUG("disk(%zu) readonly: %s", i, (def->disks[i]-
> > >src->readonly
> > + VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode);
> > + VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src-
> > >readonly
> > ? "True" : "False"));
> > - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]-
> > >src->shared
> > + VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src-
> > >shared
> > ? "True" : "False"));
> >
> > if (type == VIR_STORAGE_TYPE_FILE && src) {
> > - IMedium *medium = NULL;
> > - vboxIID mediumUUID;
> > - PRUnichar *mediumFileUtf16 = NULL;
> > - PRUint32 deviceType = DeviceType_Null;
> > - PRUint32 accessMode = AccessMode_ReadOnly;
> > - PRInt32 devicePort = def->disks[i]-
> > >info.addr.drive.unit;
> > - PRInt32 deviceSlot = def->disks[i]-
> > >info.addr.drive.bus;
> > -
> > - VBOX_IID_INITIALIZE(&mediumUUID);
> > VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
> >
> > - if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_DISK) {
> > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> > deviceType = DeviceType_HardDisk;
> > accessMode = AccessMode_ReadWrite;
> > - } else if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_CDROM) {
> > + } else if (disk->device ==
> > VIR_DOMAIN_DISK_DEVICE_CDROM) {
> > deviceType = DeviceType_DVD;
> > accessMode = AccessMode_ReadOnly;
> > - } else if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> > + } else if (disk->device ==
> > VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> > deviceType = DeviceType_Floppy;
> > accessMode = AccessMode_ReadWrite;
> > } else {
> > - VBOX_UTF16_FREE(mediumFileUtf16);
> > - continue;
> > + goto cleanup;
> > }
> >
> > gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj,
> > mediumFileUtf16,
> > deviceType,
> > accessMode, &medium);
> >
> > if (!medium) {
> > - PRUnichar *mediumEmpty = NULL;
> > -
> > VBOX_UTF8_TO_UTF16("", &mediumEmpty);
> >
> > rc = gVBoxAPI.UIVirtualBox.OpenMedium(data-
> > >vboxObj,
> > @@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > if (!medium) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Failed to attach the following
> > disk/dvd/floppy "
> > - "to the machine: %s, rc=%08x"),
> > - src, (unsigned)rc);
> > - VBOX_UTF16_FREE(mediumFileUtf16);
> > - continue;
> > + "to the machine: %s, rc=%08x"),
> > src, rc);
> > + 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;
> > + src, rc);
> > + goto cleanup;
> > }
> >
> > - if (def->disks[i]->device ==
> > VIR_DOMAIN_DISK_DEVICE_DISK) {
> > - if (def->disks[i]->src->readonly) {
> > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> > + if (disk->src->readonly) {
> > gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Immutable);
> > - VIR_DEBUG("setting harddisk to immutable");
> > - } else if (!def->disks[i]->src->readonly) {
> > + VIR_DEBUG("Setting harddisk to immutable");
> > + } else if (!disk->src->readonly) {
> > gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Normal);
> > - VIR_DEBUG("setting harddisk type to normal");
> > + VIR_DEBUG("Setting harddisk type to normal");
> > }
> > }
> >
> > - if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> > + if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> > VBOX_UTF8_TO_UTF16("IDE Controller",
> > &storageCtlName);
> > devicePort = def->disks[i]->info.addr.drive.bus;
> > deviceSlot = def->disks[i]->info.addr.drive.unit;
> > - } else if (def->disks[i]->bus ==
> > VIR_DOMAIN_DISK_BUS_SATA) {
> > + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) {
> > VBOX_UTF8_TO_UTF16("SATA Controller",
> > &storageCtlName);
> > - } else if (def->disks[i]->bus ==
> > VIR_DOMAIN_DISK_BUS_SCSI) {
> > + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
> > VBOX_UTF8_TO_UTF16("SCSI Controller",
> > &storageCtlName);
> > - } else if (def->disks[i]->bus ==
> > VIR_DOMAIN_DISK_BUS_FDC) {
> > + } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
> > VBOX_UTF8_TO_UTF16("Floppy Controller",
> > &storageCtlName);
> > devicePort = 0;
> > - deviceSlot = def->disks[i]->info.addr.drive.unit;
> > + deviceSlot = disk->info.addr.drive.unit;
> > }
> >
> > /* attach the harddisk/dvd/Floppy to the storage
> > controller */
> > @@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >
> > if (NS_FAILED(rc)) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > - _("could not attach the file as "
> > - "harddisk/dvd/floppy: %s,
> > rc=%08x"),
> > - src, (unsigned)rc);
> > + _("Could not attach the file as "
> > + "harddisk/dvd/floppy: %s,
> > rc=%08x"), src, rc);
> > } else {
> > DEBUGIID("Attached HDD/DVD/Floppy with UUID",
> > &mediumUUID);
> > }
> > -
> > + cleanup:
> > VBOX_MEDIUM_RELEASE(medium);
> > vboxIIDUnalloc(&mediumUUID);
> > VBOX_UTF16_FREE(mediumFileUtf16);
> >
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
On 11/03/2017 12:19 PM, Dawid Zamirski wrote:
On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This commit primes vboxAttachDrives for further changes so when they are made, the diff is less noisy:
* move variable declarations to the top of the function * add disk variable to replace all the def->disks[i] instances * add cleanup at the end of the loop body, so it's all in one place rather than scattered through the loop body. It's purposefully called 'cleanup' rather than 'skip' or 'continue' because future commit will treat errors as hard-failures. --- src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++------------ -------------- 1 file changed, 46 insertions(+), 49 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index fa8471e68..b949c4db7 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -959,8 +959,17 @@ static void vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; + int type, format; + const char *src = NULL; nsresult rc = 0; + virDomainDiskDefPtr disk = NULL; PRUnichar *storageCtlName = NULL; + IMedium *medium = NULL; + PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL; + PRUint32 devicePort, deviceSlot, deviceType, accessMode; + vboxIID mediumUUID; + + VBOX_IID_INITIALIZE(&mediumUUID);
The cleanup: label would clean this up on the first pass through the loop, so it needs to move outside the last }.
The VBOX_IID_INITIALIZE macro is a simple struct initializer that is equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact, src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly that, but that macro is not available to vbox_common.c This is likely another remanent of VBOX <= 3 support that needs to be cleaned up - I will do so in a separate patch as it will touch to many parts of the driver and when you take a closer look we don't really need vboxIID struct at all and can simply use PRUnichar directly for VBOX UUIDs.
OK it really wasn't overly intuitively obvious what VBOX_IID_INITIALIZE a/k/a gVBoxAPI.UIID.vboxIIDInitialize really did, your explanation makes sense.
I can do this for you; however, I have a question - something I noticed while reviewing patch 7. There's a call:
rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
for which it wasn't clear whether the mediumUUID is overwritten. If it is, then although existing - it probably needs some cleanup and of course would change the flow a bit to doing that initializer each time through the loop.
Yes, it is overwritten but since the cleanup: label is at the end of the loop body it will set mediumUIID to {NULL, true} which is the same as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in src/vbox/vbox_tmpl.c)
Moving it does have a ripple effect on subsequent patches, but it's manageable.
If moving it to the end is acceptable, then
As per above, I'd rather leave this patch "as is" as the (future) driver cleanup patches for vboxIID will make this clearer.
OK I'll leave it as is. Patches 1-2 and 4-9 are thus all R-B and will be pushed in a little while. John
Reviewed-by: John Ferlan <jferlan@redhat.com>
otherwise let me know and I'll let you repost as part of a v3
John
[...]
Previously, if one tried to define a VBOX VM and the API failed to perform the requested actions for some reason, it would just log the error and move on to process remaining disk definitions. This is not desired as it could result in incorrectly defined VM without the caller even knowing about it. So now all the code paths that call virReportError are now treated as hard failures as they should have been. --- src/vbox/vbox_common.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index b949c4db7..9f4bf18a3 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -955,17 +955,17 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data, } } -static void +static int vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; - int type, format; + int type, format, ret = 0; const char *src = NULL; nsresult rc = 0; virDomainDiskDefPtr disk = NULL; PRUnichar *storageCtlName = NULL; IMedium *medium = NULL; - PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL; + PRUnichar *mediumFileUtf16 = NULL; PRUint32 devicePort, deviceSlot, deviceType, accessMode; vboxIID mediumUUID; @@ -1049,6 +1049,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) deviceType = DeviceType_Floppy; accessMode = AccessMode_ReadWrite; } else { + ret = -1; goto cleanup; } @@ -1056,19 +1057,17 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) deviceType, accessMode, &medium); if (!medium) { - VBOX_UTF8_TO_UTF16("", &mediumEmpty); - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, mediumFileUtf16, deviceType, accessMode, &medium); - VBOX_UTF16_FREE(mediumEmpty); } if (!medium) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to attach the following disk/dvd/floppy " "to the machine: %s, rc=%08x"), src, rc); + ret = -1; goto cleanup; } @@ -1078,6 +1077,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) _("Can't get the UUID of the file to be attached " "as harddisk/dvd/floppy: %s, rc=%08x"), src, rc); + ret = -1; goto cleanup; } @@ -1117,6 +1117,8 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not attach the file as " "harddisk/dvd/floppy: %s, rc=%08x"), src, rc); + ret = -1; + goto cleanup; } else { DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID); } @@ -1125,8 +1127,13 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) vboxIIDUnalloc(&mediumUUID); VBOX_UTF16_FREE(mediumFileUtf16); VBOX_UTF16_FREE(storageCtlName); + + if (ret < 0) + break; } } + + return ret; } static void @@ -1857,7 +1864,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine); vboxSetBootDeviceOrder(def, data, machine); - vboxAttachDrives(def, data, machine); + if (vboxAttachDrives(def, data, machine) < 0) + goto cleanup; vboxAttachSound(def, machine); if (vboxAttachNetwork(def, data, machine) < 0) goto cleanup; -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Previously, if one tried to define a VBOX VM and the API failed to perform the requested actions for some reason, it would just log the error and move on to process remaining disk definitions. This is not desired as it could result in incorrectly defined VM without the caller even knowing about it. So now all the code paths that call virReportError are now treated as hard failures as they should have been. --- src/vbox/vbox_common.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index b949c4db7..9f4bf18a3 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -955,17 +955,17 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data, } }
-static void +static int vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; - int type, format; + int type, format, ret = 0; const char *src = NULL; nsresult rc = 0; virDomainDiskDefPtr disk = NULL; PRUnichar *storageCtlName = NULL; IMedium *medium = NULL; - PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL; + PRUnichar *mediumFileUtf16 = NULL; PRUint32 devicePort, deviceSlot, deviceType, accessMode; vboxIID mediumUUID;
@@ -1049,6 +1049,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) deviceType = DeviceType_Floppy; accessMode = AccessMode_ReadWrite; } else { + ret = -1; goto cleanup; }
@@ -1056,19 +1057,17 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) deviceType, accessMode, &medium);
if (!medium) { - VBOX_UTF8_TO_UTF16("", &mediumEmpty); - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj, mediumFileUtf16, deviceType, accessMode, &medium); - VBOX_UTF16_FREE(mediumEmpty); }
The mediumEmpty changes could have gone into the previous patch or in a separate patch. It's unused as of commit 34364df3 which should have never copied it in from the "old" code which ended up getting removed as part of commit c7c286c6. I can extract it into a separate patch before pushing... Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
Original code was checking for non empty disk source before proceeding to actually attach disk device to VM. This prevented from creating empty removable devices like DVD or floppy. Therefore, this patch re-organizes the loop work-flow to allow such configurations as well as makes the code follow better libvirt practices. Additionally, adjusted debug logs to be more helpful - removed old ones and added new which give more valuable info for troubleshooting. --- src/vbox/vbox_common.c | 206 +++++++++++++++++++++++++++++++------------------ 1 file changed, 130 insertions(+), 76 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9f4bf18a3..2bd891efb 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -959,11 +959,12 @@ static int vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; - int type, format, ret = 0; + int type, ret = 0; const char *src = NULL; nsresult rc = 0; virDomainDiskDefPtr disk = NULL; PRUnichar *storageCtlName = NULL; + char *controllerName = NULL; IMedium *medium = NULL; PRUnichar *mediumFileUtf16 = NULL; PRUint32 devicePort, deviceSlot, deviceType, accessMode; @@ -1015,47 +1016,104 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) disk = def->disks[i]; src = virDomainDiskGetSource(disk); type = virDomainDiskGetType(disk); - format = virDomainDiskGetFormat(disk); deviceType = DeviceType_Null; accessMode = AccessMode_ReadOnly; devicePort = disk->info.addr.drive.unit; deviceSlot = disk->info.addr.drive.bus; - VIR_DEBUG("disk(%zu) type: %d", i, type); - VIR_DEBUG("disk(%zu) device: %d", i, disk->device); - VIR_DEBUG("disk(%zu) bus: %d", i, disk->bus); - VIR_DEBUG("disk(%zu) src: %s", i, src); - VIR_DEBUG("disk(%zu) dst: %s", i, disk->dst); - VIR_DEBUG("disk(%zu) driverName: %s", i, - virDomainDiskGetDriver(disk)); - VIR_DEBUG("disk(%zu) driverType: %s", i, - virStorageFileFormatTypeToString(format)); - VIR_DEBUG("disk(%zu) cachemode: %d", i, disk->cachemode); - VIR_DEBUG("disk(%zu) readonly: %s", i, (disk->src->readonly - ? "True" : "False")); - VIR_DEBUG("disk(%zu) shared: %s", i, (disk->src->shared - ? "True" : "False")); - - if (type == VIR_STORAGE_TYPE_FILE && src) { - 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)); + ret = -1; + goto cleanup; + } - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - 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 { + switch ((virDomainDiskDevice) disk->device) { + case VIR_DOMAIN_DISK_DEVICE_DISK: + if (!src) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Missing disk source file path")); ret = -1; goto cleanup; } - gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16, - deviceType, accessMode, &medium); + deviceType = DeviceType_HardDisk; + accessMode = AccessMode_ReadWrite; + + break; + + case VIR_DOMAIN_DISK_DEVICE_CDROM: + deviceType = DeviceType_DVD; + accessMode = AccessMode_ReadOnly; + + break; + case VIR_DOMAIN_DISK_DEVICE_FLOPPY: + deviceType = DeviceType_Floppy; + accessMode = AccessMode_ReadWrite; + + break; + case VIR_DOMAIN_DISK_DEVICE_LUN: + case VIR_DOMAIN_DISK_DEVICE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s disk device"), + virDomainDiskDeviceTypeToString(disk->device)); + ret = -1; + goto cleanup; + } + + switch ((virDomainDiskBus) disk->bus) { + case VIR_DOMAIN_DISK_BUS_IDE: + VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); + devicePort = def->disks[i]->info.addr.drive.bus; + deviceSlot = def->disks[i]->info.addr.drive.unit; + + break; + case VIR_DOMAIN_DISK_BUS_SATA: + VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName); + + break; + case VIR_DOMAIN_DISK_BUS_SCSI: + VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName); + + break; + case VIR_DOMAIN_DISK_BUS_FDC: + VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName); + devicePort = 0; + deviceSlot = disk->info.addr.drive.unit; + + break; + case VIR_DOMAIN_DISK_BUS_VIRTIO: + case VIR_DOMAIN_DISK_BUS_XEN: + case VIR_DOMAIN_DISK_BUS_USB: + case VIR_DOMAIN_DISK_BUS_UML: + case VIR_DOMAIN_DISK_BUS_SD: + case VIR_DOMAIN_DISK_BUS_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s bus type"), + virDomainDiskBusTypeToString(disk->bus)); + ret = -1; + goto cleanup; + } + /* If disk source is specified, lookup IMedium - removable drives don't + * have either. + */ + if (src) { + VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16); + VIR_DEBUG("Looking up medium %s, type: %d, mode: %d", src, + deviceType, accessMode); + + rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16, + deviceType, accessMode, &medium); + + /* The following is not needed for vbox 4.2+ but older versions have + * distinct find and open operations where the former looks in vbox + * media registry while the 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, @@ -1065,7 +1123,7 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!medium) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Failed to attach the following disk/dvd/floppy " + _("Failed to open the following disk/dvd/floppy " "to the machine: %s, rc=%08x"), src, rc); ret = -1; goto cleanup; @@ -1080,57 +1138,53 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) ret = -1; goto cleanup; } + } - if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - if (disk->src->readonly) { - gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable); - VIR_DEBUG("Setting harddisk to immutable"); - } else if (!disk->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); + VIR_DEBUG("Setting hard disk to immutable"); + } else if (!disk->src->readonly) { + gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal); + VIR_DEBUG("Setting hard disk type to normal"); } - if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) { - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); - devicePort = def->disks[i]->info.addr.drive.bus; - deviceSlot = def->disks[i]->info.addr.drive.unit; - } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) { - VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName); - } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) { - VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName); - } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { - VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName); - devicePort = 0; - deviceSlot = disk->info.addr.drive.unit; - } + } - /* attach the harddisk/dvd/Floppy to the storage controller */ - rc = gVBoxAPI.UIMachine.AttachDevice(machine, - storageCtlName, - devicePort, - deviceSlot, - deviceType, - medium); + VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName); + VIR_DEBUG("Attaching disk(%zu), controller: %s, port: %d, slot: %d, " + "type: %d, medium: %s", i, controllerName, devicePort, + deviceSlot, deviceType, medium == NULL ? "empty" : src); + VBOX_UTF8_FREE(controllerName); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not attach the file as " - "harddisk/dvd/floppy: %s, rc=%08x"), src, rc); - ret = -1; - goto cleanup; - } else { - DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID); - } - cleanup: - VBOX_MEDIUM_RELEASE(medium); - vboxIIDUnalloc(&mediumUUID); - VBOX_UTF16_FREE(mediumFileUtf16); - VBOX_UTF16_FREE(storageCtlName); + /* Attach the harddisk/dvd/Floppy to the storage controller, + * medium == NULL is ok here + */ + rc = gVBoxAPI.UIMachine.AttachDevice(machine, + storageCtlName, + devicePort, + deviceSlot, + deviceType, + medium); - if (ret < 0) - break; + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not attach the file as " + "harddisk/dvd/floppy: %s, rc=%08x"), src, rc); + ret = -1; + goto cleanup; + } else { + DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID); } + + cleanup: + VBOX_MEDIUM_RELEASE(medium); + vboxIIDUnalloc(&mediumUUID); + VBOX_UTF16_FREE(mediumFileUtf16); + VBOX_UTF16_FREE(storageCtlName); + + if (ret < 0) + break; } return ret; -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Original code was checking for non empty disk source before proceeding to actually attach disk device to VM. This prevented from creating empty removable devices like DVD or floppy. Therefore, this patch re-organizes the loop work-flow to allow such configurations as well as makes the code follow better libvirt practices. Additionally, adjusted debug logs to be more helpful - removed old ones and added new which give more valuable info for troubleshooting. --- src/vbox/vbox_common.c | 206 +++++++++++++++++++++++++++++++------------------ 1 file changed, 130 insertions(+), 76 deletions(-)
There's a lot going on here, but I'm not sure how much is separable other than perhaps the debug stuff, so even though there's a lot going on ... Reviewed-by: John Ferlan <jferlan@redhat.com> John
This patch exposes additional methods of the native VBOX API to the libvirt 'unified' vbox API to deal with IStorageController. The exposed methods are: * IStorageController->GetStorageControllerType() * IStorageController->SetStorageControllerType() * IMachine->GetStorageControllers() --- src/vbox/vbox_common.h | 13 +++++++++++++ src/vbox/vbox_tmpl.c | 20 ++++++++++++++++++++ src/vbox/vbox_uniformed_api.h | 3 +++ 3 files changed, 36 insertions(+) diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h index c6da8929d..b08ad1e3e 100644 --- a/src/vbox/vbox_common.h +++ b/src/vbox/vbox_common.h @@ -235,6 +235,19 @@ enum StorageBus StorageBus_SAS = 5 }; +enum StorageControllerType +{ + StorageControllerType_Null = 0, + StorageControllerType_LsiLogic = 1, + StorageControllerType_BusLogic = 2, + StorageControllerType_IntelAhci = 3, + StorageControllerType_PIIX3 = 4, + StorageControllerType_PIIX4 = 5, + StorageControllerType_ICH6 = 6, + StorageControllerType_I82078 = 7, + StorageControllerType_LsiLogicSas = 8 +}; + enum AccessMode { AccessMode_ReadOnly = 1, diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index 2679b60f7..a25f14c09 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -550,6 +550,11 @@ static void* _handleUSBGetDeviceFilters(IUSBCommon *USBCommon) return USBCommon->vtbl->GetDeviceFilters; } +static void* _handleMachineGetStorageControllers(IMachine *machine) +{ + return machine->vtbl->GetStorageControllers; +} + static void* _handleMachineGetMediumAttachments(IMachine *machine) { return machine->vtbl->GetMediumAttachments; @@ -1916,6 +1921,18 @@ _storageControllerGetBus(IStorageController *storageController, PRUint32 *bus) return storageController->vtbl->GetBus(storageController, bus); } +static nsresult +_storageControllerGetControllerType(IStorageController *storageController, PRUint32 *controllerType) +{ + return storageController->vtbl->GetControllerType(storageController, controllerType); +} + +static nsresult +_storageControllerSetControllerType(IStorageController *storageController, PRUint32 controllerType) +{ + return storageController->vtbl->SetControllerType(storageController, controllerType); +} + static nsresult _sharedFolderGetHostPath(ISharedFolder *sharedFolder, PRUnichar **hostPath) { @@ -2265,6 +2282,7 @@ static vboxUniformedArray _UArray = { .handleGetMachines = _handleGetMachines, .handleGetHardDisks = _handleGetHardDisks, .handleUSBGetDeviceFilters = _handleUSBGetDeviceFilters, + .handleMachineGetStorageControllers = _handleMachineGetStorageControllers, .handleMachineGetMediumAttachments = _handleMachineGetMediumAttachments, .handleMachineGetSharedFolders = _handleMachineGetSharedFolders, .handleSnapshotGetChildren = _handleSnapshotGetChildren, @@ -2496,6 +2514,8 @@ static vboxUniformedIMediumAttachment _UIMediumAttachment = { static vboxUniformedIStorageController _UIStorageController = { .GetBus = _storageControllerGetBus, + .GetControllerType = _storageControllerGetControllerType, + .SetControllerType = _storageControllerSetControllerType, }; static vboxUniformedISharedFolder _UISharedFolder = { diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h index 2ccaf43e8..dc0b391b2 100644 --- a/src/vbox/vbox_uniformed_api.h +++ b/src/vbox/vbox_uniformed_api.h @@ -135,6 +135,7 @@ typedef struct { void* (*handleGetMachines)(IVirtualBox *vboxObj); void* (*handleGetHardDisks)(IVirtualBox *vboxObj); void* (*handleUSBGetDeviceFilters)(IUSBCommon *USBCommon); + void* (*handleMachineGetStorageControllers)(IMachine *machine); void* (*handleMachineGetMediumAttachments)(IMachine *machine); void* (*handleMachineGetSharedFolders)(IMachine *machine); void* (*handleSnapshotGetChildren)(ISnapshot *snapshot); @@ -410,6 +411,8 @@ typedef struct { /* Functions for IStorageController */ typedef struct { nsresult (*GetBus)(IStorageController *storageController, PRUint32 *bus); + nsresult (*SetControllerType)(IStorageController *storageController, PRUint32 controllerType); + nsresult (*GetControllerType)(IStorageController *storageController, PRUint32 *controllerType); } vboxUniformedIStorageController; /* Functions for ISharedFolder */ -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This patch exposes additional methods of the native VBOX API to the libvirt 'unified' vbox API to deal with IStorageController. The exposed methods are:
* IStorageController->GetStorageControllerType() * IStorageController->SetStorageControllerType() * IMachine->GetStorageControllers() --- src/vbox/vbox_common.h | 13 +++++++++++++ src/vbox/vbox_tmpl.c | 20 ++++++++++++++++++++ src/vbox/vbox_uniformed_api.h | 3 +++ 3 files changed, 36 insertions(+)
Reviewed-by: John Ferlan <jferlan@redhat.com> John
The optional values are 'piix3', 'piix4' or 'ich6'. Those will be needed to allow setting IDE controller model in VirtualBox driver. --- docs/formatdomain.html.in | 4 ++++ docs/schemas/domaincommon.rng | 18 ++++++++++++++++-- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 40 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 4609e2ec2..8dff685ad 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -3648,6 +3648,10 @@ <span class="since">Since 1.3.5</span>, USB controllers accept a <code>ports</code> attribute to configure how many devices can be connected to the controller.</dd> + <dt><code>ide</code></dt> + <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> </dl> <p> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 710b3af7f..9cec1a063 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1975,12 +1975,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> @@ -2041,6 +2040,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 77c20c697..57f291624 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -377,6 +377,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", @@ -9785,6 +9790,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; } @@ -9800,6 +9807,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 38de70b15..0def905b2 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 || \ @@ -3219,6 +3227,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 448d962b2..2e67366b7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -234,6 +234,8 @@ virDomainControllerFindUnusedIndex; virDomainControllerInsert; virDomainControllerInsertPreAlloced; virDomainControllerIsPSeriesPHB; +virDomainControllerModelIDETypeFromString; +virDomainControllerModelIDETypeToString; virDomainControllerModelPCITypeToString; virDomainControllerModelSCSITypeFromString; virDomainControllerModelSCSITypeToString; -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
The optional values are 'piix3', 'piix4' or 'ich6'. Those will be needed to allow setting IDE controller model in VirtualBox driver. --- docs/formatdomain.html.in | 4 ++++ docs/schemas/domaincommon.rng | 18 ++++++++++++++++-- src/conf/domain_conf.c | 9 +++++++++ src/conf/domain_conf.h | 9 +++++++++ src/libvirt_private.syms | 2 ++ 5 files changed, 40 insertions(+), 2 deletions(-)
Reviewed-by: John Ferlan <jferlan@redhat.com> John NB: Since 3.9 is already generated, I can modify formatdomain.html.in to list 3.10
This patch enables the VBOX driver to process the <controller> element in domain XML through which one can now customize the controller model. Since VirtualBox has two distinct SAS and SCSI they do not "map" directly to libvirt XML, he VBOX driver uses <contoller type="scsi" model="lsisas1068" /> to create SAS controller in VBOX VM. Additionally once can set model on the IDE controller to be one of "piix3", "piix4" or "ich6". --- src/vbox/vbox_common.c | 214 ++++++++++++++++++++++++++++++++++++++----------- src/vbox/vbox_common.h | 8 ++ 2 files changed, 176 insertions(+), 46 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 2bd891efb..9d45e4a76 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -406,6 +406,160 @@ static char *vboxGenerateMediumName(PRUint32 storageBus, return name; } + +static int +vboxSetStorageController(virDomainControllerDefPtr controller, + vboxDriverPtr data, + IMachine *machine) +{ + PRUnichar *controllerName = NULL; + PRInt32 vboxModel = StorageControllerType_Null; + PRInt32 vboxBusType = StorageBus_Null; + IStorageController *vboxController = NULL; + nsresult rc = 0; + char *debugName = NULL; + int ret = -1; + + /* libvirt controller type => vbox bus type */ + switch ((virDomainControllerType) controller->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &controllerName); + vboxBusType = StorageBus_Floppy; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &controllerName); + vboxBusType = StorageBus_IDE; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &controllerName); + vboxBusType = StorageBus_SCSI; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &controllerName); + vboxBusType = StorageBus_SATA; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s controller type"), + virDomainControllerTypeToString(controller->type)); + return -1; + } + + /* libvirt scsi model => vbox scsi model */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + switch ((virDomainControllerModelSCSI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: + vboxModel = StorageControllerType_LsiLogic; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: + vboxModel = StorageControllerType_BusLogic; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + /* in vbox, lsisas has a dedicated SAS bus type with no model */ + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &controllerName); + vboxBusType = StorageBus_SAS; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s SCSI " + "controller model"), + virDomainControllerModelSCSITypeToString(controller->model)); + goto cleanup; + } + /* libvirt ide model => vbox ide model */ + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { + switch ((virDomainControllerModelIDE) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3: + vboxModel = StorageControllerType_PIIX3; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4: + vboxModel = StorageControllerType_PIIX4; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6: + vboxModel = StorageControllerType_ICH6; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s IDE " + "controller model"), + virDomainControllerModelIDETypeToString(controller->model)); + goto cleanup; + } + } + + VBOX_UTF16_TO_UTF8(controllerName, &debugName); + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType: %d)", + debugName, vboxBusType); + + rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName, + vboxBusType, &vboxController); + + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to add storage controller " + "(name: %s, busType: %d), rc=%08x"), + debugName, vboxBusType, rc); + goto cleanup; + } + + /* only IDE or SCSI controller have model choices */ + if (vboxModel != StorageControllerType_Null) { + rc = gVBoxAPI.UIStorageController.SetControllerType(vboxController, + vboxModel); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to change storage controller model, " + "rc=%08x"), rc); + goto cleanup; + } + } + + ret = 0; + + cleanup: + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(debugName); + VBOX_RELEASE(vboxController); + + return ret; +} + + +static int +vboxAttachStorageControllers(virDomainDefPtr def, + vboxDriverPtr data, + IMachine *machine) +{ + size_t i; + for (i = 0; i < def->ncontrollers; i++) { + if (vboxSetStorageController(def->controllers[i], data, machine) < 0) + return -1; + } + + return 0; +} + + static virDrvOpenStatus vboxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -959,7 +1113,7 @@ static int vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; - int type, ret = 0; + int type, ret = 0, model = -1; const char *src = NULL; nsresult rc = 0; virDomainDiskDefPtr disk = NULL; @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) VBOX_IID_INITIALIZE(&mediumUUID); - /* add a storage controller for the mediums to be attached */ - /* this needs to change when multiple controller are supported for - * ver > 3.1 */ - { - IStorageController *storageCtl = NULL; - PRUnichar *sName = NULL; - - VBOX_UTF8_TO_UTF16("IDE Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_IDE, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - - VBOX_UTF8_TO_UTF16("SATA Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_SATA, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - - VBOX_UTF8_TO_UTF16("SCSI Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_SCSI, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - - VBOX_UTF8_TO_UTF16("Floppy Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_Floppy, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - } - for (i = 0; i < def->ndisks; i++) { disk = def->disks[i]; src = virDomainDiskGetSource(disk); @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &storageCtlName); devicePort = def->disks[i]->info.addr.drive.bus; deviceSlot = def->disks[i]->info.addr.drive.unit; break; case VIR_DOMAIN_DISK_BUS_SATA: - VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &storageCtlName); break; case VIR_DOMAIN_DISK_BUS_SCSI: - VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName); + + model = virDomainDeviceFindControllerModel(def, &disk->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { + VBOX_UTF16_FREE(storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName); + } break; case VIR_DOMAIN_DISK_BUS_FDC: - VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName); devicePort = 0; deviceSlot = disk->info.addr.drive.unit; @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal); VIR_DEBUG("Setting hard disk type to normal"); } - } VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName); @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine); vboxSetBootDeviceOrder(def, data, machine); + if (vboxAttachStorageControllers(def, data, machine) < 0) + goto cleanup; if (vboxAttachDrives(def, data, machine) < 0) goto cleanup; vboxAttachSound(def, machine); diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h index b08ad1e3e..3340374c1 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; -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This patch enables the VBOX driver to process the <controller> element in domain XML through which one can now customize the controller model.
Since VirtualBox has two distinct SAS and SCSI they do not "map" directly to libvirt XML, he VBOX driver uses <contoller type="scsi" model="lsisas1068" /> to create SAS controller in VBOX VM. Additionally once can set model on the IDE controller to be one of "piix3", "piix4" or "ich6". --- src/vbox/vbox_common.c | 214 ++++++++++++++++++++++++++++++++++++++----------- src/vbox/vbox_common.h | 8 ++ 2 files changed, 176 insertions(+), 46 deletions(-)
So beyond patch 3 which I know you have to repost anyway - starting with this patch and going forward, I figure you have to repost anyway as long as I get the question in patch 5 answered of course... This patch left me with a concern. Up to this point vboxAttachDrives would add at least one controller for each type supported regardless of whether the VM used it. After this patch only those controllers defined in the VM XML will be added. Which would seem to be fine, except for the case of hotplug which I wasn't clear whether vbox support or not. Let's say the VM is defined with and IDE controller, if someone tried to add a SCSI disk after startup, then there'd be no SCSI controller already present.
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 2bd891efb..9d45e4a76 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -406,6 +406,160 @@ static char *vboxGenerateMediumName(PRUint32 storageBus, return name; }
+ +static int +vboxSetStorageController(virDomainControllerDefPtr controller, + vboxDriverPtr data, + IMachine *machine) +{ + PRUnichar *controllerName = NULL; + PRInt32 vboxModel = StorageControllerType_Null; + PRInt32 vboxBusType = StorageBus_Null; + IStorageController *vboxController = NULL; + nsresult rc = 0; + char *debugName = NULL; + int ret = -1; + + /* libvirt controller type => vbox bus type */ + switch ((virDomainControllerType) controller->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &controllerName); + vboxBusType = StorageBus_Floppy; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &controllerName); + vboxBusType = StorageBus_IDE; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &controllerName); + vboxBusType = StorageBus_SCSI; + + break; + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &controllerName); + vboxBusType = StorageBus_SATA;
I think prior to this patch - there should be a patch that "manages" all those range check differences, storageBus comparison checks, etc. for VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches. John
+ + break; + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s controller type"), + virDomainControllerTypeToString(controller->type)); + return -1; + } + + /* libvirt scsi model => vbox scsi model */ + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { + switch ((virDomainControllerModelSCSI) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: + vboxModel = StorageControllerType_LsiLogic; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: + vboxModel = StorageControllerType_BusLogic; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + /* in vbox, lsisas has a dedicated SAS bus type with no model */ + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &controllerName); + vboxBusType = StorageBus_SAS; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s SCSI " + "controller model"), + virDomainControllerModelSCSITypeToString(controller->model)); + goto cleanup; + } + /* libvirt ide model => vbox ide model */ + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) { + switch ((virDomainControllerModelIDE) controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3: + vboxModel = StorageControllerType_PIIX3; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4: + vboxModel = StorageControllerType_PIIX4; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6: + vboxModel = StorageControllerType_ICH6; + + break; + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("The vbox driver does not support %s IDE " + "controller model"), + virDomainControllerModelIDETypeToString(controller->model)); + goto cleanup; + } + } + + VBOX_UTF16_TO_UTF8(controllerName, &debugName); + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType: %d)", + debugName, vboxBusType); + + rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName, + vboxBusType, &vboxController); + + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to add storage controller " + "(name: %s, busType: %d), rc=%08x"), + debugName, vboxBusType, rc); + goto cleanup; + } + + /* only IDE or SCSI controller have model choices */ + if (vboxModel != StorageControllerType_Null) { + rc = gVBoxAPI.UIStorageController.SetControllerType(vboxController, + vboxModel); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to change storage controller model, " + "rc=%08x"), rc); + goto cleanup; + } + } + + ret = 0; + + cleanup: + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(debugName); + VBOX_RELEASE(vboxController); + + return ret; +} + + +static int +vboxAttachStorageControllers(virDomainDefPtr def, + vboxDriverPtr data, + IMachine *machine) +{ + size_t i; + for (i = 0; i < def->ncontrollers; i++) { + if (vboxSetStorageController(def->controllers[i], data, machine) < 0) + return -1; + } + + return 0; +} + + static virDrvOpenStatus vboxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, @@ -959,7 +1113,7 @@ static int vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { size_t i; - int type, ret = 0; + int type, ret = 0, model = -1; const char *src = NULL; nsresult rc = 0; virDomainDiskDefPtr disk = NULL; @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
VBOX_IID_INITIALIZE(&mediumUUID);
- /* add a storage controller for the mediums to be attached */ - /* this needs to change when multiple controller are supported for - * ver > 3.1 */ - { - IStorageController *storageCtl = NULL; - PRUnichar *sName = NULL; - - VBOX_UTF8_TO_UTF16("IDE Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_IDE, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - - VBOX_UTF8_TO_UTF16("SATA Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_SATA, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - - VBOX_UTF8_TO_UTF16("SCSI Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_SCSI, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - - VBOX_UTF8_TO_UTF16("Floppy Controller", &sName); - gVBoxAPI.UIMachine.AddStorageController(machine, - sName, - StorageBus_Floppy, - &storageCtl); - VBOX_UTF16_FREE(sName); - VBOX_RELEASE(storageCtl); - } - for (i = 0; i < def->ndisks; i++) { disk = def->disks[i]; src = virDomainDiskGetSource(disk); @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
switch ((virDomainDiskBus) disk->bus) { case VIR_DOMAIN_DISK_BUS_IDE: - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, &storageCtlName); devicePort = def->disks[i]->info.addr.drive.bus; deviceSlot = def->disks[i]->info.addr.drive.unit;
break; case VIR_DOMAIN_DISK_BUS_SATA: - VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, &storageCtlName);
break; case VIR_DOMAIN_DISK_BUS_SCSI: - VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName); + + model = virDomainDeviceFindControllerModel(def, &disk->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { + VBOX_UTF16_FREE(storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, &storageCtlName); + }
break; case VIR_DOMAIN_DISK_BUS_FDC: - VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName); + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName); devicePort = 0; deviceSlot = disk->info.addr.drive.unit;
@@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal); VIR_DEBUG("Setting hard disk type to normal"); } - }
VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName); @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, const char *xml, unsigned int flags gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
vboxSetBootDeviceOrder(def, data, machine); + if (vboxAttachStorageControllers(def, data, machine) < 0) + goto cleanup; if (vboxAttachDrives(def, data, machine) < 0) goto cleanup; vboxAttachSound(def, machine); diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h index b08ad1e3e..3340374c1 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;
On Fri, 2017-11-03 at 09:51 -0400, John Ferlan wrote:
>
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> > This patch enables the VBOX driver to process the <controller>
> > element
> > in domain XML through which one can now customize the controller
> > model.
> >
> > Since VirtualBox has two distinct SAS and SCSI they do not "map"
> > directly to libvirt XML, he VBOX driver uses <contoller type="scsi"
> > model="lsisas1068" /> to create SAS controller in VBOX VM.
> > Additionally
> > once can set model on the IDE controller to be one of "piix3",
> > "piix4"
> > or "ich6".
> > ---
> > src/vbox/vbox_common.c | 214
> > ++++++++++++++++++++++++++++++++++++++-----------
> > src/vbox/vbox_common.h | 8 ++
> > 2 files changed, 176 insertions(+), 46 deletions(-)
> >
>
> So beyond patch 3 which I know you have to repost anyway - starting
> with
> this patch and going forward, I figure you have to repost anyway as
> long
> as I get the question in patch 5 answered of course...
>
> This patch left me with a concern. Up to this point vboxAttachDrives
> would add at least one controller for each type supported regardless
> of
> whether the VM used it.
>
> After this patch only those controllers defined in the VM XML will be
> added. Which would seem to be fine, except for the case of hotplug
> which
> I wasn't clear whether vbox support or not.
>
> Let's say the VM is defined with and IDE controller, if someone tried
> to
> add a SCSI disk after startup, then there'd be no SCSI controller
> already present.
>
I've just checked this with VirutalBox GUI and the only thing that can
be changed on live VM is removable media (CD/DVD). All options to add
disk devices to controller are disabled when VM is running. Neither
quick web search returned anything useful regarding hot-add disk
support on VBOX.
Having said that, the original behavior was, IIRC, the reason why I
actually started working on this series. According to our tech support
they have ran into a couple of cases where the OS would BSOD (some
legacy Windows OS e.g. 2000 or XP) when there was a controller (without
a disk) attached to VM that the OS did not handle well. It sounded very
strange to me but I've been told it was not a one-off case though I
personally haven't reproduced it. Unfortunately, I won't be able to
provide more details as we no longer use VBox internally (migrated to
KVM) so those potential test cases are no longer around.
>
> > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> > index 2bd891efb..9d45e4a76 100644
> > --- a/src/vbox/vbox_common.c
> > +++ b/src/vbox/vbox_common.c
> > @@ -406,6 +406,160 @@ static char *vboxGenerateMediumName(PRUint32
> > storageBus,
> > return name;
> > }
> >
> > +
> > +static int
> > +vboxSetStorageController(virDomainControllerDefPtr controller,
> > + vboxDriverPtr data,
> > + IMachine *machine)
> > +{
> > + PRUnichar *controllerName = NULL;
> > + PRInt32 vboxModel = StorageControllerType_Null;
> > + PRInt32 vboxBusType = StorageBus_Null;
> > + IStorageController *vboxController = NULL;
> > + nsresult rc = 0;
> > + char *debugName = NULL;
> > + int ret = -1;
> > +
> > + /* libvirt controller type => vbox bus type */
> > + switch ((virDomainControllerType) controller->type) {
> > + case VIR_DOMAIN_CONTROLLER_TYPE_FDC:
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_Floppy;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_TYPE_IDE:
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_IDE;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_SCSI;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_SATA;
>
>
> I think prior to this patch - there should be a patch that "manages"
> all
> those range check differences, storageBus comparison checks, etc. for
> VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches.
>
> John
>
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("The vbox driver does not support %s
> > controller type"),
> > + virDomainControllerTypeToString(controller-
> > >type));
> > + return -1;
> > + }
> > +
> > + /* libvirt scsi model => vbox scsi model */
> > + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> > + switch ((virDomainControllerModelSCSI) controller->model)
> > {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
> > + vboxModel = StorageControllerType_LsiLogic;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
> > + vboxModel = StorageControllerType_BusLogic;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
> > + /* in vbox, lsisas has a dedicated SAS bus type with
> > no model */
> > + VBOX_UTF16_FREE(controllerName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_SAS;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("The vbox driver does not support %s
> > SCSI "
> > + "controller model"),
> > + virDomainControllerModelSCSITypeToStrin
> > g(controller->model));
> > + goto cleanup;
> > + }
> > + /* libvirt ide model => vbox ide model */
> > + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > {
> > + switch ((virDomainControllerModelIDE) controller->model) {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3:
> > + vboxModel = StorageControllerType_PIIX3;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4:
> > + vboxModel = StorageControllerType_PIIX4;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6:
> > + vboxModel = StorageControllerType_ICH6;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("The vbox driver does not support %s
> > IDE "
> > + "controller model"),
> > + virDomainControllerModelIDETypeToStri
> > ng(controller->model));
> > + goto cleanup;
> > + }
> > + }
> > +
> > + VBOX_UTF16_TO_UTF8(controllerName, &debugName);
> > + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType:
> > %d)",
> > + debugName, vboxBusType);
> > +
> > + rc = gVBoxAPI.UIMachine.AddStorageController(machine,
> > controllerName,
> > + vboxBusType,
> > &vboxController);
> > +
> > + if (NS_FAILED(rc)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to add storage controller "
> > + "(name: %s, busType: %d), rc=%08x"),
> > + debugName, vboxBusType, rc);
> > + goto cleanup;
> > + }
> > +
> > + /* only IDE or SCSI controller have model choices */
> > + if (vboxModel != StorageControllerType_Null) {
> > + rc =
> > gVBoxAPI.UIStorageController.SetControllerType(vboxController,
> > + vboxMo
> > del);
> > + if (NS_FAILED(rc)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to change storage controller
> > model, "
> > + "rc=%08x"), rc);
> > + goto cleanup;
> > + }
> > + }
> > +
> > + ret = 0;
> > +
> > + cleanup:
> > + VBOX_UTF16_FREE(controllerName);
> > + VBOX_UTF8_FREE(debugName);
> > + VBOX_RELEASE(vboxController);
> > +
> > + return ret;
> > +}
> > +
> > +
> > +static int
> > +vboxAttachStorageControllers(virDomainDefPtr def,
> > + vboxDriverPtr data,
> > + IMachine *machine)
> > +{
> > + size_t i;
> > + for (i = 0; i < def->ncontrollers; i++) {
> > + if (vboxSetStorageController(def->controllers[i], data,
> > machine) < 0)
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > static virDrvOpenStatus
> > vboxConnectOpen(virConnectPtr conn,
> > virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> > @@ -959,7 +1113,7 @@ static int
> > vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> > *machine)
> > {
> > size_t i;
> > - int type, ret = 0;
> > + int type, ret = 0, model = -1;
> > const char *src = NULL;
> > nsresult rc = 0;
> > virDomainDiskDefPtr disk = NULL;
> > @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >
> > VBOX_IID_INITIALIZE(&mediumUUID);
> >
> > - /* add a storage controller for the mediums to be attached */
> > - /* this needs to change when multiple controller are supported
> > for
> > - * ver > 3.1 */
> > - {
> > - IStorageController *storageCtl = NULL;
> > - PRUnichar *sName = NULL;
> > -
> > - VBOX_UTF8_TO_UTF16("IDE Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_IDE,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > -
> > - VBOX_UTF8_TO_UTF16("SATA Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_SATA,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > -
> > - VBOX_UTF8_TO_UTF16("SCSI Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_SCSI,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > -
> > - VBOX_UTF8_TO_UTF16("Floppy Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_Floppy,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > - }
> > -
> > for (i = 0; i < def->ndisks; i++) {
> > disk = def->disks[i];
> > src = virDomainDiskGetSource(disk);
> > @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >
> > switch ((virDomainDiskBus) disk->bus) {
> > case VIR_DOMAIN_DISK_BUS_IDE:
> > - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> > &storageCtlName);
> > devicePort = def->disks[i]->info.addr.drive.bus;
> > deviceSlot = def->disks[i]->info.addr.drive.unit;
> >
> > break;
> > case VIR_DOMAIN_DISK_BUS_SATA:
> > - VBOX_UTF8_TO_UTF16("SATA Controller",
> > &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &storageCtlName);
> >
> > break;
> > case VIR_DOMAIN_DISK_BUS_SCSI:
> > - VBOX_UTF8_TO_UTF16("SCSI Controller",
> > &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &storageCtlName);
> > +
> > + model = virDomainDeviceFindControllerModel(def, &disk-
> > >info,
> > + VIR_DOMAIN_
> > CONTROLLER_TYPE_SCSI);
> > + if (model ==
> > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> > + VBOX_UTF16_FREE(storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &storageCtlName);
> > + }
> >
> > break;
> > case VIR_DOMAIN_DISK_BUS_FDC:
> > - VBOX_UTF8_TO_UTF16("Floppy Controller",
> > &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> > &storageCtlName);
> > devicePort = 0;
> > deviceSlot = disk->info.addr.drive.unit;
> >
> > @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Normal);
> > VIR_DEBUG("Setting hard disk type to normal");
> > }
> > -
> > }
> >
> > VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
> > @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> > const char *xml, unsigned int flags
> > gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
> >
> > vboxSetBootDeviceOrder(def, data, machine);
> > + if (vboxAttachStorageControllers(def, data, machine) < 0)
> > + goto cleanup;
> > if (vboxAttachDrives(def, data, machine) < 0)
> > goto cleanup;
> > vboxAttachSound(def, machine);
> > diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> > index b08ad1e3e..3340374c1 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;
> >
On Fri, 2017-11-03 at 09:51 -0400, John Ferlan wrote:
>
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> >
> > + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI:
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_SCSI;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_TYPE_SATA:
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_SATA;
>
>
> I think prior to this patch - there should be a patch that "manages"
> all
> those range check differences, storageBus comparison checks, etc. for
> VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches.
>
> John
Up until this patch, the series modifies the definexml functionality in
which case input is libvirt XML that is translated to vbox values. The
supported libvirt types are 'validated' by the typed switch...case
statement so it's not possible to run into a vbox value that the driver
does not recognize. The patches that follow are for dumpxml and those
do get updated to handle StorageBus_SAS (the new one introduced in this
series) in patch #12 (see vboxGenerateMediumName change) and in patch
#15 where vboxDumpDisks is changed to use typed switch statement on
storageBus.
Dawid
>
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_CCID:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_USB:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_PCI:
> > + case VIR_DOMAIN_CONTROLLER_TYPE_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("The vbox driver does not support %s
> > controller type"),
> > + virDomainControllerTypeToString(controller-
> > >type));
> > + return -1;
> > + }
> > +
> > + /* libvirt scsi model => vbox scsi model */
> > + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> > + switch ((virDomainControllerModelSCSI) controller->model)
> > {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO:
> > + vboxModel = StorageControllerType_LsiLogic;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC:
> > + vboxModel = StorageControllerType_BusLogic;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068:
> > + /* in vbox, lsisas has a dedicated SAS bus type with
> > no model */
> > + VBOX_UTF16_FREE(controllerName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &controllerName);
> > + vboxBusType = StorageBus_SAS;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078:
> > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("The vbox driver does not support %s
> > SCSI "
> > + "controller model"),
> > + virDomainControllerModelSCSITypeToStrin
> > g(controller->model));
> > + goto cleanup;
> > + }
> > + /* libvirt ide model => vbox ide model */
> > + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE)
> > {
> > + switch ((virDomainControllerModelIDE) controller->model) {
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3:
> > + vboxModel = StorageControllerType_PIIX3;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4:
> > + vboxModel = StorageControllerType_PIIX4;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6:
> > + vboxModel = StorageControllerType_ICH6;
> > +
> > + break;
> > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST:
> > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > + _("The vbox driver does not support %s
> > IDE "
> > + "controller model"),
> > + virDomainControllerModelIDETypeToStri
> > ng(controller->model));
> > + goto cleanup;
> > + }
> > + }
> > +
> > + VBOX_UTF16_TO_UTF8(controllerName, &debugName);
> > + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType:
> > %d)",
> > + debugName, vboxBusType);
> > +
> > + rc = gVBoxAPI.UIMachine.AddStorageController(machine,
> > controllerName,
> > + vboxBusType,
> > &vboxController);
> > +
> > + if (NS_FAILED(rc)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to add storage controller "
> > + "(name: %s, busType: %d), rc=%08x"),
> > + debugName, vboxBusType, rc);
> > + goto cleanup;
> > + }
> > +
> > + /* only IDE or SCSI controller have model choices */
> > + if (vboxModel != StorageControllerType_Null) {
> > + rc =
> > gVBoxAPI.UIStorageController.SetControllerType(vboxController,
> > + vboxMo
> > del);
> > + if (NS_FAILED(rc)) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Failed to change storage controller
> > model, "
> > + "rc=%08x"), rc);
> > + goto cleanup;
> > + }
> > + }
> > +
> > + ret = 0;
> > +
> > + cleanup:
> > + VBOX_UTF16_FREE(controllerName);
> > + VBOX_UTF8_FREE(debugName);
> > + VBOX_RELEASE(vboxController);
> > +
> > + return ret;
> > +}
> > +
> > +
> > +static int
> > +vboxAttachStorageControllers(virDomainDefPtr def,
> > + vboxDriverPtr data,
> > + IMachine *machine)
> > +{
> > + size_t i;
> > + for (i = 0; i < def->ncontrollers; i++) {
> > + if (vboxSetStorageController(def->controllers[i], data,
> > machine) < 0)
> > + return -1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +
> > static virDrvOpenStatus
> > vboxConnectOpen(virConnectPtr conn,
> > virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> > @@ -959,7 +1113,7 @@ static int
> > vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine
> > *machine)
> > {
> > size_t i;
> > - int type, ret = 0;
> > + int type, ret = 0, model = -1;
> > const char *src = NULL;
> > nsresult rc = 0;
> > virDomainDiskDefPtr disk = NULL;
> > @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >
> > VBOX_IID_INITIALIZE(&mediumUUID);
> >
> > - /* add a storage controller for the mediums to be attached */
> > - /* this needs to change when multiple controller are supported
> > for
> > - * ver > 3.1 */
> > - {
> > - IStorageController *storageCtl = NULL;
> > - PRUnichar *sName = NULL;
> > -
> > - VBOX_UTF8_TO_UTF16("IDE Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_IDE,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > -
> > - VBOX_UTF8_TO_UTF16("SATA Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_SATA,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > -
> > - VBOX_UTF8_TO_UTF16("SCSI Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_SCSI,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > -
> > - VBOX_UTF8_TO_UTF16("Floppy Controller", &sName);
> > - gVBoxAPI.UIMachine.AddStorageController(machine,
> > - sName,
> > - StorageBus_Floppy,
> > - &storageCtl);
> > - VBOX_UTF16_FREE(sName);
> > - VBOX_RELEASE(storageCtl);
> > - }
> > -
> > for (i = 0; i < def->ndisks; i++) {
> > disk = def->disks[i];
> > src = virDomainDiskGetSource(disk);
> > @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> >
> > switch ((virDomainDiskBus) disk->bus) {
> > case VIR_DOMAIN_DISK_BUS_IDE:
> > - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME,
> > &storageCtlName);
> > devicePort = def->disks[i]->info.addr.drive.bus;
> > deviceSlot = def->disks[i]->info.addr.drive.unit;
> >
> > break;
> > case VIR_DOMAIN_DISK_BUS_SATA:
> > - VBOX_UTF8_TO_UTF16("SATA Controller",
> > &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME,
> > &storageCtlName);
> >
> > break;
> > case VIR_DOMAIN_DISK_BUS_SCSI:
> > - VBOX_UTF8_TO_UTF16("SCSI Controller",
> > &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME,
> > &storageCtlName);
> > +
> > + model = virDomainDeviceFindControllerModel(def, &disk-
> > >info,
> > + VIR_DOMAIN_
> > CONTROLLER_TYPE_SCSI);
> > + if (model ==
> > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) {
> > + VBOX_UTF16_FREE(storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME,
> > &storageCtlName);
> > + }
> >
> > break;
> > case VIR_DOMAIN_DISK_BUS_FDC:
> > - VBOX_UTF8_TO_UTF16("Floppy Controller",
> > &storageCtlName);
> > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME,
> > &storageCtlName);
> > devicePort = 0;
> > deviceSlot = disk->info.addr.drive.unit;
> >
> > @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > gVBoxAPI.UIMedium.SetType(medium,
> > MediumType_Normal);
> > VIR_DEBUG("Setting hard disk type to normal");
> > }
> > -
> > }
> >
> > VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName);
> > @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn,
> > const char *xml, unsigned int flags
> > gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine);
> >
> > vboxSetBootDeviceOrder(def, data, machine);
> > + if (vboxAttachStorageControllers(def, data, machine) < 0)
> > + goto cleanup;
> > if (vboxAttachDrives(def, data, machine) < 0)
> > goto cleanup;
> > vboxAttachSound(def, machine);
> > diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h
> > index b08ad1e3e..3340374c1 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;
> >
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--- src/vbox/vbox_common.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9d45e4a76..715eb670e 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3153,6 +3153,123 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr def, IMachine *mach goto release_filters; } + +static int +vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine) +{ + vboxArray storageControllers = VBOX_ARRAY_INITIALIZER; + IStorageController *controller = NULL; + PRUint32 storageBus = StorageBus_Null; + PRUint32 controllerType = StorageControllerType_Null; + virDomainControllerDefPtr cont = NULL; + size_t i = 0; + int model = -1, ret = -1; + virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST; + + gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine, + gVBoxAPI.UArray.handleMachineGetStorageControllers(machine)); + + for (i = 0; i < storageControllers.count; i++) { + controller = storageControllers.items[i]; + storageBus = StorageBus_Null; + controllerType = StorageControllerType_Null; + type = VIR_DOMAIN_CONTROLLER_TYPE_LAST; + model = -1; + + if (!controller) + continue; + + gVBoxAPI.UIStorageController.GetBus(controller, &storageBus); + gVBoxAPI.UIStorageController.GetControllerType(controller, + &controllerType); + + /* vbox controller model => libvirt controller model */ + switch ((enum StorageControllerType) 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; + case StorageControllerType_BusLogic: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC; + + break; + case StorageControllerType_LsiLogic: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC; + + break; + case StorageControllerType_LsiLogicSas: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068; + + break; + case StorageControllerType_IntelAhci: + case StorageControllerType_I82078: + case StorageControllerType_Null: + model = -1; + + break; + } + + /* vbox controller bus => libvirt controller type */ + switch ((enum StorageBus) storageBus) { + case StorageBus_IDE: + type = VIR_DOMAIN_CONTROLLER_TYPE_IDE; + + break; + case StorageBus_SCSI: + case StorageBus_SAS: + type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; + + break; + case StorageBus_SATA: + type = VIR_DOMAIN_CONTROLLER_TYPE_SATA; + + break; + case StorageBus_Floppy: + type = VIR_DOMAIN_CONTROLLER_TYPE_FDC; + + break; + case StorageBus_Null: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unsupported null storage bus")); + + goto cleanup; + } + + if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) { + cont = virDomainDefAddController(def, type, -1, model); + if (!cont) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to add %s controller type definition"), + virDomainControllerTypeToString(type)); + goto cleanup; + } + } + } + + ret = 0; + + cleanup: + gVBoxAPI.UArray.vboxArrayRelease(&storageControllers); + + if (ret < 0) { + for (i = 0; i < def->ncontrollers; i++) + virDomainControllerDefFree(def->controllers[i]); + VIR_FREE(def->controllers); + def->ncontrollers = 0; + } + + return ret; +} + + static void vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { @@ -4000,6 +4117,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; if (vboxDumpDisplay(def, data, machine) < 0) goto cleanup; + if (vboxDumpStorageControllers(def, machine) < 0) + goto cleanup; vboxDumpIDEHDDs(def, data, machine); -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
--- src/vbox/vbox_common.c | 119 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9d45e4a76..715eb670e 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3153,6 +3153,123 @@ vboxHostDeviceGetXMLDesc(vboxDriverPtr data, virDomainDefPtr def, IMachine *mach goto release_filters; }
+ +static int +vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine) +{ + vboxArray storageControllers = VBOX_ARRAY_INITIALIZER; + IStorageController *controller = NULL; + PRUint32 storageBus = StorageBus_Null; + PRUint32 controllerType = StorageControllerType_Null; + virDomainControllerDefPtr cont = NULL; + size_t i = 0; + int model = -1, ret = -1; + virDomainControllerType type = VIR_DOMAIN_CONTROLLER_TYPE_LAST; + + gVBoxAPI.UArray.vboxArrayGet(&storageControllers, machine, + gVBoxAPI.UArray.handleMachineGetStorageControllers(machine)); + + for (i = 0; i < storageControllers.count; i++) { + controller = storageControllers.items[i]; + storageBus = StorageBus_Null; + controllerType = StorageControllerType_Null; + type = VIR_DOMAIN_CONTROLLER_TYPE_LAST; + model = -1; + + if (!controller) + continue; + + gVBoxAPI.UIStorageController.GetBus(controller, &storageBus); + gVBoxAPI.UIStorageController.GetControllerType(controller, + &controllerType); + + /* vbox controller model => libvirt controller model */ + switch ((enum StorageControllerType) 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; + case StorageControllerType_BusLogic: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC; + + break; + case StorageControllerType_LsiLogic: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC; + + break; + case StorageControllerType_LsiLogicSas: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068; + + break; + case StorageControllerType_IntelAhci: + case StorageControllerType_I82078: + case StorageControllerType_Null: + model = -1; + + break; + } + + /* vbox controller bus => libvirt controller type */ + switch ((enum StorageBus) storageBus) { + case StorageBus_IDE: + type = VIR_DOMAIN_CONTROLLER_TYPE_IDE; + + break; + case StorageBus_SCSI: + case StorageBus_SAS: + type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI; + + break; + case StorageBus_SATA: + type = VIR_DOMAIN_CONTROLLER_TYPE_SATA; + + break; + case StorageBus_Floppy: + type = VIR_DOMAIN_CONTROLLER_TYPE_FDC; + + break; + case StorageBus_Null: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unsupported null storage bus")); + + goto cleanup; + } + + if (type != VIR_DOMAIN_CONTROLLER_TYPE_LAST) { + cont = virDomainDefAddController(def, type, -1, model); + if (!cont) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to add %s controller type definition"), + virDomainControllerTypeToString(type)); + goto cleanup; + } + } + } + + ret = 0; + + cleanup: + gVBoxAPI.UArray.vboxArrayRelease(&storageControllers); + + if (ret < 0) { + for (i = 0; i < def->ncontrollers; i++) + virDomainControllerDefFree(def->controllers[i]); + VIR_FREE(def->controllers); + def->ncontrollers = 0;
Although not necessarily a problem, since we're erroring out anyway, eventually when virDomainDefFree is called, this will be cleaned out anyway, so it can be removed... Remember that vboxDomainGetXMLDesc won't call virDomainDefFormat and it'll return NULL With that adjustment, Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ } + + return ret; +} + + static void vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { @@ -4000,6 +4117,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; if (vboxDumpDisplay(def, data, machine) < 0) goto cleanup; + if (vboxDumpStorageControllers(def, machine) < 0) + goto cleanup;
vboxDumpIDEHDDs(def, data, machine);
If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML generated by dumpxml used to produce "sda" for both of those disks. This is an invalid domain XML as libvirt does not allow duplicate device names. To address this, keep the running total of disks that will use "sd" prefix for device name and pass it to the vboxGenerateMediumName which no longer tries to "compute" the value based only on current and max port and slot values. After this the vboxGetMaxPortSlotValues is not needed and was deleted. --- src/vbox/vbox_common.c | 414 +++++++++++++++++++++---------------------------- 1 file changed, 177 insertions(+), 237 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 715eb670e..9dc36a1b2 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -290,61 +290,6 @@ 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_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_Floppy, - &maxSlotPerPort[StorageBus_Floppy]); - - VBOX_RELEASE(sysProps); - - return true; -} /** * function to generate the name for medium, @@ -352,57 +297,40 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox, * * @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) { 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_Floppy)) + (storageBus > StorageBus_SAS)) return NULL; - maxPortPerInst = aMaxPortPerInst[storageBus]; - maxSlotPerPort = aMaxSlotPerPort[storageBus]; - total = (deviceInst * maxPortPerInst * maxSlotPerPort) - + (devicePort * maxSlotPerPort) - + deviceSlot; - if (storageBus == StorageBus_IDE) { prefix = "hd"; - } else if ((storageBus == StorageBus_SATA) || - (storageBus == StorageBus_SCSI)) { + 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; } @@ -3270,20 +3198,17 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine) } -static void -vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) +static int +vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { - /* dump IDE hdds if present */ vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; - bool error = false; + size_t sdCount = 0, i; int diskCount = 0; - size_t i; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int ret = -1; def->ndisks = 0; gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, - gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine)); + gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine)); /* get the number of attachments */ for (i = 0; i < mediumAttachments.count; i++) { @@ -3300,24 +3225,19 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) } /* 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; - } - def->disks[i] = disk; - } - } else { - error = true; - } + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) + goto cleanup; - if (!error) - error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + if (!disk) + goto cleanup; + + def->disks[i] = disk; + } /* get the attachment details here */ - for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) { + for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { IMediumAttachment *imediumattach = mediumAttachments.items[i]; IStorageController *storageController = NULL; PRUnichar *storageControllerName = NULL; @@ -3327,7 +3247,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) IMedium *medium = NULL; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; @@ -3363,16 +3282,36 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!virDomainDiskGetSource(def->disks[diskCount])) { VBOX_RELEASE(medium); VBOX_RELEASE(storageController); - error = true; - break; + + goto cleanup; } gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); + gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); + gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + + def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + devicePort, + deviceSlot, + sdCount); + if (!def->disks[diskCount]->dst) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not generate medium name for the disk " + "at: port:%d, slot:%d"), devicePort, deviceSlot); + VBOX_RELEASE(medium); + VBOX_RELEASE(storageController); + + goto cleanup; + } + if (storageBus == StorageBus_IDE) { def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { + sdCount++; def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; - } else if (storageBus == StorageBus_SCSI) { + } else if (storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) { + sdCount++; def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; @@ -3386,24 +3325,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) 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; - } gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) @@ -3417,15 +3338,20 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) diskCount++; } + ret = 0; + + cleanup: gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); /* cleanup on error */ - if (error) { + if (ret < 0) { for (i = 0; i < def->ndisks; i++) VIR_FREE(def->disks[i]); VIR_FREE(def->disks); def->ndisks = 0; } + + return ret; } static int @@ -4120,7 +4046,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (vboxDumpStorageControllers(def, machine) < 0) goto cleanup; - vboxDumpIDEHDDs(def, data, machine); + if (vboxDumpDisks(def, data, machine) < 0) + goto cleanup; vboxDumpSharedFolders(def, data, machine); vboxDumpNetwork(def, data, machine, networkAdapterCount); @@ -5676,8 +5603,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; @@ -5686,9 +5614,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; @@ -5757,9 +5683,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; @@ -5769,7 +5692,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; @@ -5778,26 +5700,72 @@ 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 storage controller name")); goto cleanup; } - if (!disk) - continue; - rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); + + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, + storageControllerName, + &storageController); + VBOX_UTF16_FREE(storageControllerName); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get controller")); + _("Cannot get storage controller by name")); goto cleanup; } - if (!storageControllerName) { - VBOX_RELEASE(disk); + + rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get storage controller bus")); + VBOX_RELEASE(storageController); + goto cleanup; + } + + 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 port")); + 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 slot")); + 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 empty removable disk */ + if (!disk) { + VBOX_RELEASE(storageController); continue; } + handle = gVBoxAPI.UArray.handleMediumGetChildren(disk); rc = gVBoxAPI.UArray.vboxArrayGet(&children, disk, handle); if (NS_FAILED(rc)) { @@ -5820,60 +5788,30 @@ 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(storageController); + VBOX_RELEASE(disk); + VBOX_RELEASE(child); goto cleanup; } VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8); VBOX_UTF16_FREE(childLocUtf16); if (VIR_STRDUP(def->disks[diskCount].src->path, childLocUtf8) < 0) { - VBOX_RELEASE(child); VBOX_RELEASE(storageController); + VBOX_RELEASE(disk); + VBOX_RELEASE(child); 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); } @@ -5881,10 +5819,16 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, 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++) @@ -5896,9 +5840,9 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, return ret; } -static -int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, - virDomainSnapshotDefPtr def) +static int +vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, + virDomainSnapshotDefPtr def) { virDomainPtr dom = snapshot->domain; vboxDriverPtr data = dom->conn->privateData; @@ -5910,10 +5854,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, IMedium *disk = NULL; 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 i = 0, diskCount = 0, sdCount = 0; int ret = -1; if (!data->vboxObj) @@ -5976,9 +5917,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; @@ -5987,7 +5925,6 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, PRBool readOnly = PR_FALSE; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; IMediumAttachment *imediumattach = mediumAttachments.items[i]; @@ -5996,7 +5933,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get medium")); + _("Cannot get medium")); goto cleanup; } if (!disk) @@ -6004,7 +5941,7 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get storage controller name")); + _("Cannot get storage controller name")); goto cleanup; } if (!storageControllerName) @@ -6012,18 +5949,18 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, storageControllerName, &storageController); + VBOX_UTF16_FREE(storageControllerName); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get storage controller")); + _("Cannot get storage controller")); goto cleanup; } - VBOX_UTF16_FREE(storageControllerName); if (!storageController) continue; rc = gVBoxAPI.UIMedium.GetLocation(disk, &mediumLocUtf16); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get disk location")); + _("Cannot get disk location")); goto cleanup; } VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); @@ -6036,14 +5973,48 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get storage controller bus")); + _("Cannot get storage controller bus")); + goto cleanup; + } + 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 slot")); + 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; + } + + def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + devicePort, + deviceSlot, + sdCount); + if (!def->dom->disks[diskCount]->dst) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not generate medium name for the disk " + "at: port:%d, slot:%d"), devicePort, deviceSlot); + ret = -1; goto cleanup; } + if (storageBus == StorageBus_IDE) { def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { + sdCount++; def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; - } else if (storageBus == StorageBus_SCSI) { + } else if (storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) { + sdCount++; def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; @@ -6062,46 +6033,15 @@ 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 " - "at: controller instance:%u, port:%d, slot:%d"), - deviceInst, devicePort, deviceSlot); - ret = -1; - goto cleanup; - } - diskCount ++; + + diskCount++; } - /* cleanup on error */ ret = 0; + cleanup: if (ret < 0) { for (i = 0; i < def->dom->ndisks; i++) -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML generated by dumpxml used to produce "sda" for both of those disks. This is an invalid domain XML as libvirt does not allow duplicate device names. To address this, keep the running total of disks that will use "sd" prefix for device name and pass it to the vboxGenerateMediumName which no longer tries to "compute" the value based only on current and max port and slot values. After this the vboxGetMaxPortSlotValues is not needed and was deleted. --- src/vbox/vbox_common.c | 414 +++++++++++++++++++++---------------------------- 1 file changed, 177 insertions(+), 237 deletions(-)
I think this needs a bit more splitting. 1. As noted in patch 10, managing StorageBus_SAS should already be separated so that it doesn't show as a new range/comparison check here 2. The rename of vboxDumpIDEHDDs to vboxDumpDisks and change from void to int should be its own patch. While it seems superfluous, it makes review that much easier. NB: I have a couple more thoughts within the function about 3. I think the changes to vboxSnapshotGetReadWriteDisks and vboxSnapshotGetReadOnlyDisks could be in a separate patch and is where the vboxGetMaxPortSlotValues would get deleted. Although I'm not 100% sure w/r/t the relationship. Perhaps once vboxDumpDisks is cleaned up a bit before making the change described in the commit message it'd be clearer. Just a lot going on.
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 715eb670e..9dc36a1b2 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -290,61 +290,6 @@ 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_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_Floppy, - &maxSlotPerPort[StorageBus_Floppy]); - - VBOX_RELEASE(sysProps); - - return true; -}
/** * function to generate the name for medium, @@ -352,57 +297,40 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox, * * @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) { 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_Floppy)) + (storageBus > StorageBus_SAS)) return NULL;
- maxPortPerInst = aMaxPortPerInst[storageBus]; - maxSlotPerPort = aMaxSlotPerPort[storageBus]; - total = (deviceInst * maxPortPerInst * maxSlotPerPort) - + (devicePort * maxSlotPerPort) - + deviceSlot; - if (storageBus == StorageBus_IDE) { prefix = "hd"; - } else if ((storageBus == StorageBus_SATA) || - (storageBus == StorageBus_SCSI)) { + 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; }
@@ -3270,20 +3198,17 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine) }
-static void -vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) +static int +vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { - /* dump IDE hdds if present */ vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; - bool error = false; + size_t sdCount = 0, i; int diskCount = 0; - size_t i; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + int ret = -1;
def->ndisks = 0; gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, - gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine)); + gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine));
/* get the number of attachments */ for (i = 0; i < mediumAttachments.count; i++) { @@ -3300,24 +3225,19 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) }
From here....
/* 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; - } - def->disks[i] = disk; - } - } else { - error = true; - } + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) + goto cleanup;
- if (!error) - error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + if (!disk) + goto cleanup; + + def->disks[i] = disk; + }> /* get the attachment details here */ - for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) { + for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) {
... to here should be a separate patch after the rename function and change to int patch. It's just cleaning up the logic a bit, but now that you have a cleanup label @error is unnecessary. Of course it'd need to keep the vboxGetMaxPortSlotValues which would then be removed on the subsequent patch...
IMediumAttachment *imediumattach = mediumAttachments.items[i]; IStorageController *storageController = NULL; PRUnichar *storageControllerName = NULL; @@ -3327,7 +3247,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) IMedium *medium = NULL; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0;
@@ -3363,16 +3282,36 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!virDomainDiskGetSource(def->disks[diskCount])) { VBOX_RELEASE(medium); VBOX_RELEASE(storageController); - error = true; - break; + + goto cleanup; }
gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); + gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); + gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + + def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, + devicePort, + deviceSlot, + sdCount); + if (!def->disks[diskCount]->dst) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not generate medium name for the disk " + "at: port:%d, slot:%d"), devicePort, deviceSlot); + VBOX_RELEASE(medium); + VBOX_RELEASE(storageController); + + goto cleanup; + } + if (storageBus == StorageBus_IDE) { def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { + sdCount++; def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; - } else if (storageBus == StorageBus_SCSI) { + } else if (storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) { + sdCount++; def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; @@ -3386,24 +3325,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) 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; - }
gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) @@ -3417,15 +3338,20 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) diskCount++; }
+ ret = 0; + + cleanup: gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
/* cleanup on error */ - if (error) { + if (ret < 0) { for (i = 0; i < def->ndisks; i++) VIR_FREE(def->disks[i]); VIR_FREE(def->disks); def->ndisks = 0;
Similar to my note about controllers in patch 11, this is now unnecessary... With the void function, I can see why, but it'll be cleaned up properly now anyway, so it's unnecessary.
} + + return ret; }
static int @@ -4120,7 +4046,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) if (vboxDumpStorageControllers(def, machine) < 0) goto cleanup;
- vboxDumpIDEHDDs(def, data, machine); + if (vboxDumpDisks(def, data, machine) < 0) + goto cleanup;
vboxDumpSharedFolders(def, data, machine); vboxDumpNetwork(def, data, machine, networkAdapterCount); @@ -5676,8 +5603,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data, return snapshot; }
Other than considering whether the split out or not, these looked OK to me - a lot going on, but it's the same stuff and looks reasonable. John [...]
On Fri, 2017-11-03 at 09:52 -0400, John Ferlan wrote:
>
> On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> > If a VBOX VM has e.g. a SATA and SCSI disk attached, the XML
> > generated
> > by dumpxml used to produce "sda" for both of those disks. This is
> > an
> > invalid domain XML as libvirt does not allow duplicate device
> > names. To
> > address this, keep the running total of disks that will use "sd"
> > prefix
> > for device name and pass it to the vboxGenerateMediumName which no
> > longer tries to "compute" the value based only on current and max
> > port and slot values. After this the vboxGetMaxPortSlotValues is
> > not
> > needed and was deleted.
> > ---
> > src/vbox/vbox_common.c | 414 +++++++++++++++++++++--------------
> > --------------
> > 1 file changed, 177 insertions(+), 237 deletions(-)
> >
>
> I think this needs a bit more splitting.
>
> 1. As noted in patch 10, managing StorageBus_SAS should already be
> separated so that it doesn't show as a new range/comparison check
> here
>
> 2. The rename of vboxDumpIDEHDDs to vboxDumpDisks and change from
> void
> to int should be its own patch. While it seems superfluous, it makes
> review that much easier. NB: I have a couple more thoughts within
> the
> function about
>
> 3. I think the changes to vboxSnapshotGetReadWriteDisks and
> vboxSnapshotGetReadOnlyDisks could be in a separate patch and is
> where
> the vboxGetMaxPortSlotValues would get deleted. Although I'm not 100%
> sure w/r/t the relationship. Perhaps once vboxDumpDisks is cleaned up
> a
> bit before making the change described in the commit message it'd be
> clearer. Just a lot going on.
>
>
Hi John,
I've just send V3 [1] when I separated most of the changes into
individual patches as you requested. However, I could not separate
vboxSnapshotGetReadWrite and vboxSnapshotGetReadOnly disks completely
as the changes made to vboxGenerateMedium name must be combined with
the removal of vboxGetMaxPortSlotValues so instead, I've isolated the
code movement changes where I moved the code to read VBOX's storage
controller, slot and port to the beginning of the loop body - either
way, I think it's much cleaner and easier to follow the changes now.
[1] https://www.redhat.com/archives/libvir-list/2017-November/msg00252.
html
Dawid
>
> > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> > index 715eb670e..9dc36a1b2 100644
> > --- a/src/vbox/vbox_common.c
> > +++ b/src/vbox/vbox_common.c
> > @@ -290,61 +290,6 @@ 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(sysPr
> > ops,
> > - Stora
> > geBus_IDE,
> > - &maxP
> > ortPerInst[StorageBus_IDE]);
> > - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_SATA,
> > - &maxP
> > ortPerInst[StorageBus_SATA]);
> > - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_SCSI,
> > - &maxP
> > ortPerInst[StorageBus_SCSI]);
> > - gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysPr
> > ops,
> > - Stora
> > geBus_Floppy,
> > - &maxP
> > ortPerInst[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_Floppy,
> > -
> > &maxSlotPerPort[StorageBus_Floppy]);
> > -
> > - VBOX_RELEASE(sysProps);
> > -
> > - return true;
> > -}
> >
> > /**
> > * function to generate the name for medium,
> > @@ -352,57 +297,40 @@ static bool
> > vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> > *
> > * @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)
> > {
> > 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_Floppy))
> > + (storageBus > StorageBus_SAS))
> > return NULL;
> >
> > - maxPortPerInst = aMaxPortPerInst[storageBus];
> > - maxSlotPerPort = aMaxSlotPerPort[storageBus];
> > - total = (deviceInst * maxPortPerInst * maxSlotPerPort)
> > - + (devicePort * maxSlotPerPort)
> > - + deviceSlot;
> > -
> > if (storageBus == StorageBus_IDE) {
> > prefix = "hd";
> > - } else if ((storageBus == StorageBus_SATA) ||
> > - (storageBus == StorageBus_SCSI)) {
> > + 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;
> > }
> >
> > @@ -3270,20 +3198,17 @@ vboxDumpStorageControllers(virDomainDefPtr
> > def, IMachine *machine)
> > }
> >
> >
> > -static void
> > -vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine
> > *machine)
> > +static int
> > +vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine
> > *machine)
> > {
> > - /* dump IDE hdds if present */
> > vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
> > - bool error = false;
> > + size_t sdCount = 0, i;
> > int diskCount = 0;
> > - size_t i;
> > - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> > - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> > + int ret = -1;
> >
> > def->ndisks = 0;
> > gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine,
> > - gVBoxAPI.UArray.handleMachineGetM
> > ediumAttachments(machine));
> > + gVBoxAPI.UArray.handleMachineGetMediumAttachments
> > (machine));
> >
> > /* get the number of attachments */
> > for (i = 0; i < mediumAttachments.count; i++) {
> > @@ -3300,24 +3225,19 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > }
> >
>
> From here....
>
> > /* 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;
> > - }
> > - def->disks[i] = disk;
> > - }
> > - } else {
> > - error = true;
> > - }
> > + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0)
> > + goto cleanup;
> >
> > - if (!error)
> > - error = !vboxGetMaxPortSlotValues(data->vboxObj,
> > maxPortPerInst, maxSlotPerPort);
> > + for (i = 0; i < def->ndisks; i++) {
> > + virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
> > + if (!disk)
> > + goto cleanup;
> > +
> > + def->disks[i] = disk;
> > + }>
> > /* get the attachment details here */
> > - for (i = 0; i < mediumAttachments.count && diskCount < def-
> > >ndisks && !error; i++) {
> > + for (i = 0; i < mediumAttachments.count && diskCount < def-
> > >ndisks; i++) {
>
> ... to here should be a separate patch after the rename function and
> change to int patch. It's just cleaning up the logic a bit, but now
> that you have a cleanup label @error is unnecessary.
>
> Of course it'd need to keep the vboxGetMaxPortSlotValues which would
> then be removed on the subsequent patch...
>
> > IMediumAttachment *imediumattach =
> > mediumAttachments.items[i];
> > IStorageController *storageController = NULL;
> > PRUnichar *storageControllerName = NULL;
> > @@ -3327,7 +3247,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > IMedium *medium = NULL;
> > PRUnichar *mediumLocUtf16 = NULL;
> > char *mediumLocUtf8 = NULL;
> > - PRUint32 deviceInst = 0;
> > PRInt32 devicePort = 0;
> > PRInt32 deviceSlot = 0;
> >
> > @@ -3363,16 +3282,36 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > if (!virDomainDiskGetSource(def->disks[diskCount])) {
> > VBOX_RELEASE(medium);
> > VBOX_RELEASE(storageController);
> > - error = true;
> > - break;
> > +
> > + goto cleanup;
> > }
> >
> > gVBoxAPI.UIStorageController.GetBus(storageController,
> > &storageBus);
> > + gVBoxAPI.UIMediumAttachment.GetPort(imediumattach,
> > &devicePort);
> > + gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach,
> > &deviceSlot);
> > +
> > + def->disks[diskCount]->dst =
> > vboxGenerateMediumName(storageBus,
> > + device
> > Port,
> > + device
> > Slot,
> > + sdCoun
> > t);
> > + if (!def->disks[diskCount]->dst) {
> > + virReportError(VIR_ERR_INTERNAL_ERROR,
> > + _("Could not generate medium name for
> > the disk "
> > + "at: port:%d, slot:%d"), devicePort,
> > deviceSlot);
> > + VBOX_RELEASE(medium);
> > + VBOX_RELEASE(storageController);
> > +
> > + goto cleanup;
> > + }
> > +
> > if (storageBus == StorageBus_IDE) {
> > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
> > } else if (storageBus == StorageBus_SATA) {
> > + sdCount++;
> > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
> > - } else if (storageBus == StorageBus_SCSI) {
> > + } else if (storageBus == StorageBus_SCSI ||
> > + storageBus == StorageBus_SAS) {
> > + sdCount++;
> > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> > } else if (storageBus == StorageBus_Floppy) {
> > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
> > @@ -3386,24 +3325,6 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > 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,
> > - device
> > Inst,
> > - device
> > Port,
> > - device
> > Slot,
> > - maxPor
> > tPerInst,
> > - maxSlo
> > tPerPort);
> > - 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;
> > - }
> >
> > gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
> > if (readOnly == PR_TRUE)
> > @@ -3417,15 +3338,20 @@ vboxDumpIDEHDDs(virDomainDefPtr def,
> > vboxDriverPtr data, IMachine *machine)
> > diskCount++;
> > }
> >
> > + ret = 0;
> > +
> > + cleanup:
> > gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
> >
> > /* cleanup on error */
> > - if (error) {
> > + if (ret < 0) {
> > for (i = 0; i < def->ndisks; i++)
> > VIR_FREE(def->disks[i]);
> > VIR_FREE(def->disks);
> > def->ndisks = 0;
>
> Similar to my note about controllers in patch 11, this is now
> unnecessary... With the void function, I can see why, but it'll be
> cleaned up properly now anyway, so it's unnecessary.
>
> > }
> > +
> > + return ret;
> > }
> >
> > static int
> > @@ -4120,7 +4046,8 @@ static char
> > *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags)
> > if (vboxDumpStorageControllers(def, machine) < 0)
> > goto cleanup;
> >
> > - vboxDumpIDEHDDs(def, data, machine);
> > + if (vboxDumpDisks(def, data, machine) < 0)
> > + goto cleanup;
> >
> > vboxDumpSharedFolders(def, data, machine);
> > vboxDumpNetwork(def, data, machine, networkAdapterCount);
> > @@ -5676,8 +5603,9 @@ vboxDomainSnapshotGet(vboxDriverPtr data,
> > return snapshot;
> > }
> >
>
> Other than considering whether the split out or not, these looked OK
> to
> me - a lot going on, but it's the same stuff and looks reasonable.
>
>
> John
>
>
> [...]
Primer the code for further changes: * move variable declarations to the top of the function * group together free/release statements * error check and report VBOX API calls used --- src/vbox/vbox_common.c | 188 +++++++++++++++++++++++++++++++------------------ 1 file changed, 120 insertions(+), 68 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9dc36a1b2..ee6421aae 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3202,6 +3202,16 @@ static int vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *mediumAttachment = NULL; + IMedium *medium = NULL; + IStorageController *controller = NULL; + PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL; + PRUint32 deviceType, storageBus; + PRInt32 devicePort, deviceSlot; + PRBool readOnly; + nsresult rc; + virDomainDiskDefPtr disk = NULL; + char *mediumLocUtf8 = NULL; size_t sdCount = 0, i; int diskCount = 0; int ret = -1; @@ -3212,15 +3222,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) /* get the number of attachments */ for (i = 0; i < mediumAttachments.count; i++) { - IMediumAttachment *imediumattach = mediumAttachments.items[i]; - if (imediumattach) { - IMedium *medium = NULL; + mediumAttachment = mediumAttachments.items[i]; + if (!mediumAttachment) + continue; - gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); - if (medium) { - def->ndisks++; - VBOX_RELEASE(medium); - } + gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (medium) { + def->ndisks++; + VBOX_RELEASE(medium); } } @@ -3229,7 +3238,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; for (i = 0; i < def->ndisks; i++) { - virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + disk = virDomainDiskDefNew(NULL); if (!disk) goto cleanup; @@ -3238,104 +3247,141 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) /* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { - IMediumAttachment *imediumattach = 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; - PRInt32 devicePort = 0; - PRInt32 deviceSlot = 0; - - if (!imediumattach) + mediumAttachment = mediumAttachments.items[i]; + controller = NULL; + controllerName = NULL; + deviceType = DeviceType_Null; + storageBus = StorageBus_Null; + readOnly = PR_FALSE; + medium = NULL; + mediumLocUtf16 = NULL; + mediumLocUtf8 = NULL; + devicePort = 0; + deviceSlot = 0; + disk = def->disks[diskCount]; + + if (!mediumAttachment) continue; - gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get IMedium, rc=%08x"), rc); + goto cleanup; + } + if (!medium) continue; - gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); - if (!storageControllerName) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment, + &controllerName); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get storage controller name, rc=%08x"), + rc); + goto cleanup; } - gVBoxAPI.UIMachine.GetStorageControllerByName(machine, - storageControllerName, - &storageController); - VBOX_UTF16_FREE(storageControllerName); - if (!storageController) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, + controllerName, + &controller); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get storage controller by name, rc=%08x"), + rc); + goto cleanup; + } + + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get medium storage location, rc=%08x"), + rc); + goto cleanup; } - 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); - if (!virDomainDiskGetSource(def->disks[diskCount])) { - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set disk source")); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device type, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumAttachment, &devicePort); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device port, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumAttachment, &deviceSlot); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device slot, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIStorageController.GetBus(controller, &storageBus); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get storage controller bus, rc=%08x"), + rc); + goto cleanup; + } + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get read only state, rc=%08x"), rc); goto cleanup; } - gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); - gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); - gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot, + sdCount); - def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, - devicePort, - deviceSlot, - sdCount); - if (!def->disks[diskCount]->dst) { + if (!disk->dst) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not generate medium name for the disk " "at: port:%d, slot:%d"), devicePort, deviceSlot); - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); - goto cleanup; } if (storageBus == StorageBus_IDE) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + disk->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { sdCount++; - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + disk->bus = VIR_DOMAIN_DISK_BUS_SATA; } else if (storageBus == StorageBus_SCSI || storageBus == StorageBus_SAS) { sdCount++; - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + disk->bus = VIR_DOMAIN_DISK_BUS_FDC; } - 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; + 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); - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); diskCount++; + + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); } ret = 0; @@ -3345,6 +3391,12 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) /* cleanup on error */ if (ret < 0) { + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); + for (i = 0; i < def->ndisks; i++) VIR_FREE(def->disks[i]); VIR_FREE(def->disks); -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Primer the code for further changes:
* move variable declarations to the top of the function * group together free/release statements * error check and report VBOX API calls used --- src/vbox/vbox_common.c | 188 +++++++++++++++++++++++++++++++------------------ 1 file changed, 120 insertions(+), 68 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9dc36a1b2..ee6421aae 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3202,6 +3202,16 @@ static int vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *mediumAttachment = NULL; + IMedium *medium = NULL; + IStorageController *controller = NULL; + PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL; + PRUint32 deviceType, storageBus; + PRInt32 devicePort, deviceSlot; + PRBool readOnly; + nsresult rc; + virDomainDiskDefPtr disk = NULL; + char *mediumLocUtf8 = NULL; size_t sdCount = 0, i; int diskCount = 0; int ret = -1; @@ -3212,15 +3222,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
/* get the number of attachments */ for (i = 0; i < mediumAttachments.count; i++) { - IMediumAttachment *imediumattach = mediumAttachments.items[i]; - if (imediumattach) { - IMedium *medium = NULL; + mediumAttachment = mediumAttachments.items[i]; + if (!mediumAttachment) + continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); - if (medium) { - def->ndisks++; - VBOX_RELEASE(medium); - } + gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (medium) { + def->ndisks++; + VBOX_RELEASE(medium); } }
@@ -3229,7 +3238,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup;
for (i = 0; i < def->ndisks; i++) { - virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + disk = virDomainDiskDefNew(NULL); if (!disk) goto cleanup;
@@ -3238,104 +3247,141 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
/* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { - IMediumAttachment *imediumattach = 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; - PRInt32 devicePort = 0; - PRInt32 deviceSlot = 0; - - if (!imediumattach) + mediumAttachment = mediumAttachments.items[i]; + controller = NULL; + controllerName = NULL; + deviceType = DeviceType_Null; + storageBus = StorageBus_Null; + readOnly = PR_FALSE; + medium = NULL; + mediumLocUtf16 = NULL; + mediumLocUtf8 = NULL; + devicePort = 0; + deviceSlot = 0; + disk = def->disks[diskCount]; + + if (!mediumAttachment) continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get IMedium, rc=%08x"), rc); + goto cleanup; + } + if (!medium) continue;
- gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); - if (!storageControllerName) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment, + &controllerName); ^^^^^ Alignment for second line... Needs 5 more spaces right...
+ if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get storage controller name, rc=%08x"), + rc); + goto cleanup; }
- gVBoxAPI.UIMachine.GetStorageControllerByName(machine, - storageControllerName, - &storageController); - VBOX_UTF16_FREE(storageControllerName); - if (!storageController) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, + controllerName, + &controller);
One more space each for arg2 and arg3 The rest looks OK... with the above adjustments... Reviewed-by: John Ferlan <jferlan@redhat.com> John
+ if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get storage controller by name, rc=%08x"), + rc); + goto cleanup; + } + + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get medium storage location, rc=%08x"), + rc); + goto cleanup; }
- 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);
- if (!virDomainDiskGetSource(def->disks[diskCount])) { - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set disk source")); + goto cleanup; + }
+ rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device type, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumAttachment, &devicePort); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device port, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumAttachment, &deviceSlot); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device slot, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIStorageController.GetBus(controller, &storageBus); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get storage controller bus, rc=%08x"), + rc); + goto cleanup; + } + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get read only state, rc=%08x"), rc); goto cleanup; }
- gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); - gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); - gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot, + sdCount);
- def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, - devicePort, - deviceSlot, - sdCount); - if (!def->disks[diskCount]->dst) { + if (!disk->dst) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not generate medium name for the disk " "at: port:%d, slot:%d"), devicePort, deviceSlot); - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); - goto cleanup; }
if (storageBus == StorageBus_IDE) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + disk->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { sdCount++; - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + disk->bus = VIR_DOMAIN_DISK_BUS_SATA; } else if (storageBus == StorageBus_SCSI || storageBus == StorageBus_SAS) { sdCount++; - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + disk->bus = VIR_DOMAIN_DISK_BUS_FDC; }
- 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; + 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);
- VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); diskCount++; + + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); }
ret = 0; @@ -3345,6 +3391,12 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
/* cleanup on error */ if (ret < 0) { + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); + for (i = 0; i < def->ndisks; i++) VIR_FREE(def->disks[i]); VIR_FREE(def->disks);
Previously any removable storage device without media attached was omitted from domain XML dump. They're still (rightfully) omitted in snapshot XMl dump but need to be accounted properly to for the device names to stay in 'sync' between domain and snapshot XML dumps. --- src/vbox/vbox_common.c | 128 ++++++++++++++++++++++++++++--------------------- 1 file changed, 74 insertions(+), 54 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index ee6421aae..d1d8804c7 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3213,7 +3213,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) virDomainDiskDefPtr disk = NULL; char *mediumLocUtf8 = NULL; size_t sdCount = 0, i; - int diskCount = 0; int ret = -1; def->ndisks = 0; @@ -3226,11 +3225,15 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!mediumAttachment) continue; - gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); - if (medium) { - def->ndisks++; - VBOX_RELEASE(medium); + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get IMedium, rc=%08x"), rc); + goto cleanup; } + + def->ndisks++; + VBOX_RELEASE(medium); } /* Allocate mem, if fails return error */ @@ -3246,7 +3249,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) } /* get the attachment details here */ - for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { + for (i = 0; i < mediumAttachments.count; i++) { mediumAttachment = mediumAttachments.items[i]; controller = NULL; controllerName = NULL; @@ -3258,7 +3261,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) mediumLocUtf8 = NULL; devicePort = 0; deviceSlot = 0; - disk = def->disks[diskCount]; + disk = def->disks[i]; if (!mediumAttachment) continue; @@ -3270,9 +3273,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; } - if (!medium) - continue; - rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment, &controllerName); if (NS_FAILED(rc)) { @@ -3292,22 +3292,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; } - rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get medium storage location, rc=%08x"), - rc); - goto cleanup; - } - - VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); - - if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not set disk source")); - goto cleanup; - } - rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3333,11 +3317,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) rc); goto cleanup; } - rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get read only state, rc=%08x"), rc); - goto cleanup; + + if (medium) { + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get medium storage location, rc=%08x"), + rc); + goto cleanup; + } + + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set disk source")); + goto cleanup; + } + + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get read only state, rc=%08x"), rc); + goto cleanup; + } } disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot, @@ -3375,8 +3378,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); - diskCount++; - VBOX_UTF16_FREE(controllerName); VBOX_UTF8_FREE(mediumLocUtf8); VBOX_UTF16_FREE(mediumLocUtf16); @@ -5814,6 +5815,14 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, /* skip empty removable disk */ if (!disk) { + /* removable disks with empty (ejected) media won't be displayed + * in XML, but we need to update "sdCount" so that device names match + * in domain dumpxml and snapshot dumpxml + */ + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) + sdCount++; + VBOX_RELEASE(storageController); continue; } @@ -5982,14 +5991,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, 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", @@ -6009,19 +6010,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, } if (!storageController) continue; - 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.UIStorageController.GetBus(storageController, &storageBus); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6040,6 +6028,38 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, _("Cannot get device slot")); goto cleanup; } + + rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get medium")); + goto cleanup; + } + + /* skip empty removable disk */ + if (!disk) { + /* removable disks with empty (ejected) media won't be displayed + * in XML, but we need to update "sdCount" so that device names match + * in domain dumpxml and snapshot dumpxml + */ + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) + sdCount++; + continue; + } + + 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", -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
Previously any removable storage device without media attached was omitted from domain XML dump. They're still (rightfully) omitted in snapshot XMl dump but need to be accounted properly to for the device
XML
names to stay in 'sync' between domain and snapshot XML dumps. --- src/vbox/vbox_common.c | 128 ++++++++++++++++++++++++++++--------------------- 1 file changed, 74 insertions(+), 54 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index ee6421aae..d1d8804c7 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3213,7 +3213,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) virDomainDiskDefPtr disk = NULL; char *mediumLocUtf8 = NULL; size_t sdCount = 0, i; - int diskCount = 0; int ret = -1;
def->ndisks = 0; @@ -3226,11 +3225,15 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) if (!mediumAttachment) continue;
- gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); - if (medium) { - def->ndisks++; - VBOX_RELEASE(medium); + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get IMedium, rc=%08x"), rc); + goto cleanup; } + + def->ndisks++; + VBOX_RELEASE(medium); }
/* Allocate mem, if fails return error */ @@ -3246,7 +3249,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) }
/* get the attachment details here */ - for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { + for (i = 0; i < mediumAttachments.count; i++) { mediumAttachment = mediumAttachments.items[i]; controller = NULL; controllerName = NULL; @@ -3258,7 +3261,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) mediumLocUtf8 = NULL; devicePort = 0; deviceSlot = 0; - disk = def->disks[diskCount]; + disk = def->disks[i];
if (!mediumAttachment) continue; @@ -3270,9 +3273,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; }
- if (!medium) - continue; - rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment, &controllerName); if (NS_FAILED(rc)) { @@ -3292,22 +3292,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; }
- rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get medium storage location, rc=%08x"), - rc); - goto cleanup; - } - - VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); - - if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Could not set disk source")); - goto cleanup; - } - rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -3333,11 +3317,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) rc); goto cleanup; } - rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); - if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not get read only state, rc=%08x"), rc); - goto cleanup; + + if (medium) { + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get medium storage location, rc=%08x"), + rc); + goto cleanup; + } + + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); + + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set disk source")); + goto cleanup; + } + + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get read only state, rc=%08x"), rc); + goto cleanup; + } }
disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot, @@ -3375,8 +3378,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
- diskCount++; - VBOX_UTF16_FREE(controllerName); VBOX_UTF8_FREE(mediumLocUtf8); VBOX_UTF16_FREE(mediumLocUtf16); @@ -5814,6 +5815,14 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
/* skip empty removable disk */ if (!disk) { + /* removable disks with empty (ejected) media won't be displayed + * in XML, but we need to update "sdCount" so that device names match + * in domain dumpxml and snapshot dumpxml + */ + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) + sdCount++; + VBOX_RELEASE(storageController); continue; } @@ -5982,14 +5991,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, 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;
Can the changes that are moving the medium code to be all together go into a patch just before this one? Then the hunk here just becomes adding the sdCount++ adjustment when !disk - just like all that changed for vboxSnapshotGetReadWriteDisks. Tks - John
rc = gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6009,19 +6010,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, } if (!storageController) continue; - 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.UIStorageController.GetBus(storageController, &storageBus); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -6040,6 +6028,38 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, _("Cannot get device slot")); goto cleanup; } + + rc = gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &disk); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot get medium")); + goto cleanup; + } + + /* skip empty removable disk */ + if (!disk) { + /* removable disks with empty (ejected) media won't be displayed + * in XML, but we need to update "sdCount" so that device names match + * in domain dumpxml and snapshot dumpxml + */ + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) + sdCount++; + continue; + } + + 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",
This patch adds <address> element to each <disk> device since device names alone won't adequately reflect the storage device layout in the VM. With this patch, the ouput produced by dumpxml will faithfully reproduce the storage layout of the VM if used with define. --- src/vbox/vbox_common.c | 79 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 11 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d1d8804c7..95d35631f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3211,8 +3211,9 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) PRBool readOnly; nsresult rc; virDomainDiskDefPtr disk = NULL; + virDomainControllerDefPtr ctrl = NULL; char *mediumLocUtf8 = NULL; - size_t sdCount = 0, i; + size_t sdCount = 0, i, j; int ret = -1; def->ndisks = 0; @@ -3353,26 +3354,83 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; } - if (storageBus == StorageBus_IDE) { + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + disk->info.addr.drive.bus = 0; + disk->info.addr.drive.unit = devicePort; + + switch ((enum StorageBus) storageBus) { + case StorageBus_IDE: disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - } else if (storageBus == StorageBus_SATA) { - sdCount++; + disk->info.addr.drive.bus = devicePort; /* primary, secondary */ + disk->info.addr.drive.unit = deviceSlot; /* master, slave */ + + break; + case StorageBus_SATA: disk->bus = VIR_DOMAIN_DISK_BUS_SATA; - } else if (storageBus == StorageBus_SCSI || - storageBus == StorageBus_SAS) { sdCount++; + + break; + case StorageBus_SCSI: + case StorageBus_SAS: disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; - } else if (storageBus == StorageBus_Floppy) { + sdCount++; + + /* In vbox, if there's a disk attached to SAS controller, there will + * be libvirt SCSI controller present with model "lsi1068", and we + * need to find its index + */ + for (j = 0; j < def->ncontrollers; j++) { + 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; + break; + } + + if (storageBus == StorageBus_SCSI && + ctrl->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { + disk->info.addr.drive.controller = ctrl->idx; + break; + } + } + + break; + case StorageBus_Floppy: disk->bus = VIR_DOMAIN_DISK_BUS_FDC; + + break; + case StorageBus_Null: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unsupported null storage bus")); + goto cleanup; } - if (deviceType == DeviceType_HardDisk) + switch ((enum DeviceType) deviceType) { + case DeviceType_HardDisk: disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; - else if (deviceType == DeviceType_Floppy) + + break; + case DeviceType_Floppy: disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; - else if (deviceType == DeviceType_DVD) + + break; + case DeviceType_DVD: disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; + break; + case DeviceType_Network: + case DeviceType_USB: + case DeviceType_SharedFolder: + case DeviceType_Null: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unsupported vbox device type: %d"), deviceType); + goto cleanup; + } + if (readOnly == PR_TRUE) disk->src->readonly = true; @@ -4098,7 +4156,6 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; if (vboxDumpStorageControllers(def, machine) < 0) goto cleanup; - if (vboxDumpDisks(def, data, machine) < 0) goto cleanup; -- 2.14.2
On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
This patch adds <address> element to each <disk> device since device names alone won't adequately reflect the storage device layout in the VM. With this patch, the ouput produced by dumpxml will faithfully reproduce the storage layout of the VM if used with define. --- src/vbox/vbox_common.c | 79 +++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 68 insertions(+), 11 deletions(-)
This one seems OK, but I didn't think long about it. I'll look again when the v3 is posted. What I'll also ask is that when v3 hits, please be sure to create a docs/news.xml article that briefly describes some of the fixes you've made in this series that would go into the next release. Tks - John
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d1d8804c7..95d35631f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3211,8 +3211,9 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) PRBool readOnly; nsresult rc; virDomainDiskDefPtr disk = NULL; + virDomainControllerDefPtr ctrl = NULL; char *mediumLocUtf8 = NULL; - size_t sdCount = 0, i; + size_t sdCount = 0, i, j; int ret = -1;
def->ndisks = 0; @@ -3353,26 +3354,83 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; }
- if (storageBus == StorageBus_IDE) { + disk->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DRIVE; + disk->info.addr.drive.bus = 0; + disk->info.addr.drive.unit = devicePort; + + switch ((enum StorageBus) storageBus) { + case StorageBus_IDE: disk->bus = VIR_DOMAIN_DISK_BUS_IDE; - } else if (storageBus == StorageBus_SATA) { - sdCount++; + disk->info.addr.drive.bus = devicePort; /* primary, secondary */ + disk->info.addr.drive.unit = deviceSlot; /* master, slave */ + + break; + case StorageBus_SATA: disk->bus = VIR_DOMAIN_DISK_BUS_SATA; - } else if (storageBus == StorageBus_SCSI || - storageBus == StorageBus_SAS) { sdCount++; + + break; + case StorageBus_SCSI: + case StorageBus_SAS: disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; - } else if (storageBus == StorageBus_Floppy) { + sdCount++; + + /* In vbox, if there's a disk attached to SAS controller, there will + * be libvirt SCSI controller present with model "lsi1068", and we + * need to find its index + */ + for (j = 0; j < def->ncontrollers; j++) { + 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; + break; + } + + if (storageBus == StorageBus_SCSI && + ctrl->model != VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { + disk->info.addr.drive.controller = ctrl->idx; + break; + } + } + + break; + case StorageBus_Floppy: disk->bus = VIR_DOMAIN_DISK_BUS_FDC; + + break; + case StorageBus_Null: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unsupported null storage bus")); + goto cleanup; }
- if (deviceType == DeviceType_HardDisk) + switch ((enum DeviceType) deviceType) { + case DeviceType_HardDisk: disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; - else if (deviceType == DeviceType_Floppy) + + break; + case DeviceType_Floppy: disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; - else if (deviceType == DeviceType_DVD) + + break; + case DeviceType_DVD: disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
+ break; + case DeviceType_Network: + case DeviceType_USB: + case DeviceType_SharedFolder: + case DeviceType_Null: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unsupported vbox device type: %d"), deviceType); + goto cleanup; + } + if (readOnly == PR_TRUE) disk->src->readonly = true;
@@ -4098,7 +4156,6 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; if (vboxDumpStorageControllers(def, machine) < 0) goto cleanup; - if (vboxDumpDisks(def, data, machine) < 0) goto cleanup;
participants (2)
-
Dawid Zamirski -
John Ferlan