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@xxxxxxxxxx>
>
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
