On Fri, 2017-11-03 at 09:51 -0400, John Ferlan wrote:
>
> 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.
>
I've just checked this with VirutalBox GUI and the only thing that can
be changed on live VM is removable media (CD/DVD). All options to add
disk devices to controller are disabled when VM is running. Neither
quick web search returned anything useful regarding hot-add disk
support on VBOX.
Having said that, the original behavior was, IIRC, the reason why I
actually started working on this series. According to our tech support
they have ran into a couple of cases where the OS would BSOD (some
legacy Windows OS e.g. 2000 or XP) when there was a controller (without
a disk) attached to VM that the OS did not handle well. It sounded very
strange to me but I've been told it was not a one-off case though I
personally haven't reproduced it. Unfortunately, I won't be able to
provide more details as we no longer use VBox internally (migrated to
KVM) so those potential test cases are no longer around.
>
> > 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;
> >
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
