[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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

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

One more space each for arg2 and arg3

The rest looks OK... with the above adjustments...

Reviewed-by: John Ferlan <jferlan redhat com>

John


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]