8:43 a.m.
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>
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);