[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 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


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