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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> > 
> 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);
> > 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux