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

Re: [libvirt] [PATCH v2 05/15] vbox: Cleanup vboxAttachDrives implementation




On 10/24/2017 03:35 PM, Dawid Zamirski wrote:
> This commit primes vboxAttachDrives for further changes so when they
> are made, the diff is less noisy:
> 
> * move variable declarations to the top of the function
> * add disk variable to replace all the def->disks[i] instances
> * add cleanup at the end of the loop body, so it's all in one place
>   rather than scattered through the loop body. It's purposefully
>   called 'cleanup' rather than 'skip' or 'continue' because future
>   commit will treat errors as hard-failures.
> ---
>  src/vbox/vbox_common.c | 95 ++++++++++++++++++++++++--------------------------
>  1 file changed, 46 insertions(+), 49 deletions(-)
> 
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index fa8471e68..b949c4db7 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -959,8 +959,17 @@ static void
>  vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>  {
>      size_t i;
> +    int type, format;
> +    const char *src = NULL;
>      nsresult rc = 0;
> +    virDomainDiskDefPtr disk = NULL;
>      PRUnichar *storageCtlName = NULL;
> +    IMedium *medium = NULL;
> +    PRUnichar *mediumFileUtf16 = NULL, *mediumEmpty = NULL;
> +    PRUint32 devicePort, deviceSlot, deviceType, accessMode;
> +    vboxIID mediumUUID;
> +
> +    VBOX_IID_INITIALIZE(&mediumUUID);

The cleanup: label would clean this up on the first pass through the
loop, so it needs to move outside the last }.

I can do this for you; however, I have a question - something I noticed
while reviewing patch 7.  There's a call:

rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);

for which it wasn't clear whether the mediumUUID is overwritten. If it
is, then although existing - it probably needs some cleanup and of
course would change the flow a bit to doing that initializer each time
through the loop.

Moving it does have a ripple effect on subsequent patches, but it's
manageable.

If moving it to the end is acceptable, then

Reviewed-by: John Ferlan <jferlan redhat com>

otherwise let me know and I'll let you repost as part of a v3

John

