On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> From: Dawid Zamirski <dzamirski@xxxxxxxxx>
>
> Until now the vbox driver was completely ignoring <controller> element
ignoring the <controller>
> for storage in the XML definition. This patch adds support for
> interpretting <controller> element for storage devices. With this the
interpreting the <controller>
> following other changes were made to the whole storage attachment code:
>
> * vboxAttachDrives no longer "computes" deviceSlot and devicePort
> values based on the disk device name. This was causing the driver to
> ignore the values set by <address> element of the <disk> device. If
> that element is omitted in XML, the values produced by default by
> virDomainDiskDefAssign address should work well.
> * if any part of storage attachment code fails, i.e wherever we call
> virReportError, we also fail the DefineXML caller rather than ignoring
> those issues and moving on.
This particular code (checking vboxAttachDrives status) probably could
have been pulled out and made into it's own patch.
> * the DefineXML cleanup part of the code was changed to make sure that
> any critical failure in device attachment code does not leave any
> partially defined VM behind.
> * do not require disk source for removable drives so that "empty"
> drives can be created for cdrom and floppy types.
What's "StorageBus_SAS" and why was it not mentioned? Seems to be
something that should be separated into a patch just before this one in
order to reduce the number of changes in this patch.
> ---
> src/vbox/vbox_common.c | 535 ++++++++++++++++++++++++++-----------------------
> src/vbox/vbox_common.h | 8 +
> src/vbox/vbox_tmpl.c | 43 ++--
> 3 files changed, 310 insertions(+), 276 deletions(-)
>
> diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
> index 92ee37164..7645b29a0 100644
> --- a/src/vbox/vbox_common.c
> +++ b/src/vbox/vbox_common.c
> @@ -324,6 +324,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> StorageBus_SCSI,
> &maxPortPerInst[StorageBus_SCSI]);
> + gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> + StorageBus_SAS,
> + &maxPortPerInst[StorageBus_SAS]);
> gVBoxAPI.UISystemProperties.GetMaxPortCountForStorageBus(sysProps,
> StorageBus_Floppy,
> &maxPortPerInst[StorageBus_Floppy]);
> @@ -337,6 +340,9 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> StorageBus_SCSI,
> &maxSlotPerPort[StorageBus_SCSI]);
> + gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> + StorageBus_SAS,
> + &maxSlotPerPort[StorageBus_SAS]);
> gVBoxAPI.UISystemProperties.GetMaxDevicesPerPortForStorageBus(sysProps,
> StorageBus_Floppy,
> &maxSlotPerPort[StorageBus_Floppy]);
> @@ -346,68 +352,6 @@ static bool vboxGetMaxPortSlotValues(IVirtualBox *vbox,
> return true;
> }
>
> -/**
> - * function to get the StorageBus, Port number
> - * and Device number for the given devicename
> - * e.g: hda has StorageBus = IDE, port = 0,
> - * device = 0
> - *
> - * @returns true on Success, false on failure.
> - * @param deviceName Input device name
> - * @param aMaxPortPerInst Input array of max port per device instance
> - * @param aMaxSlotPerPort Input array of max slot per device port
> - * @param storageBus Input storage bus type
> - * @param deviceInst Output device instance number
> - * @param devicePort Output port number
> - * @param deviceSlot Output slot number
> - *
> - */
> -static bool vboxGetDeviceDetails(const char *deviceName,
> - PRUint32 *aMaxPortPerInst,
> - PRUint32 *aMaxSlotPerPort,
> - PRUint32 storageBus,
> - PRInt32 *deviceInst,
> - PRInt32 *devicePort,
> - PRInt32 *deviceSlot)
> -{
> - int total = 0;
> - PRUint32 maxPortPerInst = 0;
> - PRUint32 maxSlotPerPort = 0;
> -
> - if (!deviceName ||
> - !deviceInst ||
> - !devicePort ||
> - !deviceSlot ||
> - !aMaxPortPerInst ||
> - !aMaxSlotPerPort)
> - return false;
> -
> - if ((storageBus < StorageBus_IDE) ||
> - (storageBus > StorageBus_Floppy))
NB: If you make that _SAS specific patch this would probably have to
change too at least for the one patch to be "complete"
> - return false;
> -
> - total = virDiskNameToIndex(deviceName);
> -
> - maxPortPerInst = aMaxPortPerInst[storageBus];
> - maxSlotPerPort = aMaxSlotPerPort[storageBus];
> -
> - if (!maxPortPerInst ||
> - !maxSlotPerPort ||
> - (total < 0))
> - return false;
> -
> - *deviceInst = total / (maxPortPerInst * maxSlotPerPort);
> - *devicePort = (total % (maxPortPerInst * maxSlotPerPort)) / maxSlotPerPort;
> - *deviceSlot = (total % (maxPortPerInst * maxSlotPerPort)) % maxSlotPerPort;
> -
> - VIR_DEBUG("name=%s, total=%d, storageBus=%u, deviceInst=%d, "
> - "devicePort=%d deviceSlot=%d, maxPortPerInst=%u maxSlotPerPort=%u",
> - deviceName, total, storageBus, *deviceInst, *devicePort,
> - *deviceSlot, maxPortPerInst, maxSlotPerPort);
> -
> - return true;
> -}
> -
> /**
> * function to generate the name for medium,
> * for e.g: hda, sda, etc
> @@ -441,7 +385,7 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
> return NULL;
>
> if ((storageBus < StorageBus_IDE) ||
> - (storageBus > StorageBus_Floppy))
> + (storageBus > StorageBus_SAS))
> return NULL;
>
> maxPortPerInst = aMaxPortPerInst[storageBus];
> @@ -452,8 +396,9 @@ static char *vboxGenerateMediumName(PRUint32 storageBus,
>
> if (storageBus == StorageBus_IDE) {
> prefix = "hd";
> - } else if ((storageBus == StorageBus_SATA) ||
> - (storageBus == StorageBus_SCSI)) {
> + } else if (storageBus == StorageBus_SATA ||
> + storageBus == StorageBus_SCSI ||
> + storageBus == StorageBus_SAS) {
> prefix = "sd";
> } else if (storageBus == StorageBus_Floppy) {
> prefix = "fd";
> @@ -468,6 +413,124 @@ 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;
> + int ret = -1;
> +
> + /* libvirt controller type => vbox bus type */
> + switch (controller->type) {
If you typecast controller->type as (virDomainControllerType)
> + 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;
> +
> + break;
Then here you have cases for when @controller is VIRTIO_SERIAL, CCID,
USB, PCI, or LAST. I would think for types other than LAST, you could
just return because they're not storage controllers (avoids having a
NULL controllerName later on). For LAST one would hope it's not
provided, but it's your call whether that's an error or ignore type case.
Doing this avoids the chance that someone adds a new controller and
doesn't address the vbox code.
> + }
> +
> + /* libvirt scsi model => vbox scsi model */
> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI) {
> + switch (controller->model) {
similarly here, except use (virDomainControllerModelSCSI)
> + case VIR_DOMAIN_CONTROLLER_MODEL_SCSI_LSILOGIC:
> + 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;
> + }
and here would be the I don't know/support that SCSI controller model
type logic or we don't need to set the vboxModel value.
As for LAST, that's probably an error - shouldn't happen, but the
compiler will complain if you don't add it to the switch() case's.
Again, doing this ensures someone doesn't add a new SCSI model type and
doesn't address the vbox code.
> + }
> +
> + /* libvirt ide model => vbox ide model */
> + if (controller->type == VIR_DOMAIN_CONTROLLER_TYPE_IDE) {
I'd use
} else if {
But this is OK
> + switch (controller->model) {
typecast to (virDomainControllerModelIDE)
> + 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;
> + }
Again logic for _LAST and again, this ensures that no one adds to the
enum without addressing logic here.
> + }
> +
> + rc = gVBoxAPI.UIMachine.AddStorageController(machine, controllerName,
> + vboxBusType, &vboxController);
BTW: Without adding those switches above this fails perhaps randomly? I
don't know what happens when one makes this call with a NULL
controllerName and/or a vboxBusType == StorageBus_Null
> +
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to add storage controller "
> + "(type: %s, index=%d), rc=%08x"),
> + virDomainControllerTypeToString(controller->type),
> + controller->idx, (unsigned) rc);
> + goto cleanup;
> + }
> +
> + if (vboxModel != StorageControllerType_Null) {
So this is only necessary for SCSI and IDE of certain types? Not a
problem, mostly curious.
> + rc = gVBoxAPI.UIStorageController.SetControllerType(vboxController,
> + vboxModel);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Failed to change storage controller model, "
> + "rc=%08x"), (unsigned) rc);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> + cleanup:
> + VBOX_UTF16_FREE(controllerName);
> + VBOX_RELEASE(vboxController);
> +
> + return ret;
> +}
> +
> +static int
> +vboxAttachStorageControllers(virDomainDefPtr def, vboxDriverPtr data,
> + IMachine *machine)
> +{
> + size_t i;
> + for (i = 0; i < def->ncontrollers; i++) {
You do realize the domain controller list has more than just storage
controllers... But I address that earlier by returning 0 if nothing
needs to be done... Unless you think something should be done for other c
> + if (vboxSetStorageController(def->controllers[i], data, machine) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
> +
FWIW: More recently in libvirt we prefer new functions added with 2
blank lines between each.
> static virDrvOpenStatus
> vboxConnectOpen(virConnectPtr conn,
> virConnectAuthPtr auth ATTRIBUTE_UNUSED,
> @@ -1017,209 +1080,168 @@ vboxSetBootDeviceOrder(virDomainDefPtr def, vboxDriverPtr data,
> }
> }
>
> -static void
> +static int
> vboxAttachDrives(virDomainDefPtr def, vboxDriverPtr data, IMachine *machine)
> {
> + int ret = 0;
> size_t i;
> nsresult rc = 0;
> - PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> - PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> PRUnichar *storageCtlName = NULL;
> - bool error = false;
> + virDomainDiskDefPtr disk = NULL;
>
> - /* get the max port/slots/etc for the given storage bus */
> - error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst,
> - maxSlotPerPort);
> -
> - /* 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 && !error; i++) {
> - const char *src = virDomainDiskGetSource(def->disks[i]);
> - int type = virDomainDiskGetType(def->disks[i]);
> - int format = virDomainDiskGetFormat(def->disks[i]);
> -
> - 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) src: %s", i, src);
> - VIR_DEBUG("disk(%zu) dst: %s", i, def->disks[i]->dst);
> - VIR_DEBUG("disk(%zu) driverName: %s", i,
> - virDomainDiskGetDriver(def->disks[i]));
> - 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
> - ? "True" : "False"));
> - VIR_DEBUG("disk(%zu) shared: %s", i, (def->disks[i]->src->shared
> - ? "True" : "False"));
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,
> - mediumFileUtf16,
> - 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);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
