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);
/* 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);
--
2.14.2
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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);
>
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
>
> 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 }.
>
The VBOX_IID_INITIALIZE macro is a simple struct initializer that is
equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact,
src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly
that, but that macro is not available to vbox_common.c This is likely
another remanent of VBOX <= 3 support that needs to be cleaned up - I
will do so in a separate patch as it will touch to many parts of the
driver and when you take a closer look we don't really need vboxIID
struct at all and can simply use PRUnichar directly for VBOX UUIDs.
> 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.
Yes, it is overwritten but since the cleanup: label is at the end of
the loop body it will set mediumUIID to {NULL, true} which is the same
as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in
src/vbox/vbox_tmpl.c)
>
> Moving it does have a ripple effect on subsequent patches, but it's
> manageable.
>
> If moving it to the end is acceptable, then
As per above, I'd rather leave this patch "as is" as the (future)
driver cleanup patches for vboxIID will make this clearer.
>
> 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);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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 11/03/2017 12:19 PM, Dawid Zamirski wrote:
> On Fri, 2017-11-03 at 09:43 -0400, John Ferlan wrote:
>>
>> 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 }.
>>
>
> The VBOX_IID_INITIALIZE macro is a simple struct initializer that is
> equivalent of vboxIID mediumUUID = {NULL, true}; as a matter of fact,
> src/vbox/vbox_tmpl.c uses VBOX_IID_INITIALIZER macro which is exactly
> that, but that macro is not available to vbox_common.c This is likely
> another remanent of VBOX <= 3 support that needs to be cleaned up - I
> will do so in a separate patch as it will touch to many parts of the
> driver and when you take a closer look we don't really need vboxIID
> struct at all and can simply use PRUnichar directly for VBOX UUIDs.
>
OK it really wasn't overly intuitively obvious what VBOX_IID_INITIALIZE
a/k/a gVBoxAPI.UIID.vboxIIDInitialize really did, your explanation makes
sense.
>> 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.
>
> Yes, it is overwritten but since the cleanup: label is at the end of
> the loop body it will set mediumUIID to {NULL, true} which is the same
> as calling VBOX_IID_INITIALIZE (see _vboxIIDUnalloc in
> src/vbox/vbox_tmpl.c)
>
>>
>> Moving it does have a ripple effect on subsequent patches, but it's
>> manageable.
>>
>> If moving it to the end is acceptable, then
>
> As per above, I'd rather leave this patch "as is" as the (future)
> driver cleanup patches for vboxIID will make this clearer.
>
OK I'll leave it as is. Patches 1-2 and 4-9 are thus all R-B and will
be pushed in a little while.
John
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> otherwise let me know and I'll let you repost as part of a v3
>>
>> John
>>
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.