> 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.
>
This portion was safely removed from the patch.
> > 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.
I removed this macro in order to have 'goto' instructions clearly
visible but it increases the size of the patch.
> > 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?
>
My mistake, I should have used the macro.
> (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;
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list