Another module where Coverity was less than enthusiastic about the
changes - although less issues than the snapshot code...
This one's a bit easier - add a VIR_FREE(disk); to the two places shown
below and you're good.
John
On 05/19/2014 08:47 AM, Yohan BELLEGUIC wrote:
> All snapshots information will be deleted from the vbox XML, but
> differencing disks will be kept so the user will be able to redefine the
> snapshot.
> ---
> src/vbox/vbox_tmpl.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 463 insertions(+), 1 deletion(-)
>
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index a7f15d4..eb577f4 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -8364,7 +8364,454 @@ vboxDomainSnapshotDeleteTree(vboxGlobalData *data,
> return ret;
> }
>
> +#if VBOX_API_VERSION >= 4002000
> +static int
> +vboxDomainSnapshotDeleteMetadataOnly(virDomainSnapshotPtr snapshot)
> +{
> + /*
> + * This function will remove the node in the vbox xml corresponding to the snapshot.
> + * It is usually called by vboxDomainSnapshotDelete() with the flag
> + * VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY.
> + * If you want to use it anywhere else, be careful, if the snapshot you want to delete
> + * has children, the result is not granted, they will probably will be deleted in the
> + * xml, but you may have a problem with hard drives.
> + *
> + * If the snapshot which is being deleted is the current one, we will set the current
> + * snapshot of the machine to the parent of this snapshot. Before writing the modified
> + * xml file, we undefine the machine from vbox. After writing the file, we redefine
> + * the machine with the new file.
> + */
> +
> + virDomainPtr dom = snapshot->domain;
> + VBOX_OBJECT_CHECK(dom->conn, int, -1);
> + virDomainSnapshotDefPtr def= NULL;
> + char *defXml = NULL;
> + vboxIID domiid = VBOX_IID_INITIALIZER;
> + nsresult rc;
> + IMachine *machine = NULL;
> + PRUnichar *settingsFilePathUtf16 = NULL;
> + char *settingsFilepath = NULL;
> + virVBoxSnapshotConfMachinePtr snapshotMachineDesc = NULL;
> + int isCurrent = -1;
> + int it = 0;
> + PRUnichar *machineNameUtf16 = NULL;
> + char *machineName = NULL;
> + char *nameTmpUse = NULL;
> + char *machineLocationPath = NULL;
> + PRUint32 aMediaSize = 0;
> + IMedium **aMedia = NULL;
>
> + defXml = vboxDomainSnapshotGetXMLDesc(snapshot, 0);
> + if (!defXml) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to get XML Desc of snapshot"));
> + goto cleanup;
> + }
> + def = virDomainSnapshotDefParseString(defXml,
> + data->caps,
> + data->xmlopt,
> + -1,
> + VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
> + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE);
> + if (!def) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to get a virDomainSnapshotDefPtr"));
> + goto cleanup;
> + }
> +
> + vboxIIDFromUUID(&domiid, dom->uuid);
> + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_NO_DOMAIN, "%s",
> + _("no domain with matching UUID"));
> + goto cleanup;
> + }
> + rc = machine->vtbl->GetSettingsFilePath(machine, &settingsFilePathUtf16);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get settings file path"));
> + goto cleanup;
> + }
> + VBOX_UTF16_TO_UTF8(settingsFilePathUtf16, &settingsFilepath);
> +
> + /*Getting the machine name to retrieve the machine location path.*/
> + rc = machine->vtbl->GetName(machine, &machineNameUtf16);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get machine name"));
> + goto cleanup;
> + }
> + VBOX_UTF16_TO_UTF8(machineNameUtf16, &machineName);
> + if (virAsprintf(&nameTmpUse, "%s.vbox", machineName) < 0)
> + goto cleanup;
> + machineLocationPath = virStringReplace(settingsFilepath, nameTmpUse, "");
> + if (machineLocationPath == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to get the machine location path"));
> + goto cleanup;
> + }
> + snapshotMachineDesc = virVBoxSnapshotConfLoadVboxFile(settingsFilepath, machineLocationPath);
> + if (!snapshotMachineDesc) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot create a vboxSnapshotXmlPtr"));
> + goto cleanup;
> + }
> +
> + isCurrent = virVBoxSnapshotConfIsCurrentSnapshot(snapshotMachineDesc, def->name);
> + if (isCurrent < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to know if the snapshot is the current snapshot"));
> + goto cleanup;
> + }
> + if (isCurrent) {
> + /*
> + * If the snapshot is the current snapshot, it means that the machine has read-write
> + * disks. The first thing to do is to manipulate VirtualBox API to create
> + * differential read-write disks if the parent snapshot is not null.
> + */
> + if (def->parent != NULL) {
> + for (it = 0; it < def->dom->ndisks; it++) {
> + virVBoxSnapshotConfHardDiskPtr readOnly = NULL;
> + IMedium *medium = NULL;
> + PRUnichar *locationUtf16 = NULL;
> + PRUnichar *parentUuidUtf16 = NULL;
> + char *parentUuid = NULL;
> + IMedium *newMedium = NULL;
> + PRUnichar *formatUtf16 = NULL;
> + PRUnichar *newLocation = NULL;
> + char *newLocationUtf8 = NULL;
> + IProgress *progress = NULL;
> + PRInt32 resultCode = -1;
> + virVBoxSnapshotConfHardDiskPtr disk = NULL;
> + PRUnichar *uuidUtf16 = NULL;
> + char *uuid = NULL;
> + char *format = NULL;
> + char **searchResultTab = NULL;
> + ssize_t resultSize = 0;
> + char *tmp = NULL;
> +
> + readOnly = virVBoxSnapshotConfHardDiskPtrByLocation(snapshotMachineDesc,
> + def->dom->disks[it]->src.path);
> + if (!readOnly) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Cannot get hard disk by location"));
> + goto cleanup;
> + }
> + if (readOnly->parent == NULL) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("The read only disk has no parent"));
> + goto cleanup;
> + }
> +
> + VBOX_UTF8_TO_UTF16(readOnly->parent->location, &locationUtf16);
> + rc = data->vboxObj->vtbl->OpenMedium(data->vboxObj,
> + locationUtf16,
> + DeviceType_HardDisk,
> + AccessMode_ReadWrite,
> + false,
> + &medium);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to open HardDisk, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> +
> + rc = medium->vtbl->GetId(medium, &parentUuidUtf16);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to get hardDisk Id, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> + VBOX_UTF16_TO_UTF8(parentUuidUtf16, &parentUuid);
> + VBOX_UTF16_FREE(parentUuidUtf16);
> + VBOX_UTF16_FREE(locationUtf16);
> + VBOX_UTF8_TO_UTF16("VDI", &formatUtf16);
> +
> + if (virAsprintf(&newLocationUtf8, "%sfakedisk-%s-%d.vdi",
> + machineLocationPath, def->parent, it) < 0)
> + goto cleanup;
> + VBOX_UTF8_TO_UTF16(newLocationUtf8, &newLocation);
> + rc = data->vboxObj->vtbl->CreateHardDisk(data->vboxObj,
> + formatUtf16,
> + newLocation,
> + &newMedium);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to create HardDisk, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> + VBOX_UTF16_FREE(formatUtf16);
> + VBOX_UTF16_FREE(newLocation);
> +
> +# if VBOX_API_VERSION < 4003000
> + medium->vtbl->CreateDiffStorage(medium, newMedium, MediumVariant_Diff, &progress);
> +# else
> + PRUint32 tab[1];
> + tab[0] = MediumVariant_Diff;
> + medium->vtbl->CreateDiffStorage(medium, newMedium, 1, tab, &progress);
> +# endif
> +
> + progress->vtbl->WaitForCompletion(progress, -1);
> + progress->vtbl->GetResultCode(progress, &resultCode);
> + if (NS_FAILED(resultCode)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Error while creating diff storage, rc=%08x"),
> + (unsigned)resultCode);
> + goto cleanup;
> + }
> + VBOX_RELEASE(progress);
> + /*
> + * The differential disk is created, we add it to the media registry and
> + * the machine storage controller.
> + */
> +
> + if (VIR_ALLOC(disk) < 0)
> + goto cleanup;
Coverity complains:
(43) Event alloc_arg: "virAlloc" allocates memory that is stored into
"disk". [details]
> +
> + rc = newMedium->vtbl->GetId(newMedium, &uuidUtf16);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to get medium uuid, rc=%08x"),
> + (unsigned)rc);
(47) Event goto: Jumping to label "cleanup"
(48) Event leaked_storage: Variable "disk" going out of scope leaks the
storage it points to.
> + goto cleanup;
> + }
> + VBOX_UTF16_TO_UTF8(uuidUtf16, &uuid);
> + disk->uuid = uuid;
> + VBOX_UTF16_FREE(uuidUtf16);
> +
> + if (VIR_STRDUP(disk->location, newLocationUtf8) < 0)
(50) Event goto: Jumping to label "cleanup"
(51) Event leaked_storage: Variable "disk" going out of scope leaks the
storage it points to.
> + goto cleanup;
> +
> + rc = newMedium->vtbl->GetFormat(newMedium, &formatUtf16);
> + VBOX_UTF16_TO_UTF8(formatUtf16, &format);
> + disk->format = format;
> + VBOX_UTF16_FREE(formatUtf16);
> +
> + if (virVBoxSnapshotConfAddHardDiskToMediaRegistry(disk,
> + snapshotMachineDesc->mediaRegistry,
> + parentUuid) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to add hard disk to the media registry"));
> + goto cleanup;
> + }
> + /*Adding fake disks to the machine storage controllers*/
> +
> + resultSize = virStringSearch(snapshotMachineDesc->storageController,
> + VBOX_UUID_REGEX,
> + it + 1,
> + &searchResultTab);
> + if (resultSize != it + 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find UUID %s"), searchResultTab[it]);
> + goto cleanup;
> + }
> +
> + tmp = virStringReplace(snapshotMachineDesc->storageController,
> + searchResultTab[it],
> + disk->uuid);
> + virStringFreeList(searchResultTab);
> + VIR_FREE(snapshotMachineDesc->storageController);
> + if (!tmp)
> + goto cleanup;
> + if (VIR_STRDUP(snapshotMachineDesc->storageController, tmp) < 0)
> + goto cleanup;
> +
> + VIR_FREE(tmp);
> + /*Closing the "fake" disk*/
> + rc = newMedium->vtbl->Close(newMedium);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to close the new medium, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> + }
> + } else {
> + for (it = 0; it < def->dom->ndisks; it++) {
> + char *uuidRO = NULL;
> + char **searchResultTab = NULL;
> + ssize_t resultSize = 0;
> + char *tmp = NULL;
> + uuidRO = virVBoxSnapshotConfHardDiskUuidByLocation(snapshotMachineDesc,
> + def->dom->disks[it]->src.path);
> + if (!uuidRO) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("No such disk in media registry %s"),
> + def->dom->disks[it]->src.path);
> + goto cleanup;
> + }
> +
> + resultSize = virStringSearch(snapshotMachineDesc->storageController,
> + VBOX_UUID_REGEX,
> + it + 1,
> + &searchResultTab);
> + if (resultSize != it + 1) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find UUID %s"),
> + searchResultTab[it]);
> + goto cleanup;
> + }
> +
> + tmp = virStringReplace(snapshotMachineDesc->storageController,
> + searchResultTab[it],
> + uuidRO);
> + virStringFreeList(searchResultTab);
> + VIR_FREE(snapshotMachineDesc->storageController);
> + if (!tmp)
> + goto cleanup;
> + if (VIR_STRDUP(snapshotMachineDesc->storageController, tmp) < 0)
> + goto cleanup;
> +
> + VIR_FREE(tmp);
> + }
> + }
> + }
> + /*We remove the read write disks from the media registry*/
> + for (it = 0; it < def->ndisks; it++) {
> + char *uuidRW = virVBoxSnapshotConfHardDiskUuidByLocation(snapshotMachineDesc, def->disks[it].src.path);
> + if (!uuidRW) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find UUID for location %s"), def->disks[it].src.path);
> + goto cleanup;
> + }
> + if (virVBoxSnapshotConfRemoveHardDisk(snapshotMachineDesc->mediaRegistry, uuidRW) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to remove disk from media registry. uuid = %s"), uuidRW);
> + goto cleanup;
> + }
> + }
> + /*If the parent snapshot is not NULL, we remove the-read only disks from the media registry*/
> + if (def->parent != NULL) {
> + for (it = 0; it < def->dom->ndisks; it++) {
> + char *uuidRO = virVBoxSnapshotConfHardDiskUuidByLocation(snapshotMachineDesc, def->dom->disks[it]->src.path);
> + if (!uuidRO) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to find UUID for location %s"), def->dom->disks[it]->src.path);
> + goto cleanup;
> + }
> + if (virVBoxSnapshotConfRemoveHardDisk(snapshotMachineDesc->mediaRegistry, uuidRO) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to remove disk from media registry. uuid = %s"), uuidRO);
> + goto cleanup;
> + }
> + }
> + }
> + rc = machine->vtbl->Unregister(machine,
> + CleanupMode_DetachAllReturnHardDisksOnly,
> + &aMediaSize,
> + &aMedia);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to unregister machine, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> + VBOX_RELEASE(machine);
> + for (it = 0; it < aMediaSize; it++) {
> + IMedium *medium = aMedia[it];
> + if (medium) {
> + PRUnichar *locationUtf16 = NULL;
> + char *locationUtf8 = NULL;
> + rc = medium->vtbl->GetLocation(medium, &locationUtf16);
> + VBOX_UTF16_TO_UTF8(locationUtf16, &locationUtf8);
> + if (isCurrent && strstr(locationUtf8, "fake") != NULL) {
> + /*we delete the fake disk because we don't need it anymore*/
> + IProgress *progress = NULL;
> + PRInt32 resultCode = -1;
> + rc = medium->vtbl->DeleteStorage(medium, &progress);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to delete medium, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> + progress->vtbl->WaitForCompletion(progress, -1);
> + progress->vtbl->GetResultCode(progress, &resultCode);
> + if (NS_FAILED(resultCode)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Error while closing medium, rc=%08x"),
> + (unsigned)resultCode);
> + goto cleanup;
> + }
> + VBOX_RELEASE(progress);
> + } else {
> + /* This a comment from vboxmanage code in the handleUnregisterVM
> + * function in VBoxManageMisc.cpp :
> + * Note that the IMachine::Unregister method will return the medium
> + * reference in a sane order, which means that closing will normally
> + * succeed, unless there is still another machine which uses the
> + * medium. No harm done if we ignore the error. */
> + rc = medium->vtbl->Close(medium);
> + }
> + VBOX_UTF16_FREE(locationUtf16);
> + VBOX_UTF8_FREE(locationUtf8);
> + }
> + }
> +
> + /*removing the snapshot*/
> + if (virVBoxSnapshotConfRemoveSnapshot(snapshotMachineDesc, def->name) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to remove snapshot %s"), def->name);
> + goto cleanup;
> + }
> +
> + if (isCurrent) {
> + VIR_FREE(snapshotMachineDesc->currentSnapshot);
> + if (def->parent != NULL) {
> + virVBoxSnapshotConfSnapshotPtr snap = virVBoxSnapshotConfSnapshotByName(snapshotMachineDesc->snapshot, def->parent);
> + if (!snap) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to get the snapshot to remove"));
> + goto cleanup;
> + }
> + if (VIR_STRDUP(snapshotMachineDesc->currentSnapshot, snap->uuid) < 0)
> + goto cleanup;
> + }
> + }
> +
> + /*Registering the machine*/
> + if (virVBoxSnapshotConfSaveVboxFile(snapshotMachineDesc, settingsFilepath) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("Unable to serialize the machine description"));
> + goto cleanup;
> + }
> + rc = data->vboxObj->vtbl->OpenMachine(data->vboxObj,
> + settingsFilePathUtf16,
> + &machine);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to open Machine, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> +
> + rc = data->vboxObj->vtbl->RegisterMachine(data->vboxObj, machine);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to register Machine, rc=%08x"),
> + (unsigned)rc);
> + goto cleanup;
> + }
> +
> + ret = 0;
> + cleanup:
> + VIR_FREE(def);
> + VIR_FREE(defXml);
> + VBOX_RELEASE(machine);
> + VBOX_UTF16_FREE(settingsFilePathUtf16);
> + VBOX_UTF8_FREE(settingsFilepath);
> + VIR_FREE(snapshotMachineDesc);
> + VBOX_UTF16_FREE(machineNameUtf16);
> + VBOX_UTF8_FREE(machineName);
> + VIR_FREE(machineLocationPath);
> + VIR_FREE(nameTmpUse);
> +
> + return ret;
> +}
> +#endif
>
> static int
> vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> @@ -8378,6 +8825,7 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> IConsole *console = NULL;
> PRUint32 state;
> nsresult rc;
> + vboxArray snapChildren = VBOX_ARRAY_INITIALIZER;
>
> virCheckFlags(VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN |
> VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY, -1);
> @@ -8405,7 +8853,21 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> *to remove the node concerning the snapshot
> */
> if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) {
> - ret = 0;
> + rc = vboxArrayGet(&snapChildren, snap, snap->vtbl->GetChildren);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("could not get snapshot children"));
> + goto cleanup;
> + }
> + if (snapChildren.count != 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot delete metadata of a snapshot with children"));
> + goto cleanup;
> + } else {
> +#if VBOX_API_VERSION >= 4002000
> + ret = vboxDomainSnapshotDeleteMetadataOnly(snapshot);
> +#endif
> + }
> goto cleanup;
> }
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list