[libvirt] [PATCHv4 3/5] vbox_tmpl.c: Better XML description for snapshots
Laine Stump
laine at laine.org
Tue Dec 17 11:32:59 UTC 2013
On 11/25/2013 07:23 PM, Manuel VIVES wrote:
> It will be needed for the futur patches because we will
> redefine snapshots
s/futur/future/ :-)
> ---
> src/conf/domain_conf.c | 2 +-
> src/vbox/vbox_tmpl.c | 427 ++++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 417 insertions(+), 12 deletions(-)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 7b0e3ea..ae20eb5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -17054,7 +17054,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> if (virDomainChrDefFormat(buf, &console, flags) < 0)
> goto error;
> }
> - if (STREQ(def->os.type, "hvm") &&
> + if (STREQ_NULLABLE(def->os.type, "hvm") &&
> def->nconsoles == 0 &&
> def->nserials > 0) {
> virDomainChrDef console;
This hunk should be a separate patch. BTW, is it valid for vbox domains
to not have a valid os.type? virDomainDefPostParseInternal() requires it
to be set, and there are several other places in domain_conf.c that
compare it without allowing for NULL, so if NULL is valid, those other
places should at least get scrutiny, or you should make sure that it's
set when you construct the domain object in the vbox driver.
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index 983a595..e05ed97 100644
> --- a/src/vbox/vbox_tmpl.c
> +++ b/src/vbox/vbox_tmpl.c
> @@ -38,6 +38,7 @@
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> +#include <libxml/xmlwriter.h>
>
> #include "internal.h"
> #include "datatypes.h"
> @@ -58,6 +59,8 @@
> #include "fdstream.h"
> #include "viruri.h"
> #include "virstring.h"
> +#include "virtime.h"
> +#include "virutil.h"
>
> /* This one changes from version to version. */
> #if VBOX_API_VERSION == 2002
> @@ -273,10 +276,16 @@ static vboxGlobalData *g_pVBoxGlobalData = NULL;
>
> #endif /* VBOX_API_VERSION >= 4000 */
>
> +#define reportInternalErrorIfNS_FAILED(message) \
> + if (NS_FAILED(rc)) { \
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _(message)); \
> + goto cleanup; \
> + }
> +
> +
I would prefer these if() clauses with goto to be explicitly in the
code, rather than hidden in a macro. That way it's easier to see all
potential code paths when scanning through the code. If you have a goto
hidden in a macro, then it's not immediately visible and that can only
lead to problems.
> static virDomainPtr vboxDomainDefineXML(virConnectPtr conn, const char *xml);
> static int vboxDomainCreate(virDomainPtr dom);
> static int vboxDomainUndefineFlags(virDomainPtr dom, unsigned int flags);
> -
> static void vboxDriverLock(vboxGlobalData *data) {
You should leave in that blank line, as it separates static function
declarations from a function definition.
> virMutexLock(&data->lock);
> }
> @@ -285,6 +294,12 @@ static void vboxDriverUnlock(vboxGlobalData *data) {
> virMutexUnlock(&data->lock);
> }
>
> +typedef enum {
> + VBOX_STORAGE_DELETE_FLAG = 0,
> +#if VBOX_API_VERSION >= 4002
> + VBOX_STORAGE_CLOSE_FLAG = 1,
> +#endif
> +} vboxStorageDeleteOrCloseFlags;
> #if VBOX_API_VERSION == 2002
>
> static void nsIDtoChar(unsigned char *uuid, const nsID *iid) {
> @@ -5957,7 +5972,8 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
> virCheckFlags(VIR_DOMAIN_SNAPSHOT_CREATE_NO_METADATA, NULL);
>
> if (!(def = virDomainSnapshotDefParseString(xmlDesc, data->caps,
> - data->xmlopt, 0, 0)))
> + data->xmlopt, -1, VIR_DOMAIN_SNAPSHOT_PARSE_DISKS |
> + VIR_DOMAIN_SNAPSHOT_PARSE_REDEFINE)))
Move the VIR_DOMAIN_SNAPSHOT... down one line so that it can be moved
left and hopefully fit within 80 columns.
> goto cleanup;
>
> if (def->ndisks) {
> @@ -5965,7 +5981,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
> _("disk snapshots not supported yet"));
> goto cleanup;
> }
> -
Any particular reason for removing this blank line (and the one below)?
> vboxIIDFromUUID(&domiid, dom->uuid);
> rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine);
> if (NS_FAILED(rc)) {
> @@ -5973,7 +5988,6 @@ vboxDomainSnapshotCreateXML(virDomainPtr dom,
> _("no domain with matching UUID"));
> goto cleanup;
> }
> -
> rc = machine->vtbl->GetState(machine, &state);
> if (NS_FAILED(rc)) {
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> @@ -6048,6 +6062,344 @@ cleanup:
> return ret;
> }
>
> +#if VBOX_API_VERSION >=4002
> +static
> +int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
> + virDomainSnapshotPtr snapshot)
> +{
> + virDomainPtr dom = snapshot->domain;
> + VBOX_OBJECT_CHECK(dom->conn, int, -1);
> + vboxIID domiid = VBOX_IID_INITIALIZER;
> + IMachine *machine = NULL;
> + ISnapshot *snap = NULL;
> + IMachine *snapMachine = NULL;
> + bool error = false;
The combination of having "error" and "ret" is confusing, and the
possibility at the bottom of the function of either going to cleanup to
return "ret" or dropping through to check for error == true and then
return 0 or -1 is also confusing. You can probably make it much simpler
by removing error, initializing ret = -1, then when you get to cleanup
at the end, just check for ret < 0 and do the error cleanup in that case.
All of the places you set "error = true" would then just be "goto
cleanup" (I'm not sure why your current code continues to process once
it has encountered an error, but I'm making the assumption that is
superfluous and not intentional).
For the case of success, you would just drop through to the cleanup
label, and just before that would be a "ret = 0;". This is the way a
vast majority of functions in libvirt are written, and it works very
well for us in most cases.
> + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
> + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> + int diskCount = 0;
> + nsresult rc;
> + vboxIID snapIid = VBOX_IID_INITIALIZER;
> + char *snapshotUuidStr = NULL;
> + size_t i = 0;
> + vboxIIDFromUUID(&domiid, dom->uuid);
> + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine);
> + reportInternalErrorIfNS_FAILED("no domain with matching UUID");
> + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name))) {
> + ret = -1;
> + goto cleanup;
> + }
> + rc = snap->vtbl->GetId(snap, &snapIid.value);
> + reportInternalErrorIfNS_FAILED("Could not get snapshot id");
> +
> + VBOX_UTF16_TO_UTF8(snapIid.value, &snapshotUuidStr);
> + rc = snap->vtbl->GetMachine(snap, &snapMachine);
> + reportInternalErrorIfNS_FAILED("could not get machine");
> + def->ndisks = 0;
> + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments);
> + reportInternalErrorIfNS_FAILED("no medium attachments");
> + /* get the number of attachments */
> + for (i = 0; i < mediumAttachments.count; i++) {
> + IMediumAttachment *imediumattach = mediumAttachments.items[i];
> + if (imediumattach) {
> + IMedium *medium = NULL;
> +
> + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium);
> + reportInternalErrorIfNS_FAILED("cannot get medium");
> + if (medium) {
> + def->ndisks++;
> + VBOX_RELEASE(medium);
> + }
> + }
> + }
> + /* Allocate mem, if fails return error */
> + if (VIR_ALLOC_N(def->disks, def->ndisks) < 0) {
> + virReportOOMError();
You no longer need to report an error if VIR_ALLOC_N() fails - it's done
for you; just return failure to your caller.
> + error = true;
> + }
> + if (!error)
> + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort);
> +
> + /* get the attachment details here */
> + for (i = 0; i < mediumAttachments.count && diskCount < def->ndisks && !error; i++) {
> + IStorageController *storageController = NULL;
> + PRUnichar *storageControllerName = NULL;
> + PRUint32 deviceType = DeviceType_Null;
> + PRUint32 storageBus = StorageBus_Null;
> + IMedium *disk = NULL;
> + PRUnichar *childLocUtf16 = NULL;
> + char *childLocUtf8 = NULL;
> + PRUint32 deviceInst = 0;
> + PRInt32 devicePort = 0;
> + PRInt32 deviceSlot = 0;
> + vboxArray children = VBOX_ARRAY_INITIALIZER;
> + vboxArray snapshotIids = VBOX_ARRAY_INITIALIZER;
> + IMediumAttachment *imediumattach = mediumAttachments.items[i];
> + size_t j = 0;
> + size_t k = 0;
> + if (!imediumattach)
> + continue;
> + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk);
> + reportInternalErrorIfNS_FAILED("cannot get medium");
> + if (!disk)
> + continue;
> + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName);
> + reportInternalErrorIfNS_FAILED("cannot get medium");
> + if (!storageControllerName) {
> + VBOX_RELEASE(disk);
> + continue;
> + }
> + rc= vboxArrayGet(&children, disk, disk->vtbl->GetChildren);
> + reportInternalErrorIfNS_FAILED("cannot get children disk");
> + rc = vboxArrayGetWithPtrArg(&snapshotIids, disk, disk->vtbl->GetSnapshotIds, domiid.value);
> + reportInternalErrorIfNS_FAILED("cannot get snapshot ids");
> + for (j = 0; j < children.count; ++j) {
> + IMedium *child = children.items[j];
> + for (k = 0; k < snapshotIids.count; ++k) {
> + PRUnichar *diskSnapId = snapshotIids.items[k];
> + char *diskSnapIdStr = NULL;
> + VBOX_UTF16_TO_UTF8(diskSnapId, &diskSnapIdStr);
> + if (STREQ(diskSnapIdStr, snapshotUuidStr)) {
> + rc = machine->vtbl->GetStorageControllerByName(machine,
> + storageControllerName,
> + &storageController);
> + VBOX_UTF16_FREE(storageControllerName);
> + if (!storageController) {
> + VBOX_RELEASE(child);
> + break;
> + }
> + rc = child->vtbl->GetLocation(child, &childLocUtf16);
> + reportInternalErrorIfNS_FAILED("cannot get disk location");
> + VBOX_UTF16_TO_UTF8(childLocUtf16, &childLocUtf8);
> + VBOX_UTF16_FREE(childLocUtf16);
> + ignore_value(VIR_STRDUP(def->disks[diskCount].file, childLocUtf8));
> + if (!(def->disks[diskCount].file)) {
> + VBOX_RELEASE(child);
> + VBOX_RELEASE(storageController);
> + virReportOOMError();
Same here - VIR_STRDUP() reports the error for you. (in the rare case
that you *don't* want it automatically reported, you would call
VIR_STRDUP_QUIET()).
(I probably ignored other occurrences of this problem in your patches...)
> + error = true;
> + break;
> + }
> + VBOX_UTF8_FREE(childLocUtf8);
> +
> + rc = storageController->vtbl->GetBus(storageController, &storageBus);
> + reportInternalErrorIfNS_FAILED("cannot get storage controller bus");
> + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType);
> + reportInternalErrorIfNS_FAILED("cannot get medium attachment type");
> + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort);
> + reportInternalErrorIfNS_FAILED("cannot get medium attachchment type");
> + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot);
> + reportInternalErrorIfNS_FAILED("cannot get medium attachment device");
> + def->disks[diskCount].name = vboxGenerateMediumName(storageBus,
> + deviceInst,
> + devicePort,
> + deviceSlot,
> + maxPortPerInst,
> + maxSlotPerPort);
> + }
> + VBOX_UTF8_FREE(diskSnapIdStr);
> + }
> + }
> + VBOX_RELEASE(storageController);
> + VBOX_RELEASE(disk);
> + diskCount++;
> + }
> + vboxArrayRelease(&mediumAttachments);
> + /* cleanup on error */
> + if (error) {
> + for (i = 0; i < def->dom->ndisks; i++) {
> + VIR_FREE(def->dom->disks[i]);
> + }
> + VIR_FREE(def->dom->disks);
> + def->dom->ndisks = 0;
> + return -1;
> + }
> + return 0;
> +
> +cleanup:
> + VBOX_RELEASE(snap);
> + return ret;
> +}
> +
> +static
> +int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
> + virDomainSnapshotDefPtr def)
> +{
> + virDomainPtr dom = snapshot->domain;
> + VBOX_OBJECT_CHECK(dom->conn, int, -1);
> + vboxIID domiid = VBOX_IID_INITIALIZER;
> + ISnapshot *snap = NULL;
> + IMachine *machine = NULL;
> + IMachine *snapMachine = NULL;
> + IStorageController *storageController = NULL;
> + IMedium *disk = NULL;
> + nsresult rc;
> + vboxIIDFromUUID(&domiid, dom->uuid);
> + bool error = false;
> + vboxArray mediumAttachments = VBOX_ARRAY_INITIALIZER;
> + size_t i = 0;
> + PRUint32 maxPortPerInst[StorageBus_Floppy + 1] = {};
> + PRUint32 maxSlotPerPort[StorageBus_Floppy + 1] = {};
> + int diskCount = 0;
> + rc = VBOX_OBJECT_GET_MACHINE(domiid.value, &machine);
Put a blank line between the automatic data declarations and the first
statement.
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_NO_DOMAIN, "%s",
> + _("no domain with matching UUID"));
> + return -1;
> + }
Hmm. You defined a macro to do almost exactly this, but couldn't use it
here because you couldn't goto cleanup. Are the vbox cleanup
unctions/macros not safe to call when the data they're cleaning up is
still NULL?
(not that I think you should try to use that macro - I think it should
be killed. I'm just curious why you didn't use it here)
> +
> + if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name)))
> + return -1;
> + rc = snap->vtbl->GetMachine(snap, &snapMachine);
> + reportInternalErrorIfNS_FAILED("cannot get machine");
> + /*
> + * Get READ ONLY disks
> + * In the snapshot metadata, these are the disks written inside the <domain> node
> + */
> + rc = vboxArrayGet(&mediumAttachments, snapMachine, snapMachine->vtbl->GetMediumAttachments);
> + reportInternalErrorIfNS_FAILED("cannot get medium attachments");
> + /* get the number of attachments */
> + for (i = 0; i < mediumAttachments.count; i++) {
> + IMediumAttachment *imediumattach = mediumAttachments.items[i];
> + if (imediumattach) {
> + IMedium *medium = NULL;
> +
> + rc = imediumattach->vtbl->GetMedium(imediumattach, &medium);
> + reportInternalErrorIfNS_FAILED("cannot get medium");
> + if (medium) {
> + def->dom->ndisks++;
> + VBOX_RELEASE(medium);
> + }
> + }
> + }
> +
> + /* Allocate mem, if fails return error */
> + if (VIR_ALLOC_N(def->dom->disks, def->dom->ndisks) >= 0) {
> + for (i = 0; i < def->dom->ndisks; i++) {
> + if (VIR_ALLOC(def->dom->disks[i]) < 0) {
> + virReportOOMError();
> + error = true;
Again, unneeded call to virReportOOMError(), and it would be better to
have initialized ret = -1 at the top, then done "goto cleanup" here.
> + break;
> + }
> + }
> + } else {
> + virReportOOMError();
> + error = true;
Likewise. I'll stop pointing these out now...
> + }
> +
> + if (!error)
> + error = !vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort);
> +
> + /* get the attachment details here */
> + for (i = 0; i < mediumAttachments.count && diskCount < def->dom->ndisks && !error; i++) {
You' going to a lot of trouble to avoid executing this code after error
has be set to true, again indicating that it would be simpler to just
goto cleanup immediately when you encounter an error (and have cleanup
contain the proper code to cleanup from an error).
> + PRUnichar *storageControllerName = NULL;
> + PRUint32 deviceType = DeviceType_Null;
> + PRUint32 storageBus = StorageBus_Null;
> + PRBool readOnly = PR_FALSE;
> + PRUnichar *mediumLocUtf16 = NULL;
> + char *mediumLocUtf8 = NULL;
> + PRUint32 deviceInst = 0;
> + PRInt32 devicePort = 0;
> + PRInt32 deviceSlot = 0;
> + IMediumAttachment *imediumattach = mediumAttachments.items[i];
> + if (!imediumattach)
> + continue;
> + rc = imediumattach->vtbl->GetMedium(imediumattach, &disk);
> + reportInternalErrorIfNS_FAILED("cannot get medium");
> + if (!disk)
> + continue;
> + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName);
> + reportInternalErrorIfNS_FAILED("cannot get storage controller name");
> + if (!storageControllerName) {
> + continue;
> + }
> + rc = machine->vtbl->GetStorageControllerByName(machine,
> + storageControllerName,
> + &storageController);
> + reportInternalErrorIfNS_FAILED("cannot get storage controller");
> + VBOX_UTF16_FREE(storageControllerName);
> + if (!storageController) {
> + continue;
> + }
> + rc = disk->vtbl->GetLocation(disk, &mediumLocUtf16);
> + reportInternalErrorIfNS_FAILED("cannot get disk location");
> + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
> + VBOX_UTF16_FREE(mediumLocUtf16);
> + ignore_value(VIR_STRDUP(def->dom->disks[diskCount]->src, mediumLocUtf8));
> +
> + if (!(def->dom->disks[diskCount]->src)) {
> + virReportOOMError();
> + ret = -1;
> + goto cleanup;
> + }
> + VBOX_UTF8_FREE(mediumLocUtf8);
> +
> + rc = storageController->vtbl->GetBus(storageController, &storageBus);
> + reportInternalErrorIfNS_FAILED("cannot get storage controller bus");
> + if (storageBus == StorageBus_IDE) {
> + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_IDE;
> + } else if (storageBus == StorageBus_SATA) {
> + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SATA;
> + } else if (storageBus == StorageBus_SCSI) {
> + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_SCSI;
> + } else if (storageBus == StorageBus_Floppy) {
> + def->dom->disks[diskCount]->bus = VIR_DOMAIN_DISK_BUS_FDC;
> + }
> +
> + rc = imediumattach->vtbl->GetType(imediumattach, &deviceType);
> + reportInternalErrorIfNS_FAILED("cannot get medium attachment type");
> + if (deviceType == DeviceType_HardDisk)
> + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_DISK;
> + else if (deviceType == DeviceType_Floppy)
> + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_FLOPPY;
> + else if (deviceType == DeviceType_DVD)
> + def->dom->disks[diskCount]->device = VIR_DOMAIN_DISK_DEVICE_CDROM;
> +
> + rc = imediumattach->vtbl->GetPort(imediumattach, &devicePort);
> + reportInternalErrorIfNS_FAILED("cannot get medium attachment port");
> + rc = imediumattach->vtbl->GetDevice(imediumattach, &deviceSlot);
> + reportInternalErrorIfNS_FAILED("cannot get device");
> + rc = disk->vtbl->GetReadOnly(disk, &readOnly);
> + reportInternalErrorIfNS_FAILED("cannot get read only attribute");
> + if (readOnly == PR_TRUE)
> + def->dom->disks[diskCount]->readonly = true;
> + def->dom->disks[diskCount]->type = VIR_DOMAIN_DISK_TYPE_FILE;
> + def->dom->disks[diskCount]->dst = vboxGenerateMediumName(storageBus,
> + deviceInst,
> + devicePort,
> + deviceSlot,
> + maxPortPerInst,
> + maxSlotPerPort);
> + if (!def->dom->disks[diskCount]->dst) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Could not generate medium name for the disk "
> + "at: controller instance:%u, port:%d, slot:%d"),
> + deviceInst, devicePort, deviceSlot);
> + ret = -1;
> + goto cleanup;
> + }
> + diskCount ++;
> + }
> + /* cleanup on error */
... but not on *all* errors, since you sometimes set error = true and
continue all the way down to here, and sometime you set set = -1 and
goto cleanup (thus skipping this extra cleanup). ALL of the cleanup
should be after the cleanup label, and you should always goto cleanup
when you encounter an error. That way you won't have some error recovery
paths that skip some of the cleanup.
> + if (error) {
> + for (i = 0; i < def->dom->ndisks; i++) {
> + VIR_FREE(def->dom->disks[i]);
> + }
> + VIR_FREE(def->dom->disks);
> + def->dom->ndisks = 0;
> + ret = -1;
> + goto cleanup;
> + }
> + ret = 0;
> +cleanup:
> + VBOX_RELEASE(disk);
> + VBOX_RELEASE(storageController);
> + vboxArrayRelease(&mediumAttachments);
> + VBOX_RELEASE(snap);
> + return ret;
> +}
> +#endif
> +
> static char *
> vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> unsigned int flags)
> @@ -6065,6 +6417,10 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> PRInt64 timestamp;
> PRBool online = PR_FALSE;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> +#if VBOX_API_VERSION >=4002
> + PRUint32 memorySize = 0;
> + PRUint32 CPUCount = 0;
> +#endif
>
> virCheckFlags(0, NULL);
>
> @@ -6079,11 +6435,40 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> if (!(snap = vboxDomainSnapshotGet(data, dom, machine, snapshot->name)))
> goto cleanup;
>
> - if (VIR_ALLOC(def) < 0)
> + if (VIR_ALLOC(def) < 0
> + || VIR_ALLOC(def->dom) < 0)
> goto cleanup;
> if (VIR_STRDUP(def->name, snapshot->name) < 0)
> goto cleanup;
>
> +#if VBOX_API_VERSION >=4002
> + /* Register def->dom properties for them to be saved inside the snapshot XMl
> + * Otherwise, there is a problem while parsing the xml
> + */
> + def->dom->virtType = VIR_DOMAIN_VIRT_VBOX;
> + def->dom->id = dom->id;
> + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
> + ignore_value(VIR_STRDUP(def->dom->name, dom->name));
> + machine->vtbl->GetMemorySize(machine, &memorySize);
> + def->dom->mem.cur_balloon = memorySize * 1024;
> + /* Currently setting memory and maxMemory as same, cause
> + * the notation here seems to be inconsistent while
> + * reading and while dumping xml
> + */
> + def->dom->mem.max_balloon = memorySize * 1024;
> + ignore_value(VIR_STRDUP(def->dom->os.type, "hvm"));
> + def->dom->os.arch = virArchFromHost();
> + machine->vtbl->GetCPUCount(machine, &CPUCount);
> + def->dom->maxvcpus = def->dom->vcpus = CPUCount;
> + if (vboxSnapshotGetReadWriteDisks(def, snapshot) < 0) {
> + VIR_DEBUG("Could not get read write disks for snapshot");
> + }
> +
> + if (vboxSnapshotGetReadOnlyDisks(snapshot, def) <0) {
> + VIR_DEBUG("Could not get Readonly disks for snapshot");
> + }
> +#endif /* VBOX_API_VERSION >= 4002 */
> +
> rc = snap->vtbl->GetDescription(snap, &str16);
> if (NS_FAILED(rc)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -6148,8 +6533,8 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> def->state = VIR_DOMAIN_SHUTOFF;
>
> virUUIDFormat(dom->uuid, uuidstr);
> + memcpy(def->dom->uuid, dom->uuid, VIR_UUID_BUFLEN);
> ret = virDomainSnapshotDefFormat(uuidstr, def, flags, 0);
> -
> cleanup:
> virDomainSnapshotDefFree(def);
> VBOX_RELEASE(parent);
> @@ -6854,6 +7239,8 @@ cleanup:
> return ret;
> }
>
> +
> +
> static int
> vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> unsigned int flags)
> @@ -6889,8 +7276,9 @@ vboxDomainSnapshotDelete(virDomainSnapshotPtr snapshot,
> goto cleanup;
> }
>
> - /* VBOX snapshots do not require any libvirt metadata, making this
> - * flag trivial once we know we have a valid snapshot. */
> + /* In case we just want to delete the metadata, we will edit the vbox file in order
> + *to remove the node concerning the snapshot
> + */
> if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_METADATA_ONLY) {
> ret = 0;
> goto cleanup;
> @@ -8716,8 +9104,9 @@ cleanup:
> return ret;
> }
>
> -static int vboxStorageVolDelete(virStorageVolPtr vol,
> - unsigned int flags)
> +static int vboxStorageDeleteOrClose(virStorageVolPtr vol,
> + unsigned int flags,
> + unsigned int flagDeleteOrClose)
> {
> VBOX_OBJECT_CHECK(vol->conn, int, -1);
> vboxIID hddIID = VBOX_IID_INITIALIZER;
> @@ -8872,8 +9261,18 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
>
> if (machineIdsSize == 0 || machineIdsSize == deregister) {
> IProgress *progress = NULL;
> -
> +#if VBOX_API_VERSION >= 4002
> + if (flagDeleteOrClose & VBOX_STORAGE_CLOSE_FLAG) {
> + rc = hardDisk->vtbl->Close(hardDisk);
> + if (NS_SUCCEEDED(rc)) {
> + DEBUGIID("HardDisk closed, UUID", hddIID.value);
> + ret = 0;
> + }
> + }
> +#endif
> + if (flagDeleteOrClose & VBOX_STORAGE_DELETE_FLAG){
> rc = hardDisk->vtbl->DeleteStorage(hardDisk, &progress);
> + }
>
> if (NS_SUCCEEDED(rc) && progress) {
> progress->vtbl->WaitForCompletion(progress, -1);
> @@ -8892,6 +9291,12 @@ static int vboxStorageVolDelete(virStorageVolPtr vol,
> return ret;
> }
>
> +static int vboxStorageVolDelete(virStorageVolPtr vol,
> + unsigned int flags)
> +{
> + return vboxStorageDeleteOrClose(vol, flags, VBOX_STORAGE_DELETE_FLAG);
> +}
> +
> static int vboxStorageVolGetInfo(virStorageVolPtr vol, virStorageVolInfoPtr info) {
> VBOX_OBJECT_CHECK(vol->conn, int, -1);
> IHardDisk *hardDisk = NULL;
More information about the libvir-list
mailing list