Re: [libvirt] [PATCH v2 13/15] vbox: Cleanup vboxDumpDisks implementation

Primer the code for further changes: * move variable declarations to the top of the function * group together free/release statements * error check and report VBOX API calls used --- src/vbox/vbox_common.c | 188 +++++++++++++++++++++++++++++++------------------ 1 file changed, 120 insertions(+), 68 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 9dc36a1b2..ee6421aae 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -3202,6 +3202,16 @@ static int vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) { vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER; + IMediumAttachment *mediumAttachment = NULL; + IMedium *medium = NULL; + IStorageController *controller = NULL; + PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL; + PRUint32 deviceType, storageBus; + PRInt32 devicePort, deviceSlot; + PRBool readOnly; + nsresult rc; + virDomainDiskDefPtr disk = NULL; + char *mediumLocUtf8 = NULL; size_t sdCount = 0, i; int diskCount = 0; int ret = -1; @@ -3212,15 +3222,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) /* get the number of attachments */ for (i = 0; i < mediumAttachments.count; i++) { - IMediumAttachment *imediumattach = mediumAttachments.items[i]; - if (imediumattach) { - IMedium *medium = NULL; + mediumAttachment = mediumAttachments.items[i]; + if (!mediumAttachment) + continue; - gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); - if (medium) { - def->ndisks++; - VBOX_RELEASE(medium); - } + gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (medium) { + def->ndisks++; + VBOX_RELEASE(medium); } } @@ -3229,7 +3238,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) goto cleanup; for (i = 0; i < def->ndisks; i++) { - virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL); + disk = virDomainDiskDefNew(NULL); if (!disk) goto cleanup; @@ -3238,104 +3247,141 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) /* get the attachment details here */ for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) { - IMediumAttachment *imediumattach = mediumAttachments.items[i]; - IStorageController *storageController = NULL; - PRUnichar *storageControllerName = NULL; - PRUint32 deviceType = DeviceType_Null; - PRUint32 storageBus = StorageBus_Null; - PRBool readOnly = PR_FALSE; - IMedium *medium = NULL; - PRUnichar *mediumLocUtf16 = NULL; - char *mediumLocUtf8 = NULL; - PRInt32 devicePort = 0; - PRInt32 deviceSlot = 0; - - if (!imediumattach) + mediumAttachment = mediumAttachments.items[i]; + controller = NULL; + controllerName = NULL; + deviceType = DeviceType_Null; + storageBus = StorageBus_Null; + readOnly = PR_FALSE; + medium = NULL; + mediumLocUtf16 = NULL; + mediumLocUtf8 = NULL; + devicePort = 0; + deviceSlot = 0; + disk = def->disks[diskCount]; + + if (!mediumAttachment) continue; - gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium); + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get IMedium, rc=%08x"), rc); + goto cleanup; + } + if (!medium) continue; - gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName); - if (!storageControllerName) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment, + &controllerName);

+ if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to get storage controller name, rc=%08x"), + rc); + goto cleanup; } - gVBoxAPI.UIMachine.GetStorageControllerByName(machine, - storageControllerName, - &storageController); - VBOX_UTF16_FREE(storageControllerName); - if (!storageController) { - VBOX_RELEASE(medium); - continue; + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine, + controllerName, + &controller);

+ if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get storage controller by name, rc=%08x"), + rc); + goto cleanup; + } + + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get medium storage location, rc=%08x"), + rc); + goto cleanup; } - gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16); VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8); - VBOX_UTF16_FREE(mediumLocUtf16); - ignore_value(virDomainDiskSetSource(def->disks[diskCount], - mediumLocUtf8)); - VBOX_UTF8_FREE(mediumLocUtf8); - if (!virDomainDiskGetSource(def->disks[diskCount])) { - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not set disk source")); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device type, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumAttachment, &devicePort); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device port, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumAttachment, &deviceSlot); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get device slot, rc=%08x"), rc); + goto cleanup; + } + rc = gVBoxAPI.UIStorageController.GetBus(controller, &storageBus); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get storage controller bus, rc=%08x"), + rc); + goto cleanup; + } + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); + if (NS_FAILED(rc)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not get read only state, rc=%08x"), rc); goto cleanup; } - gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus); - gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort); - gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot); + disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot, + sdCount); - def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus, - devicePort, - deviceSlot, - sdCount); - if (!def->disks[diskCount]->dst) { + if (!disk->dst) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not generate medium name for the disk " "at: port:%d, slot:%d"), devicePort, deviceSlot); - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); - goto cleanup; } if (storageBus == StorageBus_IDE) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; + disk->bus = VIR_DOMAIN_DISK_BUS_IDE; } else if (storageBus == StorageBus_SATA) { sdCount++; - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; + disk->bus = VIR_DOMAIN_DISK_BUS_SATA; } else if (storageBus == StorageBus_SCSI || storageBus == StorageBus_SAS) { sdCount++; - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI; } else if (storageBus == StorageBus_Floppy) { - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; + disk->bus = VIR_DOMAIN_DISK_BUS_FDC; } - gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType); if (deviceType == DeviceType_HardDisk) - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK; + disk->device = VIR_DOMAIN_DISK_DEVICE_DISK; else if (deviceType == DeviceType_Floppy) - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; + disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY; else if (deviceType == DeviceType_DVD) - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM; + disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM; - - gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly); if (readOnly == PR_TRUE) - def->disks[diskCount]->src->readonly = true; + disk->src->readonly = true; - virDomainDiskSetType(def->disks[diskCount], - VIR_STORAGE_TYPE_FILE); + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE); - VBOX_RELEASE(medium); - VBOX_RELEASE(storageController); diskCount++; + + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); } ret = 0; @@ -3345,6 +3391,12 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine) /* cleanup on error */ if (ret < 0) { + VBOX_UTF16_FREE(controllerName); + VBOX_UTF8_FREE(mediumLocUtf8); + VBOX_UTF16_FREE(mediumLocUtf16); + VBOX_RELEASE(medium); + VBOX_RELEASE(controller); + for (i = 0; i < def->ndisks; i++) VIR_FREE(def->disks[i]); VIR_FREE(def->disks);