Re: [PATCHv4 3/5] vbox_tmpl.c: Better XML description for snapshots

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

 



> 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




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