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(a)redhat.com&gt; 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);once vboxIID will (probably) be removed in separate patches > at some point in the future, I'd rather leave this patch "as is" > for now. > + 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); > -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list