On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> Primer the code for further changes:
>
> * move variable declarations to the top of the function
> * group together free/release statements
> * error check and report VBOX API calls used
> ---
> src/vbox/vbox_common.c | 188 +++++++++++++++++++++++++++++++------------------
> 1 file changed, 120 insertions(+), 68 deletions(-)
>
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 9dc36a1b2..ee6421aae 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -3202,6 +3202,16 @@ static int
> vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
> {
> vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
> + IMediumAttachment *mediumAttachment = NULL;
> + IMedium *medium = NULL;
> + IStorageController *controller = NULL;
> + PRUnichar *controllerName = NULL, *mediumLocUtf16 = NULL;
> + PRUint32 deviceType, storageBus;
> + PRInt32 devicePort, deviceSlot;
> + PRBool readOnly;
> + nsresult rc;
> + virDomainDiskDefPtr disk = NULL;
> + char *mediumLocUtf8 = NULL;
> size_t sdCount = 0, i;
> int diskCount = 0;
> int ret = -1;
> @@ -3212,15 +3222,14 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>
> /* get the number of attachments */
> for (i = 0; i < mediumAttachments.count; i++) {
> - IMediumAttachment *imediumattach = mediumAttachments.items[i];
> - if (imediumattach) {
> - IMedium *medium = NULL;
> + mediumAttachment = mediumAttachments.items[i];
> + if (!mediumAttachment)
> + continue;
>
> - gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
> - if (medium) {
> - def->ndisks++;
> - VBOX_RELEASE(medium);
> - }
> + gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
> + if (medium) {
> + def->ndisks++;
> + VBOX_RELEASE(medium);
> }
> }
>
> @@ -3229,7 +3238,7 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
> goto cleanup;
>
> for (i = 0; i < def->ndisks; i++) {
> - virDomainDiskDefPtr disk = virDomainDiskDefNew(NULL);
> + disk = virDomainDiskDefNew(NULL);
> if (!disk)
> goto cleanup;
>
> @@ -3238,104 +3247,141 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>
> /* get the attachment details here */
> for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks; i++) {
> - IMediumAttachment *imediumattach = mediumAttachments.items[i];
> - IStorageController *storageController = NULL;
> - PRUnichar *storageControllerName = NULL;
> - PRUint32 deviceType = DeviceType_Null;
> - PRUint32 storageBus = StorageBus_Null;
> - PRBool readOnly = PR_FALSE;
> - IMedium *medium = NULL;
> - PRUnichar *mediumLocUtf16 = NULL;
> - char *mediumLocUtf8 = NULL;
> - PRInt32 devicePort = 0;
> - PRInt32 deviceSlot = 0;
> -
> - if (!imediumattach)
> + mediumAttachment = mediumAttachments.items[i];
> + controller = NULL;
> + controllerName = NULL;
> + deviceType = DeviceType_Null;
> + storageBus = StorageBus_Null;
> + readOnly = PR_FALSE;
> + medium = NULL;
> + mediumLocUtf16 = NULL;
> + mediumLocUtf8 = NULL;
> + devicePort = 0;
> + deviceSlot = 0;
> + disk = def->disks[diskCount];
> +
> + if (!mediumAttachment)
> continue;
>
> - gVBoxAPI.UIMediumAttachment.GetMedium(imediumattach, &medium);
> + rc = gVBoxAPI.UIMediumAttachment.GetMedium(mediumAttachment, &medium);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get IMedium, rc=%08x"), rc);
> + goto cleanup;
> + }
> +
> if (!medium)
> continue;
>
> - gVBoxAPI.UIMediumAttachment.GetController(imediumattach, &storageControllerName);
> - if (!storageControllerName) {
> - VBOX_RELEASE(medium);
> - continue;
> + rc = gVBoxAPI.UIMediumAttachment.GetController(mediumAttachment,
> + &controllerName);
^^^^^
Alignment for second line... Needs 5 more spaces right...
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to get storage controller name, rc=%08x"),
> + rc);
> + goto cleanup;
> }
>
> - gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
> - storageControllerName,
> - &storageController);
> - VBOX_UTF16_FREE(storageControllerName);
> - if (!storageController) {
> - VBOX_RELEASE(medium);
> - continue;
> + rc = gVBoxAPI.UIMachine.GetStorageControllerByName(machine,
> + controllerName,
> + &controller);
One more space each for arg2 and arg3
The rest looks OK... with the above adjustments...
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get storage controller by name, rc=%08x"),
> + rc);
> + goto cleanup;
> + }
> +
> + rc = gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get medium storage location, rc=%08x"),
> + rc);
> + goto cleanup;
> }
>
> - gVBoxAPI.UIMedium.GetLocation(medium, &mediumLocUtf16);
> VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
> - VBOX_UTF16_FREE(mediumLocUtf16);
> - ignore_value(virDomainDiskSetSource(def->disks[diskCount],
> - mediumLocUtf8));
> - VBOX_UTF8_FREE(mediumLocUtf8);
>
> - if (!virDomainDiskGetSource(def->disks[diskCount])) {
> - VBOX_RELEASE(medium);
> - VBOX_RELEASE(storageController);
> + if (virDomainDiskSetSource(disk, mediumLocUtf8) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Could not set disk source"));
> + goto cleanup;
> + }
>
> + rc = gVBoxAPI.UIMediumAttachment.GetType(mediumAttachment, &deviceType);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get device type, rc=%08x"), rc);
> + goto cleanup;
> + }
> + rc = gVBoxAPI.UIMediumAttachment.GetPort(mediumAttachment, &devicePort);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get device port, rc=%08x"), rc);
> + goto cleanup;
> + }
> + rc = gVBoxAPI.UIMediumAttachment.GetDevice(mediumAttachment, &deviceSlot);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get device slot, rc=%08x"), rc);
> + goto cleanup;
> + }
> + rc = gVBoxAPI.UIStorageController.GetBus(controller, &storageBus);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get storage controller bus, rc=%08x"),
> + rc);
> + goto cleanup;
> + }
> + rc = gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not get read only state, rc=%08x"), rc);
> goto cleanup;
> }
>
> - gVBoxAPI.UIStorageController.GetBus(storageController, &storageBus);
> - gVBoxAPI.UIMediumAttachment.GetPort(imediumattach, &devicePort);
> - gVBoxAPI.UIMediumAttachment.GetDevice(imediumattach, &deviceSlot);
> + disk->dst = vboxGenerateMediumName(storageBus, devicePort, deviceSlot,
> + sdCount);
>
> - def->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
> - devicePort,
> - deviceSlot,
> - sdCount);
> - if (!def->disks[diskCount]->dst) {
> + if (!disk->dst) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Could not generate medium name for the disk "
> "at: port:%d, slot:%d"), devicePort, deviceSlot);
> - VBOX_RELEASE(medium);
> - VBOX_RELEASE(storageController);
> -
> goto cleanup;
> }
>
> if (storageBus == StorageBus_IDE) {
> - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
> + disk->bus = VIR_DOMAIN_DISK_BUS_IDE;
> } else if (storageBus == StorageBus_SATA) {
> sdCount++;
> - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
> + disk->bus = VIR_DOMAIN_DISK_BUS_SATA;
> } else if (storageBus == StorageBus_SCSI ||
> storageBus == StorageBus_SAS) {
> sdCount++;
> - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> + disk->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> } else if (storageBus == StorageBus_Floppy) {
> - def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
> + disk->bus = VIR_DOMAIN_DISK_BUS_FDC;
> }
>
> - gVBoxAPI.UIMediumAttachment.GetType(imediumattach, &deviceType);
> if (deviceType == DeviceType_HardDisk)
> - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK;
> + disk->device = VIR_DOMAIN_DISK_DEVICE_DISK;
> else if (deviceType == DeviceType_Floppy)
> - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
> + disk->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
> else if (deviceType == DeviceType_DVD)
> - def->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
> + disk->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
>
> -
> - gVBoxAPI.UIMedium.GetReadOnly(medium, &readOnly);
> if (readOnly == PR_TRUE)
> - def->disks[diskCount]->src->readonly = true;
> + disk->src->readonly = true;
>
> - virDomainDiskSetType(def->disks[diskCount],
> - VIR_STORAGE_TYPE_FILE);
> + virDomainDiskSetType(disk, VIR_STORAGE_TYPE_FILE);
>
> - VBOX_RELEASE(medium);
> - VBOX_RELEASE(storageController);
> diskCount++;
> +
> + VBOX_UTF16_FREE(controllerName);
> + VBOX_UTF8_FREE(mediumLocUtf8);
> + VBOX_UTF16_FREE(mediumLocUtf16);
> + VBOX_RELEASE(medium);
> + VBOX_RELEASE(controller);
> }
>
> ret = 0;
> @@ -3345,6 +3391,12 @@ vboxDumpDisks(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>
> /* cleanup on error */
> if (ret < 0) {
> + VBOX_UTF16_FREE(controllerName);
> + VBOX_UTF8_FREE(mediumLocUtf8);
> + VBOX_UTF16_FREE(mediumLocUtf16);
> + VBOX_RELEASE(medium);
> + VBOX_RELEASE(controller);
> +
> for (i = 0; i < def->ndisks; i++)
> VIR_FREE(def->disks[i]);
> VIR_FREE(def->disks);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
