On 10/23/2014 09:46 PM, Taowei Luo wrote: > The API on IHardDiskAttachment is merged into IMediumAttachment. > So, we don't need it anymore. > --- > src/vbox/vbox_storage.c | 160 ++++++++++++++++++++++++++++++++ > src/vbox/vbox_tmpl.c | 202 > ++++++----------------------------------- > src/vbox/vbox_uniformed_api.h | 6 ++ > 3 files changed, 192 insertions(+), 176 deletions(-) >
The Coverity checker had some issues with this patch. Coverity has a single UNUSED_VALUE for 5 different checks. It wasn't initially clear what the problem was, but I was able to at least determine that although I'm not quite sure of the "correct" fix/patch. > diff --git a/src/vbox/vbox_storage.c b/src/vbox/vbox_storage.c > index 1e6ca67..45090eb 100644 > --- a/src/vbox/vbox_storage.c > +++ b/src/vbox/vbox_storage.c > @@ -530,3 +530,163 @@ virStorageVolPtr > vboxStorageVolCreateXML(virStoragePoolPtr pool, > virStorageVolDefFree(def); > return ret; > } > + > +int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int flags) > +{ > + vboxGlobalData *data = vol->conn->privateData; > + unsigned char uuid[VIR_UUID_BUFLEN]; > + IHardDisk *hardDisk = NULL; > + int deregister = 0; > + PRUint32 hddstate = 0; > + size_t i = 0; > + size_t j = 0; > + PRUint32 machineIdsSize = 0; > + vboxArray machineIds = VBOX_ARRAY_INITIALIZER; > + vboxIIDUnion hddIID; > + nsresult rc; > + int ret = -1; > + > + if (!data->vboxObj) { > + return ret; > + } > + > + VBOX_IID_INITIALIZE(&hddIID); > + virCheckFlags(0, -1); > + > + if (virUUIDParse(vol->key, uuid) < 0) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("Could not parse UUID from '%s'"), vol->key); > + return -1; > + } > + > + vboxIIDFromUUID(&hddIID, uuid); > + rc = gVBoxAPI.UIVirtualBox.GetHardDiskByIID(data->vboxObj, &hddIID, > &hardDisk); > + if (NS_FAILED(rc)) > + goto cleanup; > + > + gVBoxAPI.UIMedium.GetState(hardDisk, &hddstate); > + if (hddstate == MediaState_Inaccessible) > + goto cleanup; > + > + gVBoxAPI.UArray.vboxArrayGet(&machineIds, hardDisk, > + > gVBoxAPI.UArray.handleMediumGetMachineIds(hardDisk)); > + > +#if defined WIN32 > + /* VirtualBox 2.2 on Windows represents IIDs as GUIDs and the > + * machineIds array contains direct instances of the GUID struct > + * instead of pointers to the actual struct instances. But there > + * is no 128bit width simple item type for a SafeArray to fit a > + * GUID in. The largest simple type it 64bit width and VirtualBox > + * uses two of this 64bit items to represents one GUID. Therefore, > + * we divide the size of the SafeArray by two, to compensate for > + * this workaround in VirtualBox */ > + if (gVBoxAPI.uVersion >= 2001052 && gVBoxAPI.uVersion < 2002051) > + machineIds.count /= 2; > +#endif /* !defined WIN32 */ > + > + machineIdsSize = machineIds.count; > + > + for (i = 0; i < machineIds.count; i++) { > + IMachine *machine = NULL; > + vboxIIDUnion machineId; > + vboxArray hddAttachments = VBOX_ARRAY_INITIALIZER; > + > + VBOX_IID_INITIALIZE(&machineId); > + vboxIIDFromArrayItem(&machineId, &machineIds, i); > + > + if (gVBoxAPI.getMachineForSession) { > + rc = gVBoxAPI.UIVirtualBox.GetMachine(data->vboxObj, &machineId, > &machine); Event value_overwrite: Value from "(*gVBoxAPI.UIMachine.SaveSettings)(machine)" is overwritten with value from "(*gVBoxAPI.UIVirtualBox.GetMachine)(data->vboxObj, &machineId, &machine)". > + if (NS_FAILED(rc)) { > + virReportError(VIR_ERR_NO_DOMAIN, "%s", > + _("no domain with matching uuid")); > + break; > + } > + } > + > + rc = gVBoxAPI.UISession.Open(data, &machineId, machine); Event value_overwrite: Value from "(*gVBoxAPI.UIMachine.SaveSettings)(machine)" is overwritten with value from "(*gVBoxAPI.UISession.Open)(data, &machineId, machine)". > + > + if (NS_FAILED(rc)) { > + vboxIIDUnalloc(&machineId); > + continue; > + } > + > + rc = gVBoxAPI.UISession.GetMachine(data->vboxSession, &machine); > + if (NS_FAILED(rc)) > + goto cleanupLoop; > + > + gVBoxAPI.UArray.vboxArrayGet(&hddAttachments, machine, > + > gVBoxAPI.UArray.handleMachineGetMediumAttachments(machine)); > + > + for (j = 0; j < hddAttachments.count; j++) { > + IMediumAttachment *hddAttachment = hddAttachments.items[j]; > + IHardDisk *hdd = NULL; > + vboxIIDUnion iid; > + > + if (!hddAttachment) > + continue; > + > + rc = gVBoxAPI.UIMediumAttachment.GetMedium(hddAttachment, &hdd); Event value_overwrite: Value from "(*gVBoxAPI.UIMachine.SaveSettings)(machine)" is overwritten with value from "(*gVBoxAPI.UIMediumAttachment.GetMedium)(hddAttachment, &hdd)". > + if (NS_FAILED(rc) || !hdd) > + continue; > + > + VBOX_IID_INITIALIZE(&iid); > + rc = gVBoxAPI.UIMedium.GetId(hdd, &iid); > + if (NS_FAILED(rc)) { > + VBOX_MEDIUM_RELEASE(hdd); > + continue; > + } > + > + DEBUGIID("HardDisk (to delete) UUID", &hddIID); > + DEBUGIID("HardDisk (currently processing) UUID", &iid); > + > + if (vboxIIDIsEqual(&hddIID, &iid)) { > + PRUnichar *controller = NULL; > + PRInt32 port = 0; > + PRInt32 device = 0; > + > + DEBUGIID("Found HardDisk to delete, UUID", &hddIID); > + > + gVBoxAPI.UIMediumAttachment.GetController(hddAttachment, > &controller); > + gVBoxAPI.UIMediumAttachment.GetPort(hddAttachment, &port); > + gVBoxAPI.UIMediumAttachment.GetDevice(hddAttachment, > &device); > + > + rc = gVBoxAPI.UIMachine.DetachDevice(machine, controller, > port, device); > + if (NS_SUCCEEDED(rc)) { > + rc = gVBoxAPI.UIMachine.SaveSettings(machine); Event returned_value: Value from "(*gVBoxAPI.UIMachine.SaveSettings)(machine)" is assigned to "rc" here, but that stored value is not used before it is overwritten. OK - so this seems to be "root cause" - the 'rc' isn't being checked here; however, it is checked and handled in other cases. This causes Coverity to complain that we need to "handle" it here too. If I add a simple check "if (NS_FAILED(rc) {...}", then Coverity is happy. I'm not sure what to expect to be done here, so I leave that decision up to you for a follow up patch - thanks! > + VIR_DEBUG("saving machine settings"); > + deregister++; > + VIR_DEBUG("deregistering hdd:%d", deregister); > + } > + > + VBOX_UTF16_FREE(controller); > + } > + vboxIIDUnalloc(&iid); > + VBOX_MEDIUM_RELEASE(hdd); > + } > + > + cleanupLoop: > + gVBoxAPI.UArray.vboxArrayRelease(&hddAttachments); > + VBOX_RELEASE(machine); > + gVBoxAPI.UISession.Close(data->vboxSession); > + vboxIIDUnalloc(&machineId); > + } > + > + gVBoxAPI.UArray.vboxArrayUnalloc(&machineIds); > + > + if (machineIdsSize == 0 || machineIdsSize == deregister) { > + IProgress *progress = NULL; > + rc = gVBoxAPI.UIHardDisk.DeleteStorage(hardDisk, &progress); Event value_overwrite: Value from "(*gVBoxAPI.UIMachine.SaveSettings)(machine)" is overwritten with value from "(*gVBoxAPI.UIHardDisk.DeleteStorage)(hardDisk, &progress)". > + > + if (NS_SUCCEEDED(rc) && progress) { > + gVBoxAPI.UIProgress.WaitForCompletion(progress, -1); > + VBOX_RELEASE(progress); > + DEBUGIID("HardDisk deleted, UUID", &hddIID); > + ret = 0; > + } > + } > + > + cleanup: > + VBOX_MEDIUM_RELEASE(hardDisk); > + vboxIIDUnalloc(&hddIID); > + return ret; > +} > diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c > index 8aed0d6..71dbedd 100644 > --- a/src/vbox/vbox_tmpl.c > +++ b/src/vbox/vbox_tmpl.c > @@ -106,7 +106,6 @@ typedef IUSBDeviceFilters IUSBCommon; > typedef IHardDiskAttachment IMediumAttachment; > #else /* VBOX_API_VERSION >= 3001000 */ > typedef IMedium IHardDisk; > -typedef IMediumAttachment IHardDiskAttachment; > #endif /* VBOX_API_VERSION >= 3001000 */ > > #include "vbox_uniformed_api.h" > @@ -2034,181 +2033,6 @@ _registerDomainEvent(virHypervisorDriverPtr driver) > * The Storage Functions here on > */ > > -static int vboxStorageVolDelete(virStorageVolPtr vol, > - unsigned int flags) > -{ > - VBOX_OBJECT_CHECK(vol->conn, int, -1); > - vboxIID hddIID = VBOX_IID_INITIALIZER; > - unsigned char uuid[VIR_UUID_BUFLEN]; > - IHardDisk *hardDisk = NULL; > - int deregister = 0; > - nsresult rc; > - size_t i = 0; > - size_t j = 0; > - > - virCheckFlags(0, -1); > - > - if (virUUIDParse(vol->key, uuid) < 0) { > - virReportError(VIR_ERR_INVALID_ARG, > - _("Could not parse UUID from '%s'"), vol->key); > - return -1; > - } > - > - vboxIIDFromUUID(&hddIID, uuid); > -#if VBOX_API_VERSION < 4000000 > - rc = data->vboxObj->vtbl->GetHardDisk(data->vboxObj, hddIID.value, > &hardDisk); > -#elif VBOX_API_VERSION >= 4000000 && VBOX_API_VERSION < 4002000 > - rc = data->vboxObj->vtbl->FindMedium(data->vboxObj, hddIID.value, > - DeviceType_HardDisk, &hardDisk); > -#else > - rc = data->vboxObj->vtbl->OpenMedium(data->vboxObj, hddIID.value, > - DeviceType_HardDisk, > AccessMode_ReadWrite, > - PR_FALSE, &hardDisk); > -#endif /* VBOX_API_VERSION >= 4000000 */ > - if (NS_SUCCEEDED(rc)) { > - PRUint32 hddstate; > - > - VBOX_MEDIUM_FUNC_ARG1(hardDisk, GetState, &hddstate); > - if (hddstate != MediaState_Inaccessible) { > - PRUint32 machineIdsSize = 0; > - vboxArray machineIds = VBOX_ARRAY_INITIALIZER; > - > -#if VBOX_API_VERSION < 3001000 > - vboxArrayGet(&machineIds, hardDisk, > hardDisk->vtbl->imedium.GetMachineIds); > -#else /* VBOX_API_VERSION >= 3001000 */ > - vboxArrayGet(&machineIds, hardDisk, > hardDisk->vtbl->GetMachineIds); > -#endif /* VBOX_API_VERSION >= 3001000 */ > - > -#if VBOX_API_VERSION == 2002000 && defined WIN32 > - /* VirtualBox 2.2 on Windows represents IIDs as GUIDs and the > - * machineIds array contains direct instances of the GUID struct > - * instead of pointers to the actual struct instances. But there > - * is no 128bit width simple item type for a SafeArray to fit a > - * GUID in. The largest simple type it 64bit width and VirtualBox > - * uses two of this 64bit items to represents one GUID. > Therefore, > - * we divide the size of the SafeArray by two, to compensate for > - * this workaround in VirtualBox */ > - machineIds.count /= 2; > -#endif /* VBOX_API_VERSION >= 2002000 */ > - > - machineIdsSize = machineIds.count; > - > - for (i = 0; i < machineIds.count; i++) { > - IMachine *machine = NULL; > - vboxIID machineId = VBOX_IID_INITIALIZER; > - > - vboxIIDFromArrayItem(&machineId, &machineIds, i); > - > -#if VBOX_API_VERSION >= 4000000 > - rc = VBOX_OBJECT_GET_MACHINE(machineId.value, &machine); > - if (NS_FAILED(rc)) { > - virReportError(VIR_ERR_NO_DOMAIN, "%s", > - _("no domain with matching uuid")); > - break; > - } > -#endif > - > - rc = VBOX_SESSION_OPEN(machineId.value, machine); > - > - if (NS_SUCCEEDED(rc)) { > - > - rc = > data->vboxSession->vtbl->GetMachine(data->vboxSession, &machine); > - if (NS_SUCCEEDED(rc)) { > - vboxArray hddAttachments = VBOX_ARRAY_INITIALIZER; > - > -#if VBOX_API_VERSION < 3001000 > - vboxArrayGet(&hddAttachments, machine, > - machine->vtbl->GetHardDiskAttachments); > -#else /* VBOX_API_VERSION >= 3001000 */ > - vboxArrayGet(&hddAttachments, machine, > - machine->vtbl->GetMediumAttachments); > -#endif /* VBOX_API_VERSION >= 3001000 */ > - for (j = 0; j < hddAttachments.count; j++) { > - IHardDiskAttachment *hddAttachment = > hddAttachments.items[j]; > - > - if (hddAttachment) { > - IHardDisk *hdd = NULL; > - > -#if VBOX_API_VERSION < 3001000 > - rc = > hddAttachment->vtbl->GetHardDisk(hddAttachment, &hdd); > -#else /* VBOX_API_VERSION >= 3001000 */ > - rc = > hddAttachment->vtbl->GetMedium(hddAttachment, &hdd); > -#endif /* VBOX_API_VERSION >= 3001000 */ > - if (NS_SUCCEEDED(rc) && hdd) { > - vboxIID iid = VBOX_IID_INITIALIZER; > - > - rc = VBOX_MEDIUM_FUNC_ARG1(hdd, GetId, > &iid.value); > - if (NS_SUCCEEDED(rc)) { > - > - DEBUGIID("HardDisk (to delete) > UUID", hddIID.value); > - DEBUGIID("HardDisk (currently > processing) UUID", iid.value); > - > - if (vboxIIDIsEqual(&hddIID, &iid)) { > - PRUnichar *controller = NULL; > - PRInt32 port = 0; > - PRInt32 device = 0; > - > - DEBUGIID("Found HardDisk to > delete, UUID", hddIID.value); > - > - > hddAttachment->vtbl->GetController(hddAttachment, &controller); > - > hddAttachment->vtbl->GetPort(hddAttachment, &port); > - > hddAttachment->vtbl->GetDevice(hddAttachment, &device); > - > -#if VBOX_API_VERSION < 3001000 > - rc = > machine->vtbl->DetachHardDisk(machine, controller, port, device); > -#else /* VBOX_API_VERSION >= 3001000 */ > - rc = > machine->vtbl->DetachDevice(machine, controller, port, device); > -#endif /* VBOX_API_VERSION >= 3001000 */ > - if (NS_SUCCEEDED(rc)) { > - rc = > machine->vtbl->SaveSettings(machine); > - VIR_DEBUG("saving machine > settings"); > - } > - > - if (NS_SUCCEEDED(rc)) { > - deregister++; > - VIR_DEBUG("deregistering > hdd:%d", deregister); > - } > - > - VBOX_UTF16_FREE(controller); > - } > - vboxIIDUnalloc(&iid); > - } > - VBOX_MEDIUM_RELEASE(hdd); > - } > - } > - } > - vboxArrayRelease(&hddAttachments); > - VBOX_RELEASE(machine); > - } > - VBOX_SESSION_CLOSE(); > - } > - > - vboxIIDUnalloc(&machineId); > - } > - > - vboxArrayUnalloc(&machineIds); > - > - if (machineIdsSize == 0 || machineIdsSize == deregister) { > - IProgress *progress = NULL; > - rc = hardDisk->vtbl->DeleteStorage(hardDisk, &progress); > - > - if (NS_SUCCEEDED(rc) && progress) { > - progress->vtbl->WaitForCompletion(progress, -1); > - VBOX_RELEASE(progress); > - DEBUGIID("HardDisk deleted, UUID", hddIID.value); > - ret = 0; > - } > - } > - } > - > - VBOX_MEDIUM_RELEASE(hardDisk); > - } > - > - vboxIIDUnalloc(&hddIID); > - > - return ret; > -} > - > static int > vboxStorageVolGetInfo(virStorageVolPtr vol, virStorageVolInfoPtr info) > { > @@ -3236,6 +3060,11 @@ static void* _handleMediumGetSnapshotIds(IMedium > *medium) > return medium->vtbl->GetSnapshotIds; > } > > +static void* _handleMediumGetMachineIds(IMedium *medium) > +{ > + return medium->vtbl->GetMachineIds; > +} > + > static void* _handleHostGetNetworkInterfaces(IHost *host) > { > return host->vtbl->GetNetworkInterfaces; > @@ -3548,6 +3377,17 @@ _machineFindSnapshot(IMachine *machine, vboxIIDUnion > *iidu, ISnapshot **snapshot > } > > static nsresult > +_machineDetachDevice(IMachine *machine, PRUnichar *name, > + PRInt32 controllerPort, PRInt32 device) > +{ > +#if VBOX_API_VERSION < 3001000 > + return machine->vtbl->DetachHardDisk(machine, name, controllerPort, > device); > +#else /* VBOX_API_VERSION >= 3001000 */ > + return machine->vtbl->DetachDevice(machine, name, controllerPort, > device); > +#endif /* VBOX_API_VERSION >= 3001000 */ > +} > + > +static nsresult > _machineGetAccessible(IMachine *machine, PRBool *isAccessible) > { > return machine->vtbl->GetAccessible(machine, isAccessible); > @@ -5035,6 +4875,12 @@ _hardDiskCreateBaseStorage(IHardDisk *hardDisk, > PRUint64 logicalSize, > #endif > } > > +static nsresult > +_hardDiskDeleteStorage(IHardDisk *hardDisk, IProgress **progress) > +{ > + return hardDisk->vtbl->DeleteStorage(hardDisk, progress); > +} > + > static bool _machineStateOnline(PRUint32 state) > { > return ((state >= MachineState_FirstOnline) && > @@ -5094,6 +4940,7 @@ static vboxUniformedArray _UArray = { > .vboxArrayGet = vboxArrayGet, > .vboxArrayGetWithIIDArg = _vboxArrayGetWithIIDArg, > .vboxArrayRelease = vboxArrayRelease, > + .vboxArrayUnalloc = vboxArrayUnalloc, > .handleGetMachines = _handleGetMachines, > .handleGetHardDisks = _handleGetHardDisks, > .handleUSBGetDeviceFilters = _handleUSBGetDeviceFilters, > @@ -5102,6 +4949,7 @@ static vboxUniformedArray _UArray = { > .handleSnapshotGetChildren = _handleSnapshotGetChildren, > .handleMediumGetChildren = _handleMediumGetChildren, > .handleMediumGetSnapshotIds = _handleMediumGetSnapshotIds, > + .handleMediumGetMachineIds = _handleMediumGetMachineIds, > .handleHostGetNetworkInterfaces = _handleHostGetNetworkInterfaces, > }; > > @@ -5136,6 +4984,7 @@ static vboxUniformedIMachine _UIMachine = { > .LaunchVMProcess = _machineLaunchVMProcess, > .Unregister = _machineUnregister, > .FindSnapshot = _machineFindSnapshot, > + .DetachDevice = _machineDetachDevice, > .GetAccessible = _machineGetAccessible, > .GetState = _machineGetState, > .GetName = _machineGetName, > @@ -5378,6 +5227,7 @@ static vboxUniformedIDHCPServer _UIDHCPServer = { > > static vboxUniformedIHardDisk _UIHardDisk = { > .CreateBaseStorage = _hardDiskCreateBaseStorage, > + .DeleteStorage = _hardDiskDeleteStorage, > }; > > static uniformedMachineStateChecker _machineStateChecker = { > diff --git a/src/vbox/vbox_uniformed_api.h b/src/vbox/vbox_uniformed_api.h > index 337ae9c..75299e3 100644 > --- a/src/vbox/vbox_uniformed_api.h > +++ b/src/vbox/vbox_uniformed_api.h > @@ -168,6 +168,7 @@ typedef struct { > nsresult (*vboxArrayGet)(vboxArray *array, void *self, void *getter); > nsresult (*vboxArrayGetWithIIDArg)(vboxArray *array, void *self, void > *getter, vboxIIDUnion *iidu); > void (*vboxArrayRelease)(vboxArray *array); > + void (*vboxArrayUnalloc)(vboxArray *array); > /* Generate function pointers for vboxArrayGet */ > void* (*handleGetMachines)(IVirtualBox *vboxObj); > void* (*handleGetHardDisks)(IVirtualBox *vboxObj); > @@ -177,6 +178,7 @@ typedef struct { > void* (*handleSnapshotGetChildren)(ISnapshot *snapshot); > void* (*handleMediumGetChildren)(IMedium *medium); > void* (*handleMediumGetSnapshotIds)(IMedium *medium); > + void* (*handleMediumGetMachineIds)(IMedium *medium); > void* (*handleHostGetNetworkInterfaces)(IHost *host); > } vboxUniformedArray; > > @@ -225,6 +227,8 @@ typedef struct { > nsresult (*Unregister)(IMachine *machine, PRUint32 cleanupMode, > PRUint32 *aMediaSize, IMedium ***aMedia); > nsresult (*FindSnapshot)(IMachine *machine, vboxIIDUnion *iidu, > ISnapshot **snapshot); > + nsresult (*DetachDevice)(IMachine *machine, PRUnichar *name, > + PRInt32 controllerPort, PRInt32 device); > nsresult (*GetAccessible)(IMachine *machine, PRBool *isAccessible); > nsresult (*GetState)(IMachine *machine, PRUint32 *state); > nsresult (*GetName)(IMachine *machine, PRUnichar **name); > @@ -523,6 +527,7 @@ typedef struct { > typedef struct { > nsresult (*CreateBaseStorage)(IHardDisk *hardDisk, PRUint64 logicalSize, > PRUint32 variant, IProgress **progress); > + nsresult (*DeleteStorage)(IHardDisk *hardDisk, IProgress **progress); > } vboxUniformedIHardDisk; > > typedef struct { > @@ -613,6 +618,7 @@ virStorageVolPtr vboxStorageVolLookupByKey(virConnectPtr > conn, const char *key); > virStorageVolPtr vboxStorageVolLookupByPath(virConnectPtr conn, const char > *path); > virStorageVolPtr vboxStorageVolCreateXML(virStoragePoolPtr pool, > const char *xml, unsigned int > flags); > +int vboxStorageVolDelete(virStorageVolPtr vol, unsigned int flags); > > /* Version specified functions for installing uniformed API */ > void vbox22InstallUniformedAPI(vboxUniformedAPI *pVBoxAPI); > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list