>  
>      /* add a storage controller for the mediums to be attached */
>      /* this needs to change when multiple controller are supported for
> @@ -1003,57 +1012,50 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>      }
>  
>      for (i = 0; i < def->ndisks; i++) {
> -        const char *src = virDomainDiskGetSource(def->disks[i]);
> -        int type = virDomainDiskGetType(def->disks[i]);
> -        int format = virDomainDiskGetFormat(def->disks[i]);
> +        disk = def->disks[i];
> +        src = virDomainDiskGetSource(disk);
> +        type = virDomainDiskGetType(disk);
> +        format = virDomainDiskGetFormat(disk);
> +        deviceType = DeviceType_Null;
> +        accessMode = AccessMode_ReadOnly;
> +        devicePort = disk->info.addr.drive.unit;
> +        deviceSlot = disk->info.addr.drive.bus;
>  
>          VIR_DEBUG("disk(%zu) type:       %d", i, type);
> -        VIR_DEBUG("disk(%zu) device:     %d", i, def->disks[i]->device);
> -        VIR_DEBUG("disk(%zu) bus:        %d", i, def->disks[i]->bus);
> +        VIR_DEBUG("disk(%zu) device:     %d", i, disk->device);
> +        VIR_DEBUG("disk(%zu) bus:        %d", i, disk->bus);
>          VIR_DEBUG("disk(%zu) src:        %s", i, src);
> -        VIR_DEBUG("disk(%zu) dst:        %s", i, def->disks[i]->dst);
> +        VIR_DEBUG("disk(%zu) dst:        %s", i, disk->dst);
>          VIR_DEBUG("disk(%zu) driverName: %s", i,
> -                  virDomainDiskGetDriver(def->disks[i]));
> +                  virDomainDiskGetDriver(disk));
>          VIR_DEBUG("disk(%zu) driverType: %s", i,
>                    virStorageFileFormatTypeToString(format));
> -        VIR_DEBUG("disk(%zu) cachemode:  %d", i, def->disks[i]->cachemode);
> -        VIR_DEBUG("disk(%zu) readonly:   %s", i, (def->disks[i]->src->readonly
> +        VIR_DEBUG("disk(%zu) cachemode:  %d", i, disk->cachemode);
> +        VIR_DEBUG("disk(%zu) readonly:   %s", i, (disk->src->readonly
>                                               ? "True" : "False"));
> -        VIR_DEBUG("disk(%zu) shared:     %s", i, (def->disks[i]->src->shared
> +        VIR_DEBUG("disk(%zu) shared:     %s", i, (disk->src->shared
>                                               ? "True" : "False"));
>  
>          if (type == VIR_STORAGE_TYPE_FILE && src) {
> -            IMedium *medium = NULL;
> -            vboxIID mediumUUID;
> -            PRUnichar *mediumFileUtf16 = NULL;
> -            PRUint32 deviceType = DeviceType_Null;
> -            PRUint32 accessMode = AccessMode_ReadOnly;
> -            PRInt32 devicePort = def->disks[i]->info.addr.drive.unit;
> -            PRInt32 deviceSlot = def->disks[i]->info.addr.drive.bus;
> -
> -            VBOX_IID_INITIALIZE(&mediumUUID);
>              VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16);
>  
> -            if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> +            if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
>                  deviceType = DeviceType_HardDisk;
>                  accessMode = AccessMode_ReadWrite;
> -            } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
> +            } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) {
>                  deviceType = DeviceType_DVD;
>                  accessMode = AccessMode_ReadOnly;
> -            } else if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
> +            } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {
>                  deviceType = DeviceType_Floppy;
>                  accessMode = AccessMode_ReadWrite;
>              } else {
> -                VBOX_UTF16_FREE(mediumFileUtf16);
> -                continue;
> +                goto cleanup;
>              }
>  
>              gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, mediumFileUtf16,
>                                                 deviceType, accessMode, &medium);
>  
>              if (!medium) {
> -                PRUnichar *mediumEmpty = NULL;
> -
>                  VBOX_UTF8_TO_UTF16("", &mediumEmpty);
>  
>                  rc = gVBoxAPI.UIVirtualBox.OpenMedium(data->vboxObj,
> @@ -1066,45 +1068,41 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>              if (!medium) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
>                                 _("Failed to attach the following disk/dvd/floppy "
> -                                 "to the machine: %s, rc=%08x"),
> -                               src, (unsigned)rc);
> -                VBOX_UTF16_FREE(mediumFileUtf16);
> -                continue;
> +                                 "to the machine: %s, rc=%08x"), src, rc);
> +                goto cleanup;
>              }
>  
>              rc = gVBoxAPI.UIMedium.GetId(medium, &mediumUUID);
>              if (NS_FAILED(rc)) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("can't get the uuid of the file to be attached "
> +                               _("Can't get the UUID of the file to be attached "
>                                   "as harddisk/dvd/floppy: %s, rc=%08x"),
> -                               src, (unsigned)rc);
> -                VBOX_MEDIUM_RELEASE(medium);
> -                VBOX_UTF16_FREE(mediumFileUtf16);
> -                continue;
> +                               src, rc);
> +                goto cleanup;
>              }
>  
> -            if (def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> -                if (def->disks[i]->src->readonly) {
> +            if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) {
> +                if (disk->src->readonly) {
>                      gVBoxAPI.UIMedium.SetType(medium, MediumType_Immutable);
> -                    VIR_DEBUG("setting harddisk to immutable");
> -                } else if (!def->disks[i]->src->readonly) {
> +                    VIR_DEBUG("Setting harddisk to immutable");
> +                } else if (!disk->src->readonly) {
>                      gVBoxAPI.UIMedium.SetType(medium, MediumType_Normal);
> -                    VIR_DEBUG("setting harddisk type to normal");
> +                    VIR_DEBUG("Setting harddisk type to normal");
>                  }
>              }
>  
> -            if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) {
> +            if (disk->bus == VIR_DOMAIN_DISK_BUS_IDE) {
>                  VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName);
>                  devicePort = def->disks[i]->info.addr.drive.bus;
>                  deviceSlot = def->disks[i]->info.addr.drive.unit;
> -            } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SATA) {
> +            } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SATA) {
>                  VBOX_UTF8_TO_UTF16("SATA Controller", &storageCtlName);
> -            } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
> +            } else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
>                  VBOX_UTF8_TO_UTF16("SCSI Controller", &storageCtlName);
> -            } else if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_FDC) {
> +            } else if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) {
>                  VBOX_UTF8_TO_UTF16("Floppy Controller", &storageCtlName);
>                  devicePort = 0;
> -                deviceSlot = def->disks[i]->info.addr.drive.unit;
> +                deviceSlot = disk->info.addr.drive.unit;
>              }
>  
>              /* attach the harddisk/dvd/Floppy to the storage controller */
> @@ -1117,13 +1115,12 @@ vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
>  
>              if (NS_FAILED(rc)) {
>                  virReportError(VIR_ERR_INTERNAL_ERROR,
> -                               _("could not attach the file as "
> -                                 "harddisk/dvd/floppy: %s, rc=%08x"),
> -                               src, (unsigned)rc);
> +                               _("Could not attach the file as "
> +                                 "harddisk/dvd/floppy: %s, rc=%08x"), src, rc);
>              } else {
>                  DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID);
>              }
> -
> + cleanup:
>              VBOX_MEDIUM_RELEASE(medium);
>              vboxIIDUnalloc(&mediumUUID);
>              VBOX_UTF16_FREE(mediumFileUtf16);
> 


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