[libvirt] [PATCH 5/6] vbox: Process controller definitions from xml.
Dawid Zamirski
dzamirski at datto.com
Thu Oct 19 18:50:30 UTC 2017
On Tue, 2017-10-17 at 15:46 -0400, John Ferlan wrote:
>
> On 10/09/2017 04:49 PM, Dawid Zamirski wrote:
> > From: Dawid Zamirski <dzamirski at datto.com>
> >
> 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...
I've removed those because they basically print out the values of the
XML I just passed in and I know them from XML already. I've added a
more useful (at least for me) debug statement in (WIP) V2 that prints
the arguments passed to VBOX's AttachDevice method - those are
'computed' based on XML input by this very code so it's more helpful to
know what those final values are when debugging.
>
> > -
> > - 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);
> >
More information about the libvir-list
mailing list