[libvirt] [PATCH v3 00/13] vbox: Improve handling of storage devices
v2: https://www.redhat.com/archives/libvir-list/2017-October/msg01108.html Changes since v2: * Rebase on master * Do not segfault in 'Cleanup partially-defined VM on failure' when code jumps to cleanup and machine is NULL * Take out SAS controller handling code into a separate patch * Split up the snapshot function cleanup code into separate patches * Fixed code formatting as pointed out in review * Rename vboxDumpIDEHDDs -> vboxDumpDisks into a separate patch * Add docs/news.xml patch to describe improvements/fixes in the series Dawid Zamirski (13): vbox: Cleanup partially-defined VM on failure vbox: Process <controller> element in domain XML vbox: Add vboxDumpStorageControllers vbox: Rename vboxDumpIDEHDDs to vboxDumpDisks vbox: Cleanup/prepare snasphot dumpxml functions vbox: Do not free disk definitions on cleanup vbox: Swap vboxSnapshotGetReadOnlyDisks arguments 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 vbox: Add SAS controller support docs: Update news.xml with vbox changes. docs/news.xml | 74 ++++ src/vbox/vbox_common.c | 1105 +++++++++++++++++++++++++++++++----------------- src/vbox/vbox_common.h | 8 + 3 files changed, 792 insertions(+), 395 deletions(-) -- 2.14.3
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 | 52 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index aa2563cf1..d93b0855f 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -1821,6 +1821,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); @@ -1830,12 +1832,11 @@ 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; - } + NULL, parse_flags))) + return ret; + VBOX_IID_INITIALIZE(&mchiid); virUUIDFormat(def->uuid, uuidstr); rc = gVBoxAPI.UIVirtualBox.CreateMachine(data, def, &machine, uuidstr); @@ -1928,30 +1929,49 @@ 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: + /* if machine wasn't even created, cleanup is trivial */ + if (!machine) { + vboxIIDUnalloc(&mchiid); + virDomainDefFree(def); + + return ret; + } + + /* 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: VBOX_RELEASE(machine); + vboxIIDUnalloc(&mchiid); virDomainDefFree(def); - return NULL; + + return ret; } static virDomainPtr -- 2.14.3
With this patch, the vbox driver will no longer attach all supported storage controllers by default even if no disk devices are associated with them. Instead, it will attach only those that are implicitly added by virDomainDefAddImplicitController based on <disk> element or if explicitly specified via the <controller> element. --- src/vbox/vbox_common.c | 199 ++++++++++++++++++++++++++++++++++++++----------- src/vbox/vbox_common.h | 7 ++ 2 files changed, 161 insertions(+), 45 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d93b0855f..49df52c12 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -406,6 +406,154 @@ 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_VMPVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: + 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, @@ -972,46 +1120,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 +1174,21 @@ 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); 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 +1256,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); @@ -1917,6 +2024,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 f28f2e03f..5dc87239d 100644 --- a/src/vbox/vbox_common.h +++ b/src/vbox/vbox_common.h @@ -326,6 +326,13 @@ 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" + /* Simplied definitions in vbox_CAPI_*.h */ typedef void const *PCVBOXXPCOM; -- 2.14.3
--- src/vbox/vbox_common.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 109 insertions(+) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 49df52c12..4d596075c 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3147,6 +3147,113 @@ 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: + 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); + + return ret; +} + + static void vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { @@ -3994,6 +4101,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.3
Because it deals with other disk types as well not just IDE. Also this function now returns -1 on error --- src/vbox/vbox_common.c | 57 ++++++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 4d596075c..2e0d7ee9a 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3254,13 +3254,11 @@ 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; - int diskCount = 0; + int ret = -1, diskCount = 0; size_t i; PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; @@ -3284,24 +3282,22 @@ 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; + + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + if (!disk) + goto cleanup; + + def->disks[i] = disk; } - if (!error) - error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort); + if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort)) + goto cleanup; /* 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; @@ -3347,8 +3343,8 @@ 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); @@ -3385,8 +3381,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) deviceInst, devicePort, deviceSlot); VBOX_RELEASE(medium); VBOX_RELEASE(storageController); - error = true; - break; + + goto cleanup; } gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); @@ -3401,15 +3397,12 @@ vboxDumpIDEHDDs(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) diskCount++; } + ret = 0; + + cleanup: gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); - /* cleanup on error */ - if (error) { - for (i = 0; i < def->ndisks; i++) - VIR_FREE(def->disks[i]); - VIR_FREE(def->disks); - def->ndisks = 0; - } + return ret; } static int @@ -4103,8 +4096,8 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) goto cleanup; 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); -- 2.14.3
This patch prepares the vboxSnapshotGetReadOnlyDisks and vboxSnapshotGetReadWriteDisks functions for further changes so that the code movement does not obstruct the gist of those future changes. This is done primarily because we'll need to know the type of vbox storage controller as early as possible and make decisions based on that info. --- src/vbox/vbox_common.c | 185 ++++++++++++++++++++++++++++--------------------- 1 file changed, 105 insertions(+), 80 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 2e0d7ee9a..d26ce1bce 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5653,8 +5653,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; @@ -5755,26 +5756,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)) { @@ -5797,53 +5844,25 @@ 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, @@ -5862,6 +5881,7 @@ static int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); ret = 0; + cleanup: if (ret < 0) { for (i = 0; i < def->ndisks; i++) @@ -5873,9 +5893,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; @@ -5970,18 +5990,10 @@ int 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", - _("cannot get storage controller name")); + _("Cannot get storage controller name")); goto cleanup; } if (!storageControllerName) @@ -5989,18 +6001,50 @@ 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.UIStorageController.GetBus(storageController, &storageBus); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("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.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) { + VBOX_RELEASE(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); @@ -6009,11 +6053,10 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, goto cleanup; VBOX_UTF8_FREE(mediumLocUtf8); - - rc = gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); + rc = gVBoxAPI.UIMedium.GetReadOnly(disk, &readOnly); if (NS_FAILED(rc)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("cannot get storage controller bus")); + _("Cannot get read only attribute")); goto cleanup; } if (storageBus == StorageBus_IDE) { @@ -6039,24 +6082,6 @@ 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; @@ -6076,9 +6101,9 @@ int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, } diskCount ++; } - /* cleanup on error */ ret = 0; + cleanup: if (ret < 0) { for (i = 0; i < def->dom->ndisks; i++) -- 2.14.3
Both vboxSnapshotGetReadWriteDisks and vboxSnapshotGetReadWriteDisks do not need to free the def->disks on cleanup because it's being done by the caller via virDomainSnaphotDefFree --- src/vbox/vbox_common.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index d26ce1bce..79030e36e 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5883,13 +5883,8 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, ret = 0; cleanup: - if (ret < 0) { - for (i = 0; i < def->ndisks; i++) - VIR_FREE(def->disks[i].src); - VIR_FREE(def->disks); - def->ndisks = 0; - } VBOX_RELEASE(snap); + return ret; } @@ -6105,16 +6100,11 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, ret = 0; cleanup: - if (ret < 0) { - for (i = 0; i < def->dom->ndisks; i++) - virDomainDiskDefFree(def->dom->disks[i]); - VIR_FREE(def->dom->disks); - def->dom->ndisks = 0; - } VBOX_RELEASE(disk); VBOX_RELEASE(storageController); gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); VBOX_RELEASE(snap); + return ret; } -- 2.14.3
So that the function signature matches vboxSnapshotGetReadWriteDisks --- src/vbox/vbox_common.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 79030e36e..4d39beb1e 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -5889,8 +5889,8 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, } static int -vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot, - virDomainSnapshotDefPtr def) +vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, + virDomainSnapshotPtr snapshot) { virDomainPtr dom = snapshot->domain; vboxDriverPtr data = dom->conn->privateData; @@ -6173,7 +6173,7 @@ static char *vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot, if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0) VIR_DEBUG("Could not get read write disks for snapshot"); - if (vboxSnapshotGetReadOnlyDisks(snapshot, def) < 0) + if (vboxSnapshotGetReadOnlyDisks(def, snapshot) < 0) VIR_DEBUG("Could not get Readonly disks for snapshot"); } -- 2.14.3
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 | 192 ++++++++++++++----------------------------------- 1 file changed, 52 insertions(+), 140 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 4d39beb1e..57b0fd515 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,39 @@ 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)) return NULL; - maxPortPerInst = aMaxPortPerInst[storageBus]; - maxSlotPerPort = aMaxSlotPerPort[storageBus]; - total = (deviceInst * maxPortPerInst * maxSlotPerPort) - + (devicePort * maxSlotPerPort) - + deviceSlot; - if (storageBus == StorageBus_IDE) { prefix = "hd"; + total = devicePort * 2 + deviceSlot; } else if ((storageBus == StorageBus_SATA) || (storageBus == StorageBus_SCSI)) { 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; } @@ -3259,9 +3186,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; int ret = -1, diskCount = 0; - size_t i; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + size_t sdCount = 0, i; def->ndisks = 0; gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, @@ -3293,9 +3218,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) def->disks[i] = disk; } - if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort)) - goto cleanup; - /* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { IMediumAttachment *imediumattach = mediumAttachments.items[i]; @@ -3307,7 +3229,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) IMedium *medium = NULL; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; @@ -3348,11 +3269,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) } 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) { + sdCount++; def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; @@ -3366,24 +3306,6 @@ vboxDumpDisks(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); - - goto cleanup; - } gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) @@ -5664,9 +5586,7 @@ 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; @@ -5735,9 +5655,6 @@ 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; @@ -5747,7 +5664,6 @@ 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; @@ -5865,11 +5781,9 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, 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); } @@ -5877,6 +5791,9 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, VBOX_RELEASE(storageController); VBOX_RELEASE(disk); diskCount++; + + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI) + sdCount++; } gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); @@ -5902,10 +5819,7 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, 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) @@ -5968,9 +5882,6 @@ vboxSnapshotGetReadOnlyDisks(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->dom->ndisks; i++) { PRUnichar *storageControllerName = NULL; @@ -5979,7 +5890,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, PRBool readOnly = PR_FALSE; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; IMediumAttachment *imediumattach = mediumAttachments.items[i]; @@ -6054,11 +5964,26 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, _("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) { + 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; @@ -6080,21 +6005,8 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, 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++; } ret = 0; -- 2.14.3
On 11/07/2017 01:49 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 | 192 ++++++++++++++----------------------------------- 1 file changed, 52 insertions(+), 140 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 4d39beb1e..57b0fd515 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,39 @@ 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)) return NULL;
Since this could return NULL, you could have had the error paths on the callers also print the storageBus (something perhaps for the future)... However, also note...
- maxPortPerInst = aMaxPortPerInst[storageBus]; - maxSlotPerPort = aMaxSlotPerPort[storageBus]; - total = (deviceInst * maxPortPerInst * maxSlotPerPort) - + (devicePort * maxSlotPerPort) - + deviceSlot; - if (storageBus == StorageBus_IDE) { prefix = "hd"; + total = devicePort * 2 + deviceSlot; } else if ((storageBus == StorageBus_SATA) || (storageBus == StorageBus_SCSI)) { prefix = "sd"; + total = sdCount; } else if (storageBus == StorageBus_Floppy) { + total = deviceSlot; prefix = "fd"; }
name = virIndexToDiskName(total, prefix);
Could just return name directly ... ...the failure path in callers will overwrite the OOM from VIR_ALLOC_N - so it's kind of a conundrum as to what to do for messaging. In any case, something for the future - this is fine for now and no different than previous code which would also overwrite the OOM... John
- 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; }
@@ -3259,9 +3186,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; int ret = -1, diskCount = 0; - size_t i; - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; + size_t sdCount = 0, i;
def->ndisks = 0; gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, @@ -3293,9 +3218,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) def->disks[i] = disk; }
- if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort)) - goto cleanup; - /* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { IMediumAttachment *imediumattach = mediumAttachments.items[i]; @@ -3307,7 +3229,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) IMedium *medium = NULL; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0;
@@ -3348,11 +3269,30 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) }
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) { + sdCount++; def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; @@ -3366,24 +3306,6 @@ vboxDumpDisks(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); - - goto cleanup; - }
gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) @@ -5664,9 +5586,7 @@ 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; @@ -5735,9 +5655,6 @@ 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; @@ -5747,7 +5664,6 @@ 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; @@ -5865,11 +5781,9 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
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); } @@ -5877,6 +5791,9 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, VBOX_RELEASE(storageController); VBOX_RELEASE(disk); diskCount++; + + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI) + sdCount++; } gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments);
@@ -5902,10 +5819,7 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, 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) @@ -5968,9 +5882,6 @@ vboxSnapshotGetReadOnlyDisks(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->dom->ndisks; i++) { PRUnichar *storageControllerName = NULL; @@ -5979,7 +5890,6 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, PRBool readOnly = PR_FALSE; PRUnichar *mediumLocUtf16 = NULL; char *mediumLocUtf8 = NULL; - PRUint32 deviceInst = 0; PRInt32 devicePort = 0; PRInt32 deviceSlot = 0; IMediumAttachment *imediumattach = mediumAttachments.items[i]; @@ -6054,11 +5964,26 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, _("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) { + 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; @@ -6080,21 +6005,8 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, 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++; }
ret = 0;
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 | 190 +++++++++++++++++++++++++++++++------------------ 1 file changed, 122 insertions(+), 68 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 57b0fd515..248f315eb 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3186,6 +3186,16 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; int ret = -1, diskCount = 0; + 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; def->ndisks = 0; @@ -3194,15 +3204,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); } } @@ -3211,7 +3220,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; @@ -3220,103 +3229,140 @@ 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) { 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; @@ -3324,6 +3370,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) cleanup: gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); + if (ret < 0) { + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); + } + return ret; } -- 2.14.3
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 | 82 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 49 insertions(+), 33 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 248f315eb..661e09a27 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3185,7 +3185,7 @@ static int vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; - int ret = -1, diskCount = 0; + int ret = -1; IMediumAttachment *mediumAttachment = NULL; IMedium *medium = NULL; IStorageController *controller = NULL; @@ -3208,11 +3208,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 */ @@ -3228,7 +3232,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; @@ -3240,7 +3244,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; @@ -3252,9 +3256,6 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; } - if (!medium) - continue; - rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment, &controllerName); if (NS_FAILED(rc)) { @@ -3274,22 +3275,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, @@ -3315,11 +3300,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, @@ -3356,8 +3360,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); @@ -5788,6 +5790,13 @@ 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) + sdCount++; + VBOX_RELEASE(storageController); continue; } @@ -5996,6 +6005,13 @@ vboxSnapshotGetReadOnlyDisks(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) + sdCount++; + VBOX_RELEASE(storageController); continue; } -- 2.14.3
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 | 51 ++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 661e09a27..8da08240e 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3336,25 +3336,60 @@ 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) { sdCount++; + + break; + case StorageBus_SCSI: disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; - } else if (storageBus == StorageBus_Floppy) { + sdCount++; + + break; + case StorageBus_Floppy: disk->bus = VIR_DOMAIN_DISK_BUS_FDC; + + break; + case StorageBus_SAS: + 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; -- 2.14.3
In VirtualBox SAS and SCSI are separate controller types whereas libvirt does not make such distinction. This patch adds support for attaching the VBOX SAS controllers by mapping the 'lsisas1068' controller model in libvirt XML to VBOX SAS controller type. If VBOX VM has disks attached to both SCSI and SAS controller libvirt domain XML will have two <controller type='scsci'> elements with index and model attributes set accordingly. In this case, each respective <disk> element must have <address> element specified to assign it to respective SCSI controller. --- src/vbox/vbox_common.c | 86 ++++++++++++++++++++++++++++++++++++++++---------- src/vbox/vbox_common.h | 1 + 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 8da08240e..a1d9b2e7d 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -312,20 +312,27 @@ vboxGenerateMediumName(PRUint32 storageBus, char *name = NULL; int total = 0; - if ((storageBus < StorageBus_IDE) || - (storageBus > StorageBus_Floppy)) - return NULL; - - if (storageBus == StorageBus_IDE) { + switch ((enum StorageBus) storageBus) { + case StorageBus_IDE: prefix = "hd"; total = devicePort * 2 + deviceSlot; - } else if ((storageBus == StorageBus_SATA) || - (storageBus == StorageBus_SCSI)) { + + break; + case StorageBus_SATA: + case StorageBus_SCSI: + case StorageBus_SAS: prefix = "sd"; total = sdCount; - } else if (storageBus == StorageBus_Floppy) { + + break; + case StorageBus_Floppy: total = deviceSlot; prefix = "fd"; + + break; + case StorageBus_Null: + + return NULL; } name = virIndexToDiskName(total, prefix); @@ -391,11 +398,17 @@ vboxSetStorageController(virDomainControllerDefPtr controller, 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_LSISAS1068: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -1034,7 +1047,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; @@ -1113,6 +1126,13 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) case VIR_DOMAIN_DISK_BUS_SCSI: VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, &storageCtlName); + model = virDomainDeviceFindControllerModel(def, &disk->info, + VIR_DOMAIN_CONTROLLER_TYPE_SCSI); + if (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(VBOX_CONTROLLER_FLOPPY_NAME, &storageCtlName); @@ -3127,6 +3147,9 @@ vboxDumpStorageControllers(virDomainDefPtr def, IMachine *machine) break; case StorageControllerType_LsiLogicSas: + model = VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068; + + break; case StorageControllerType_IntelAhci: case StorageControllerType_I82078: case StorageControllerType_Null: @@ -3195,12 +3218,13 @@ 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; 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++) { @@ -3353,15 +3377,38 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) break; case StorageBus_SCSI: + case StorageBus_SAS: disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; 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_SAS: case StorageBus_Null: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unsupported null storage bus")); @@ -5829,7 +5876,8 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, * 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) + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) sdCount++; VBOX_RELEASE(storageController); @@ -5890,8 +5938,10 @@ vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def, VBOX_RELEASE(disk); diskCount++; - if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI) + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) sdCount++; + } gVBoxAPI.UArray.vboxArrayRelease(&mediumAttachments); @@ -6044,7 +6094,8 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, * 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) + if (storageBus == StorageBus_SATA || storageBus == StorageBus_SCSI || + storageBus == StorageBus_SAS) sdCount++; VBOX_RELEASE(storageController); @@ -6087,7 +6138,8 @@ vboxSnapshotGetReadOnlyDisks(virDomainSnapshotDefPtr def, } 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) { diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h index 5dc87239d..05636fea2 100644 --- a/src/vbox/vbox_common.h +++ b/src/vbox/vbox_common.h @@ -332,6 +332,7 @@ enum HardDiskVariant # 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 */ -- 2.14.3
--- docs/news.xml | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index ef855d895..454c63d8b 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -37,8 +37,82 @@ <section title="New features"> </section> <section title="Improvements"> + <change> + <summary> + vbox: Add support for configuring storage controllers + </summary> + <description> + The VirtualBox driver now supports the <code><controller></code> + element in the domain XML for configuring storage controllers in VBOX + VMs. Additionally, libvirt's domain XML schema was updated to allow + optional <code>model</code> attribute for <code><controller + type='ide'></code> which is used by the VBOX driver to set the + IDE controller model to be one of 'piix4', 'piix4' (default), or + 'ich6'. Finally, with this change <code>dumpxml</code> generates + <code><controller></code> elements that correspond to current + VBOX VM storage controller configuration. + </description> + </change> + <change> + <summary> + vbox: Add support for attaching empty removable disks + </summary> + <description> + The VirutalBox driver now supports adding CD-ROM and floppy disk + devices that do not have the disk source specified. Previously such + devices were silently ignored. + </description> + </change> + <change> + <summary> + vbox: Add support for attaching SAS storage controllers + </summary> + <description> + In VirtualBox, SCSI and SAS are distinct controller types whereas + libvirt does not make such distinction. Therefore, the VBOX driver was + updated to allow attaching SAS controllers via <code><controller + type='scsi' model='lsisas1068'></code> element. If there are + both SCSI and SAS controllers present in the VBOX VM, the domain XML + can associate the disk device using the <code><address></code> + element with the <code>controller</code> attribute, and optionally, + set the port via <code>unit</code> attribute. + </description> + </change> </section> <section title="Bug fixes"> + <change> + <summary> + vbox: Do not ignore failures to attach disk devices when defining + </summary> + <description> + The <code>define</code> now fails and reports an error if any of the + <code>controller</code> or <code>disk</code> devices specified in the + domain XML fail to attach to the VirtualBox VM. + </description> + </change> + <change> + <summary> + vbox: Fix <code>dumpxml</code> to always output disk devices + </summary> + <description> + The VirtualBox driver was ignoring any disk devices in + <code>dumpxml</code> output if there was a SAS storage controller + attached to the VM. + </description> + </change> + <change> + <summary> + vbox: Fix <code>dumpxml</code> to always generate valid domain XML + </summary> + <description> + When a VirtualBox VM has multiple disks attached, each to a different + storage controller that uses 'sd' prefix for block device names e.g. + one disk attached to SATA and one to SCSI controller, it no longer + generates XML where both would have 'sda' device name assigned. + Instead it properly assigns 'sda' and 'sdb' to those disks in the + order of appearance. + </description> + </change> </section> </release> <release version="v3.9.0" date="2017-11-02"> -- 2.14.3
On 11/07/2017 01:49 PM, Dawid Zamirski wrote:
--- docs/news.xml | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+)
Perhaps more verbose than some would like, but I'm perfectly fine with it! There's a couple of syntax things... Which I'll fix up before pushing.
diff --git a/docs/news.xml b/docs/news.xml index ef855d895..454c63d8b 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -37,8 +37,82 @@ <section title="New features"> </section> <section title="Improvements"> + <change> + <summary> + vbox: Add support for configuring storage controllers + </summary> + <description> + The VirtualBox driver now supports the <code><controller></code> + element in the domain XML for configuring storage controllers in VBOX + VMs. Additionally, libvirt's domain XML schema was updated to allow + optional <code>model</code> attribute for <code><controller + type='ide'></code> which is used by the VBOX driver to set the + IDE controller model to be one of 'piix4', 'piix4' (default), or + 'ich6'. Finally, with this change <code>dumpxml</code> generates + <code><controller></code> elements that correspond to current + VBOX VM storage controller configuration. + </description> + </change> + <change> + <summary> + vbox: Add support for attaching empty removable disks + </summary> + <description> + The VirutalBox driver now supports adding CD-ROM and floppy disk + devices that do not have the disk source specified. Previously such + devices were silently ignored. + </description> + </change> + <change> + <summary> + vbox: Add support for attaching SAS storage controllers + </summary> + <description> + In VirtualBox, SCSI and SAS are distinct controller types whereas + libvirt does not make such distinction. Therefore, the VBOX driver was + updated to allow attaching SAS controllers via <code><controller + type='scsi' model='lsisas1068'></code> element. If there are + both SCSI and SAS controllers present in the VBOX VM, the domain XML + can associate the disk device using the <code><address></code> + element with the <code>controller</code> attribute, and optionally, + set the port via <code>unit</code> attribute. + </description> + </change> </section> <section title="Bug fixes"> + <change> + <summary> + vbox: Do not ignore failures to attach disk devices when defining + </summary> + <description> + The <code>define</code> now fails and reports an error if any of the + <code>controller</code> or <code>disk</code> devices specified in the + domain XML fail to attach to the VirtualBox VM. + </description> + </change> + <change> + <summary> + vbox: Fix <code>dumpxml</code> to always output disk devices
Cannot have <code> in <summary> and there was an extra space at the end of the line.
+ </summary> + <description> + The VirtualBox driver was ignoring any disk devices in + <code>dumpxml</code> output if there was a SAS storage controller + attached to the VM. + </description> + </change> + <change> + <summary> + vbox: Fix <code>dumpxml</code> to always generate valid domain XML
Cannot have <code> in <summary.
+ </summary> + <description> + When a VirtualBox VM has multiple disks attached, each to a different + storage controller that uses 'sd' prefix for block device names e.g. + one disk attached to SATA and one to SCSI controller, it no longer + generates XML where both would have 'sda' device name assigned. + Instead it properly assigns 'sda' and 'sdb' to those disks in the + order of appearance. + </description> + </change> </section> </release> <release version="v3.9.0" date="2017-11-02">
On 11/07/2017 01:49 PM, Dawid Zamirski wrote:
v2: https://www.redhat.com/archives/libvir-list/2017-October/msg01108.html
Changes since v2: * Rebase on master * Do not segfault in 'Cleanup partially-defined VM on failure' when code jumps to cleanup and machine is NULL * Take out SAS controller handling code into a separate patch * Split up the snapshot function cleanup code into separate patches * Fixed code formatting as pointed out in review * Rename vboxDumpIDEHDDs -> vboxDumpDisks into a separate patch * Add docs/news.xml patch to describe improvements/fixes in the series
Dawid Zamirski (13): vbox: Cleanup partially-defined VM on failure vbox: Process <controller> element in domain XML vbox: Add vboxDumpStorageControllers vbox: Rename vboxDumpIDEHDDs to vboxDumpDisks vbox: Cleanup/prepare snasphot dumpxml functions vbox: Do not free disk definitions on cleanup vbox: Swap vboxSnapshotGetReadOnlyDisks arguments 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 vbox: Add SAS controller support docs: Update news.xml with vbox changes.
docs/news.xml | 74 ++++ src/vbox/vbox_common.c | 1105 +++++++++++++++++++++++++++++++----------------- src/vbox/vbox_common.h | 8 + 3 files changed, 792 insertions(+), 395 deletions(-)
I noted a couple of things along the way - one is something perhaps for later and the other I fixed up... Reviewed-by: John Ferlan <jferlan@redhat.com> (series) ... and will push in a little while... Tks, John
participants (2)
-
Dawid Zamirski -
John Ferlan