Re: [libvirt] [PATCH 5/6] vbox: Process controller definitions from xml.

On 10/09/2017 04:49 PM, Dawid Zamirski wrote: > From: Dawid Zamirski <dzamirski(a)datto.com&gt; > Lots of potentially use DEBUG information being lost. IDC, but seems a shame to not have it. Although it can be overkill when DEBUG is enabled - it does help sometimes in debugging problems. Your call...

> - > - if (type == VIR_STORAGE_TYPE_FILE && src) { > - IMedium *medium = NULL; > - vboxIID mediumUUID; > - PRUnichar *mediumFileUtf16 = NULL; > - PRUint32 storageBus = StorageBus_Null; > - PRUint32 deviceType = DeviceType_Null; > - PRUint32 accessMode = AccessMode_ReadOnly; > - PRInt32 deviceInst = 0; > - PRInt32 devicePort = 0; > - PRInt32 deviceSlot = 0; > + for (i = 0; i < def->ndisks; i++) { > + disk = def->disks[i]; > + const char *src = virDomainDiskGetSource(disk); > + int type = virDomainDiskGetType(disk); > > - VBOX_IID_INITIALIZE(&mediumUUID); > - VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16); > + if (type != VIR_STORAGE_TYPE_FILE) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Unsupported storage type %s, the > only supported " > + "type is %s"), > + virStorageTypeToString(type), > + virStorageTypeToString(VIR_STORAGE_TYPE > _FILE)); > + return -1; > + } > > - if (def->disks[i]->device == > VIR_DOMAIN_DISK_DEVICE_DISK) { > - deviceType = DeviceType_HardDisk; > - accessMode = AccessMode_ReadWrite; > - } else if (def->disks[i]->device == > VIR_DOMAIN_DISK_DEVICE_CDROM) { > - deviceType = DeviceType_DVD; > - accessMode = AccessMode_ReadOnly; > - } else if (def->disks[i]->device == > VIR_DOMAIN_DISK_DEVICE_FLOPPY) { > - deviceType = DeviceType_Floppy; > - accessMode = AccessMode_ReadWrite; > - } else { > - VBOX_UTF16_FREE(mediumFileUtf16); > - continue; > - } > + if (!src && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { I think more typically there are checks against != *_CDROM || *_FLOPPY although perhaps this check should go slightly later [1] > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("Missing disk source file path")); > + return -1; > + } > > - gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, > mediumFileUtf16, > - deviceType, > accessMode, &medium); > + IMedium *medium = NULL; > + vboxIID mediumUUID; Since you call vboxIIDUnalloc later on this [2], perhaps this should be initialized to NULL... Although, I see it only gets set when "if (src)", so perhaps that becomes the key for Unalloc too. > + PRUnichar *mediumFileUtf16 = NULL; > + PRUint32 deviceType = DeviceType_Null; > + PRUint32 accessMode = AccessMode_ReadOnly; > + PRInt32 deviceSlot = disk->info.addr.drive.bus; > + PRInt32 devicePort = disk->info.addr.drive.unit; > + int model = -1; We really prefer to see definitions grouped together at the top rather than in the middle of code > + > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { [1] If you move that !src check here, then you're accomplishing the same result as previously but making it more obvious (at least for me) since _LUN isn't supported... > + deviceType = DeviceType_HardDisk; > + accessMode = AccessMode_ReadWrite; > + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { > + deviceType = DeviceType_DVD; > + accessMode = AccessMode_ReadOnly; > + } else if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) > { > + deviceType = DeviceType_Floppy; > + accessMode = AccessMode_ReadWrite; > + } else { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("Unsupported disk device type %s"), > + virDomainDiskDeviceTypeToString(disk- > >device)); > + ret = -1; > + goto cleanup; > + } Also if you change this to a : switch ((virDomainDiskDevice) disk->device) { case VIR_DOMAIN_DISK_DEVICE_DISK: ... case VIR_DOMAIN_DISK_DEVICE_LUN: VIR_DOMAIN_DISK_DEVICE_LAST: virReportError()... ... } Then you avoid the chance that someone adds a new virDomainDiskDevice and doesn't address that in the vbox code. > > - if (!medium) { > - PRUnichar *mediumEmpty = NULL; > + if (src) { > + VBOX_IID_INITIALIZE(&mediumUUID); > + VBOX_UTF8_TO_UTF16(src, &mediumFileUtf16); > > - VBOX_UTF8_TO_UTF16("", &mediumEmpty); > + rc = gVBoxAPI.UIVirtualBox.FindHardDisk(data->vboxObj, > mediumFileUtf16, > + deviceType, > accessMode, &medium); > > - rc = gVBoxAPI.UIVirtualBox.OpenMedium(data- > >vboxObj, > - mediumFileUt > f16, > - deviceType, > accessMode, > - &medium); > - VBOX_UTF16_FREE(mediumEmpty); > + /* The following is not needed for vbox 4.2+ but older > versions have > + * distinct find and open operations where former > looks in vbox media > + * registry and latter at storage location. In 4.2+, > the OpenMedium call > + * takes care of both cases internally. > + */ > + if (!medium) { > + rc = gVBoxAPI.UIVirtualBox.OpenMedium(data- > >vboxObj, mediumFileUtf16, > + deviceType, > accessMode, &medium); > } > > if (!medium) { > virReportError(VIR_ERR_INTERNAL_ERROR, > - _("Failed to attach the following > disk/dvd/floppy " > - "to the machine: %s, rc=%08x"), > + _("Failed to open the following > disk/dvd/floppy " > + "image file: %s, rc=%08x"), > src, (unsigned)rc); > - VBOX_UTF16_FREE(mediumFileUtf16); > - continue; > + ret = -1; > + 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; > + ret = -1; > + goto cleanup; > } > + } > > - if (def->disks[i]->device == > VIR_DOMAIN_DISK_DEVICE_DISK) { > - if (def->disks[i]->src->readonly) { > - gVBoxAPI.UIMedium.SetType(medium, > MediumType_Immutable); > - VIR_DEBUG("setting harddisk to immutable"); > - } else if (!def->disks[i]->src->readonly) { > - gVBoxAPI.UIMedium.SetType(medium, > MediumType_Normal); > - VIR_DEBUG("setting harddisk type to normal"); > - } > + if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { > + if (disk->src->readonly) { > + gVBoxAPI.UIMedium.SetType(medium, > MediumType_Immutable); > + } else if (!disk->src->readonly) { > + gVBoxAPI.UIMedium.SetType(medium, > MediumType_Normal); > } > + } > > - if (def->disks[i]->bus == VIR_DOMAIN_DISK_BUS_IDE) { > - VBOX_UTF8_TO_UTF16("IDE Controller", > &storageCtlName); > - storageBus = StorageBus_IDE; > - } else if (def->disks[i]->bus == > VIR_DOMAIN_DISK_BUS_SATA) { > - VBOX_UTF8_TO_UTF16("SATA Controller", > &storageCtlName); > - storageBus = StorageBus_SATA; > - } else if (def->disks[i]->bus == > VIR_DOMAIN_DISK_BUS_SCSI) { > - VBOX_UTF8_TO_UTF16("SCSI Controller", > &storageCtlName); > - storageBus = StorageBus_SCSI; > - } else if (def->disks[i]->bus == > VIR_DOMAIN_DISK_BUS_FDC) { > - VBOX_UTF8_TO_UTF16("Floppy Controller", > &storageCtlName); > - storageBus = StorageBus_Floppy; > - } > + /* asssociate <disc> bus to controller */ <disk> > > - /* get the device details i.e instance, port and slot > */ > - if (!vboxGetDeviceDetails(def->disks[i]->dst, > - maxPortPerInst, > - maxSlotPerPort, > - storageBus, > - &deviceInst, > - &devicePort, > - &deviceSlot)) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("can't get the port/slot number > of " > - "harddisk/dvd/floppy to be > attached: " > - "%s, rc=%08x"), > - src, (unsigned)rc); > - VBOX_MEDIUM_RELEASE(medium); > - vboxIIDUnalloc(&mediumUUID); > - VBOX_UTF16_FREE(mediumFileUtf16); > - continue; > - } > + switch (disk->bus) { > + case VIR_DOMAIN_DISK_BUS_IDE: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, > &storageCtlName); > > - /* attach the harddisk/dvd/Floppy to the storage > controller */ > - rc = gVBoxAPI.UIMachine.AttachDevice(machine, > - storageCtlName, > - devicePort, > - deviceSlot, > - deviceType, > - medium); > + break; > > - if (NS_FAILED(rc)) { > - virReportError(VIR_ERR_INTERNAL_ERROR, > - _("could not attach the file as " > - "harddisk/dvd/floppy: %s, > rc=%08x"), > - src, (unsigned)rc); > - } else { > - DEBUGIID("Attached HDD/DVD/Floppy with UUID", > &mediumUUID); > + case VIR_DOMAIN_DISK_BUS_SATA: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, > &storageCtlName); > + > + break; > + > + case VIR_DOMAIN_DISK_BUS_SCSI: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, > &storageCtlName); > + > + model = virDomainDeviceFindControllerModel(def, &disk- > >info, > + VIR_DOMAIN_ > CONTROLLER_TYPE_SCSI); > + > + /* if the model is lsisas1068, set vbox bus type to > SAS */ > + if (model == > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { > + VBOX_UTF16_FREE(storageCtlName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, > &storageCtlName); > } > > - VBOX_MEDIUM_RELEASE(medium); > - vboxIIDUnalloc(&mediumUUID); > - VBOX_UTF16_FREE(mediumFileUtf16); > - VBOX_UTF16_FREE(storageCtlName); > + break; > + > + case VIR_DOMAIN_DISK_BUS_FDC: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, > &storageCtlName); > + devicePort = 0; > + deviceSlot = disk->info.addr.drive.unit; > + > + break; > } > + > + /* attach the harddisk/dvd/Floppy to the storage > controller */ > + rc = gVBoxAPI.UIMachine.AttachDevice(machine, > + storageCtlName, > + devicePort, > + deviceSlot, > + deviceType, > + medium); > + > + if (NS_FAILED(rc)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Could not attach the file as " > + "harddisk/dvd/floppy: %s, rc=%08x"), > + src, (unsigned)rc); > + ret = -1; > + } you could keep the "} else { DEBUGIID("Attached HDD/DVD/Floppy with UUID", &mediumUUID); | " - your call > + > + cleanup: > + vboxIIDUnalloc(&mediumUUID); [2] This only gets set on "if (src)", so you probably need that condition here too - just to be safe... > + VBOX_MEDIUM_RELEASE(medium); > + VBOX_UTF16_FREE(mediumFileUtf16); > + VBOX_UTF16_FREE(storageCtlName); > + > + if (ret < 0) > + break; Not a normal way to have cleanup: label inside a for loop, but it does avoid having duplicated logic... > } > + > + return ret; > } > > static void > @@ -1853,6 +1875,7 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, > const char *xml, unsigned int flags > char uuidstr[VIR_UUID_STRING_BUFLEN]; > virDomainPtr ret = NULL; > unsigned int parse_flags = VIR_DOMAIN_DEF_PARSE_INACTIVE; > + int success = 0; So this is more of a "bool machineReady = false" I don't like @success, but naming is hard and whatever you pick will cause someone else to say - why not use something else... > > virCheckFlags(VIR_DOMAIN_DEFINE_VALIDATE, NULL); > > @@ -1948,7 +1971,10 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, > const char *xml, unsigned int flags > gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine); > > vboxSetBootDeviceOrder(def, data, machine); > - vboxAttachDrives(def, data, machine); > + if (vboxAttachStorageControllers(def, data, machine) < 0) > + goto cleanup; > + if (vboxAttachDrives(def, data, machine) < 0) > + goto cleanup; > vboxAttachSound(def, machine); > if (vboxAttachNetwork(def, data, machine) < 0) > goto cleanup; > @@ -1959,30 +1985,40 @@ vboxDomainDefineXMLFlags(virConnectPtr > conn, const char *xml, unsigned int flags > vboxAttachUSB(def, data, machine); > vboxAttachSharedFolder(def, data, machine); > > - /* Save the machine settings made till now and close the > - * session. also free up the mchiid variable used. > - */ > + success = 1; > + > + cleanup: > + /* Save the machine settings made till now, even if incomplete > */ > rc = gVBoxAPI.UIMachine.SaveSettings(machine); > - if (NS_FAILED(rc)) { > + if (NS_FAILED(rc)) > virReportError(VIR_ERR_INTERNAL_ERROR, > - _("failed no saving settings, rc=%08x"), > (unsigned)rc); > - goto cleanup; > + _("failed to save VM settings, rc=%08x"), > + (unsigned) rc); So if you fail to save the settings, then unlike previously you're going to call virGetDomain() which doesn't seem right. Is the (unsigned) typecast really necessary on a 08x ? > + > + if (success) { > + ret = virGetDomain(conn, def->name, def->uuid, -1); > + } else { > + /* Unregister incompletely configured VM to not leave > garbage behind */ > + gVBoxAPI.UISession.Close(data->vboxSession);> + rc > = gVBoxAPI.unregisterMachine(data, &mchiid, &machine); > + if (NS_SUCCEEDED(rc)) { > + gVBoxAPI.deleteConfig(machine); > + } else { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("could not cleanup partial domain > after failure " > + "to define, rc=%08x"), > + (unsigned) rc); > + } > } > > gVBoxAPI.UISession.Close(data->vboxSession); Prior logic allowed gVBoxAPI.UISession.Close(data->vboxSession); prior to virGetDomain(conn, def->name, def->uuid, -1); - keeping this here would mean in the } else { path from above we'd call this twice. Is that right? Can/Should it move before the "if (success) {" above? > vboxIIDUnalloc(&mchiid); > > - ret = virGetDomain(conn, def->name, def->uuid, -1); > VBOX_RELEASE(machine); > > virDomainDefFree(def); > > return ret; > - > - cleanup: > - VBOX_RELEASE(machine); > - virDomainDefFree(def); > - return NULL; > } > > static virDomainPtr > @@ -3057,8 +3093,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, > vboxDriverPtr data, IMachine *machine) > bool error = false; > int diskCount = 0; > size_t i; > - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {}; > - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {}; > + PRUint32 maxPortPerInst[StorageBus_SAS + 1] = {}; > + PRUint32 maxSlotPerPort[StorageBus_SAS + 1] = {}; > > def->ndisks = 0; > gVBoxAPI.UArray.vboxArrayGet(&mediumAttachments, machine, > @@ -3151,7 +3187,8 @@ vboxDumpIDEHDDs(virDomainDefPtr def, > vboxDriverPtr data, IMachine *machine) > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE; > } else if (storageBus == StorageBus_SATA) { > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA; > - } else if (storageBus == StorageBus_SCSI) { > + } else if (storageBus == StorageBus_SCSI || > + storageBus == StorageBus_SAS) { > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI; > } else if (storageBus == StorageBus_Floppy) { > def->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC; > diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h > index b08ad1e3e..23940fc63 100644 > --- a/src/vbox/vbox_common.h > +++ b/src/vbox/vbox_common.h > @@ -326,6 +326,14 @@ enum HardDiskVariant > # define VBOX_E_INVALID_SESSION_STATE 0x80BB000B > # define VBOX_E_OBJECT_IN_USE 0x80BB000C > > +/* VBOX storage controller name definitions */ > +# define VBOX_CONTROLLER_IDE_NAME "IDE Controller" > +# define VBOX_CONTROLLER_FLOPPY_NAME "Floppy Controller" > +# define VBOX_CONTROLLER_SATA_NAME "SATA Controller" > +# define VBOX_CONTROLLER_SCSI_NAME "SCSI Controller" > +# define VBOX_CONTROLLER_SAS_NAME "SAS Controller" > + > + > /* Simplied definitions in vbox_CAPI_*.h */ > > typedef void const *PCVBOXXPCOM; > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c > index 6592cbd63..b7ced62dc 100644 > --- a/src/vbox/vbox_tmpl.c > +++ b/src/vbox/vbox_tmpl.c I think this should be a separate patch prior to this one. You're adjusting format and removing the ATTRIBUTE_UNUSED for arguments that are actually used. > @@ -684,7 +684,9 @@ _virtualboxCreateHardDisk(IVirtualBox *vboxObj, > PRUnichar *format, > #if VBOX_API_VERSION < 5000000 > return vboxObj->vtbl->CreateHardDisk(vboxObj, format, > location, medium); > #elif VBOX_API_VERSION >= 5000000 /*VBOX_API_VERSION >= 5000000*/ > - return vboxObj->vtbl->CreateMedium(vboxObj, format, location, > AccessMode_ReadWrite, DeviceType_HardDisk, medium); > + return vboxObj->vtbl->CreateMedium(vboxObj, format, location, > + AccessMode_ReadWrite, > + DeviceType_HardDisk, > medium); > #endif /*VBOX_API_VERSION >= 5000000*/ > } > > @@ -696,37 +698,28 @@ _virtualboxRegisterMachine(IVirtualBox > *vboxObj, IMachine *machine) > > static nsresult > _virtualboxFindHardDisk(IVirtualBox *vboxObj, PRUnichar *location, > - PRUint32 deviceType ATTRIBUTE_UNUSED, > - PRUint32 accessMode ATTRIBUTE_UNUSED, > - IMedium **medium) > + PRUint32 deviceType, > + PRUint32 accessMode ATTRIBUTE_UNUSED, > IMedium **medium) Keep IMedium **medium on it's own line. It's the preferred way at least for libvirt code on function args. > { > #if VBOX_API_VERSION < 4002000 > - return vboxObj->vtbl->FindMedium(vboxObj, location, > - deviceType, medium); > + return vboxObj->vtbl->FindMedium(vboxObj, location, > deviceType, medium); > #else /* VBOX_API_VERSION >= 4002000 */ > - return vboxObj->vtbl->OpenMedium(vboxObj, location, > - deviceType, accessMode, > PR_FALSE, medium); > + return vboxObj->vtbl->OpenMedium(vboxObj, location, > deviceType, accessMode, > + PR_FALSE, medium); > #endif /* VBOX_API_VERSION >= 4002000 */ > } > > static nsresult > -_virtualboxOpenMedium(IVirtualBox *vboxObj ATTRIBUTE_UNUSED, > - PRUnichar *location ATTRIBUTE_UNUSED, > - PRUint32 deviceType ATTRIBUTE_UNUSED, > - PRUint32 accessMode ATTRIBUTE_UNUSED, > - IMedium **medium ATTRIBUTE_UNUSED) > +_virtualboxOpenMedium(IVirtualBox *vboxObj, PRUnichar *location, > + PRUint32 deviceType, PRUint32 accessMode, > + IMedium **medium) For this one - keep each argument on it's own line and just remove the ATTRIBUTE_UNUSED > { > #if VBOX_API_VERSION == 4000000 > - return vboxObj->vtbl->OpenMedium(vboxObj, > - location, > - deviceType, accessMode, > + return vboxObj->vtbl->OpenMedium(vboxObj, location, > deviceType, accessMode, > medium); > #elif VBOX_API_VERSION >= 4001000 > - return vboxObj->vtbl->OpenMedium(vboxObj, > - location, > - deviceType, accessMode, > - false, > - medium); > + return vboxObj->vtbl->OpenMedium(vboxObj, location, > deviceType, accessMode, > + false, medium); > #endif > } > > @@ -778,12 +771,8 @@ _machineGetStorageControllerByName(IMachine > *machine, PRUnichar *name, > } > > static nsresult > -_machineAttachDevice(IMachine *machine ATTRIBUTE_UNUSED, > - PRUnichar *name ATTRIBUTE_UNUSED, > - PRInt32 controllerPort ATTRIBUTE_UNUSED, > - PRInt32 device ATTRIBUTE_UNUSED, > - PRUint32 type ATTRIBUTE_UNUSED, > - IMedium * medium ATTRIBUTE_UNUSED) > +_machineAttachDevice(IMachine *machine, PRUnichar *name, PRInt32 > controllerPort, > + PRInt32 device, PRUint32 type, IMedium * > medium) Same - one line per argument is preferred John > { > return machine->vtbl->AttachDevice(machine, name, > controllerPort, > device, type, medium); >