Re: [PATCH v8 4/4] vbox_tmpl.c: Add function for undefining snapshot

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

 



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




[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]