Re: [libvirt] [PATCH v2 10/15] vbox: Process <controller> element in domain XML

On 10/24/2017 03:35 PM, Dawid Zamirski wrote: > This patch enables the VBOX driver to process the <controller> > element > in domain XML through which one can now customize the controller > model. > > Since VirtualBox has two distinct SAS and SCSI they do not "map" > directly to libvirt XML, he VBOX driver uses <contoller type="scsi" > model="lsisas1068" /> to create SAS controller in VBOX VM. > Additionally > once can set model on the IDE controller to be one of "piix3", > "piix4" > or "ich6". > --- > src/vbox/vbox_common.c | 214 > ++++++++++++++++++++++++++++++++++++++----------- > src/vbox/vbox_common.h | 8 ++ > 2 files changed, 176 insertions(+), 46 deletions(-) > So beyond patch 3 which I know you have to repost anyway - starting with this patch and going forward, I figure you have to repost anyway as long as I get the question in patch 5 answered of course... This patch left me with a concern. Up to this point vboxAttachDrives would add at least one controller for each type supported regardless of whether the VM used it. After this patch only those controllers defined in the VM XML will be added. Which would seem to be fine, except for the case of hotplug which I wasn't clear whether vbox support or not. Let's say the VM is defined with and IDE controller, if someone tried to add a SCSI disk after startup, then there'd be no SCSI controller already present.

> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c > index 2bd891efb..9d45e4a76 100644 > --- a/src/vbox/vbox_common.c > +++ b/src/vbox/vbox_common.c > @@ -406,6 +406,160 @@ static char *vboxGenerateMediumName(PRUint32 > storageBus, > return name; > } > > + > +static int > +vboxSetStorageController(virDomainControllerDefPtr controller, > + vboxDriverPtr data, > + IMachine *machine) > +{ > + PRUnichar *controllerName = NULL; > + PRInt32 vboxModel = StorageControllerType_Null; > + PRInt32 vboxBusType = StorageBus_Null; > + IStorageController *vboxController = NULL; > + nsresult rc = 0; > + char *debugName = NULL; > + int ret = -1; > + > + /* libvirt controller type => vbox bus type */ > + switch ((virDomainControllerType) controller->type) { > + case VIR_DOMAIN_CONTROLLER_TYPE_FDC: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, > &controllerName); > + vboxBusType = StorageBus_Floppy; > + > + break; > + case VIR_DOMAIN_CONTROLLER_TYPE_IDE: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, > &controllerName); > + vboxBusType = StorageBus_IDE; > + > + break; > + case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, > &controllerName); > + vboxBusType = StorageBus_SCSI; > + > + break; > + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, > &controllerName); > + vboxBusType = StorageBus_SATA; I think prior to this patch - there should be a patch that "manages" all those range check differences, storageBus comparison checks, etc. for VIR_DOMAIN_CONTROLLER_TYPE_SATA that end up in future patches. John > + > + break; > + case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: > + case VIR_DOMAIN_CONTROLLER_TYPE_CCID: > + case VIR_DOMAIN_CONTROLLER_TYPE_USB: > + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: > + case VIR_DOMAIN_CONTROLLER_TYPE_LAST: > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("The vbox driver does not support %s > controller type"), > + virDomainControllerTypeToString(controller- > >type)); > + return -1; > + } > + > + /* libvirt scsi model => vbox scsi model */ > + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) { > + switch ((virDomainControllerModelSCSI) controller->model) > { > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC: > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_AUTO: > + vboxModel = StorageControllerType_LsiLogic; > + > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_BUSLOGIC: > + vboxModel = StorageControllerType_BusLogic; > + > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068: > + /* in vbox, lsisas has a dedicated SAS bus type with > no model */ > + VBOX_UTF16_FREE(controllerName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, > &controllerName); > + vboxBusType = StorageBus_SAS; > + > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VMPVSCSI: > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_IBMVSCSI: > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI: > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1078: > + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LAST: > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("The vbox driver does not support %s > SCSI " > + "controller model"), > + virDomainControllerModelSCSITypeToStrin > g(controller->model)); > + goto cleanup; > + } > + /* libvirt ide model => vbox ide model */ > + } else if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) > { > + switch ((virDomainControllerModelIDE) controller->model) { > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX3: > + vboxModel = StorageControllerType_PIIX3; > + > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_PIIX4: > + vboxModel = StorageControllerType_PIIX4; > + > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_ICH6: > + vboxModel = StorageControllerType_ICH6; > + > + break; > + case VIR_DOMAIN_CONTROLLER_MODEL_IDE_LAST: > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("The vbox driver does not support %s > IDE " > + "controller model"), > + virDomainControllerModelIDETypeToStri > ng(controller->model)); > + goto cleanup; > + } > + } > + > + VBOX_UTF16_TO_UTF8(controllerName, &debugName); > + VIR_DEBUG("Adding VBOX storage controller (name: %s, busType: > %d)", > + debugName, vboxBusType); > + > + rc = gVBoxAPI.UIMachine.AddStorageController(machine, > controllerName, > + vboxBusType, > &vboxController); > + > + if (NS_FAILED(rc)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to add storage controller " > + "(name: %s, busType: %d), rc=%08x"), > + debugName, vboxBusType, rc); > + goto cleanup; > + } > + > + /* only IDE or SCSI controller have model choices */ > + if (vboxModel != StorageControllerType_Null) { > + rc = > gVBoxAPI.UIStorageController.SetControllerType(vboxController, > + vboxMo > del); > + if (NS_FAILED(rc)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to change storage controller > model, " > + "rc=%08x"), rc); > + goto cleanup; > + } > + } > + > + ret = 0; > + > + cleanup: > + VBOX_UTF16_FREE(controllerName); > + VBOX_UTF8_FREE(debugName); > + VBOX_RELEASE(vboxController); > + > + return ret; > +} > + > + > +static int > +vboxAttachStorageControllers(virDomainDefPtr def, > + vboxDriverPtr data, > + IMachine *machine) > +{ > + size_t i; > + for (i = 0; i < def->ncontrollers; i++) { > + if (vboxSetStorageController(def->controllers[i], data, > machine) < 0) > + return -1; > + } > + > + return 0; > +} > + > + > static virDrvOpenStatus > vboxConnectOpen(virConnectPtr conn, > virConnectAuthPtr auth ATTRIBUTE_UNUSED, > @@ -959,7 +1113,7 @@ static int > vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine > *machine) > { > size_t i; > - int type, ret = 0; > + int type, ret = 0, model = -1; > const char *src = NULL; > nsresult rc = 0; > virDomainDiskDefPtr disk = NULL; > @@ -972,46 +1126,6 @@ vboxAttachDrives(virDomainDefPtr def, > vboxDriverPtr data, IMachine *machine) > > VBOX_IID_INITIALIZE(&mediumUUID); > > - /* add a storage controller for the mediums to be attached */ > - /* this needs to change when multiple controller are supported > for > - * ver > 3.1 */ > - { > - IStorageController *storageCtl = NULL; > - PRUnichar *sName = NULL; > - > - VBOX_UTF8_TO_UTF16("IDE Controller", &sName); > - gVBoxAPI.UIMachine.AddStorageController(machine, > - sName, > - StorageBus_IDE, > - &storageCtl); > - VBOX_UTF16_FREE(sName); > - VBOX_RELEASE(storageCtl); > - > - VBOX_UTF8_TO_UTF16("SATA Controller", &sName); > - gVBoxAPI.UIMachine.AddStorageController(machine, > - sName, > - StorageBus_SATA, > - &storageCtl); > - VBOX_UTF16_FREE(sName); > - VBOX_RELEASE(storageCtl); > - > - VBOX_UTF8_TO_UTF16("SCSI Controller", &sName); > - gVBoxAPI.UIMachine.AddStorageController(machine, > - sName, > - StorageBus_SCSI, > - &storageCtl); > - VBOX_UTF16_FREE(sName); > - VBOX_RELEASE(storageCtl); > - > - VBOX_UTF8_TO_UTF16("Floppy Controller", &sName); > - gVBoxAPI.UIMachine.AddStorageController(machine, > - sName, > - StorageBus_Floppy, > - &storageCtl); > - VBOX_UTF16_FREE(sName); > - VBOX_RELEASE(storageCtl); > - } > - > for (i = 0; i < def->ndisks; i++) { > disk = def->disks[i]; > src = virDomainDiskGetSource(disk); > @@ -1066,21 +1180,28 @@ vboxAttachDrives(virDomainDefPtr def, > vboxDriverPtr data, IMachine *machine) > > switch ((virDomainDiskBus) disk->bus) { > case VIR_DOMAIN_DISK_BUS_IDE: > - VBOX_UTF8_TO_UTF16("IDE Controller", &storageCtlName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_IDE_NAME, > &storageCtlName); > devicePort = def->disks[i]->info.addr.drive.bus; > deviceSlot = def->disks[i]->info.addr.drive.unit; > > break; > case VIR_DOMAIN_DISK_BUS_SATA: > - VBOX_UTF8_TO_UTF16("SATA Controller", > &storageCtlName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SATA_NAME, > &storageCtlName); > > break; > case VIR_DOMAIN_DISK_BUS_SCSI: > - VBOX_UTF8_TO_UTF16("SCSI Controller", > &storageCtlName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SCSI_NAME, > &storageCtlName); > + > + model = virDomainDeviceFindControllerModel(def, &disk- > >info, > + VIR_DOMAIN_ > CONTROLLER_TYPE_SCSI); > + if (model == > VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSISAS1068) { > + VBOX_UTF16_FREE(storageCtlName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_SAS_NAME, > &storageCtlName); > + } > > break; > case VIR_DOMAIN_DISK_BUS_FDC: > - VBOX_UTF8_TO_UTF16("Floppy Controller", > &storageCtlName); > + VBOX_UTF8_TO_UTF16(VBOX_CONTROLLER_FLOPPY_NAME, > &storageCtlName); > devicePort = 0; > deviceSlot = disk->info.addr.drive.unit; > > @@ -1148,7 +1269,6 @@ vboxAttachDrives(virDomainDefPtr def, > vboxDriverPtr data, IMachine *machine) > gVBoxAPI.UIMedium.SetType(medium, > MediumType_Normal); > VIR_DEBUG("Setting hard disk type to normal"); > } > - > } > > VBOX_UTF16_TO_UTF8(storageCtlName, &controllerName); > @@ -1918,6 +2038,8 @@ vboxDomainDefineXMLFlags(virConnectPtr conn, > const char *xml, unsigned int flags > gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine); > > vboxSetBootDeviceOrder(def, data, machine); > + if (vboxAttachStorageControllers(def, data, machine) < 0) > + goto cleanup; > if (vboxAttachDrives(def, data, machine) < 0) > goto cleanup; > vboxAttachSound(def, machine); > diff --git a/src/vbox/vbox_common.h b/src/vbox/vbox_common.h > index b08ad1e3e..3340374c1 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; >