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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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
>
>
> [...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.