[libvirt] [PATCH v7 1/4] vbox_tmpl.c: Better XML description for snapshots
Daniel P. Berrange
berrange at redhat.com
Thu May 1 10:28:14 UTC 2014
On Fri, Apr 18, 2014 at 11:51:31AM +0200, Yohan BELLEGUIC wrote:
> From: Manuel VIVES <manuel.vives at diateam.net>
>
> It will be needed for the future patches because we will
> redefine snapshots
> ---
> src/vbox/vbox_tmpl.c | 513 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 504 insertions(+), 9 deletions(-)
>
> diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
> index a305fe2..ac000cf 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>
This can be removed, since the XML creation is now a separate module
in your second patch.
> @@ -294,6 +297,13 @@ static void vboxDriverUnlock(vboxGlobalData *data)
> virMutexUnlock(&data->lock);
> }
>
> +typedef enum {
> + VBOX_STORAGE_DELETE_FLAG = 0,
> +#if VBOX_API_VERSION >= 4002000
> + VBOX_STORAGE_CLOSE_FLAG = 1,
> +#endif
IMHO remove this #if and unconditionally define the VBOX_STORAGE_CLOSE_FLAG
item. There's no harm having it even if you don't use it, and less #ifdefs
makes for clearer code
> +#if VBOX_API_VERSION >=4002000
> +static
> +int vboxSnapshotGetReadWriteDisks(virDomainSnapshotDefPtr def,
> + virDomainSnapshotPtr snapshot)
> + ret = 0;
> + cleanup:
> + if (ret < 0) {
> + for (i = 0; i < def->dom->ndisks; i++) {
> + VIR_FREE(def->dom->disks[i]);
> + }
Nitpick, no need for {} on the single line for() statement
> + VIR_FREE(def->dom->disks);
> + def->dom->ndisks = 0;
> + ret = -1;
> + }
> + VBOX_RELEASE(snap);
> + return ret;
> +}
> +static
> +int vboxSnapshotGetReadOnlyDisks(virDomainSnapshotPtr snapshot,
> + virDomainSnapshotDefPtr def)
> +{
> + /* 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)
> + goto cleanup;
> + }
> + } else
> + goto cleanup;
When one part of an if/else has {}, then HACKING rules require
that all parts use {}
> + if (!vboxGetMaxPortSlotValues(data->vboxObj, maxPortPerInst, maxSlotPerPort))
> + goto cleanup;
This indentation makes it look like you meant this statement to be
part of the 'else' clause. If not, reduce the indentation to reflect
what you intend
> +
> + /* get the attachment details here */
> + for (i = 0; i < mediumAttachments.count && diskCount < def->dom->ndisks; i++) {
> + 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);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get medium"));
> + goto cleanup;
> + }
> + if (!disk)
> + continue;
> + rc = imediumattach->vtbl->GetController(imediumattach, &storageControllerName);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get storage controller name"));
> + goto cleanup;
> + }
> + if (!storageControllerName) {
> + continue;
> + }
No need for {} on a single line if.
> + rc = machine->vtbl->GetStorageControllerByName(machine,
> + storageControllerName,
> + &storageController);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get storage controller"));
> + goto cleanup;
> + }
> + VBOX_UTF16_FREE(storageControllerName);
> + if (!storageController) {
> + continue;
> + }
And again
> + rc = disk->vtbl->GetLocation(disk, &mediumLocUtf16);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get disk location"));
> + goto cleanup;
> + }
> + VBOX_UTF16_TO_UTF8(mediumLocUtf16, &mediumLocUtf8);
> + VBOX_UTF16_FREE(mediumLocUtf16);
> + if (VIR_STRDUP(def->dom->disks[diskCount]->src.path, mediumLocUtf8) < 0)
> + goto cleanup;
> +
> + VBOX_UTF8_FREE(mediumLocUtf8);
> +
> + rc = storageController->vtbl->GetBus(storageController, &storageBus);
> + if (NS_FAILED(rc)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("cannot get storage controller bus"));
> + goto cleanup;
> + }
> + 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;
> + }
> +
> + cleanup:
> + if (ret < 0) {
> + for (i = 0; i < def->dom->ndisks; i++) {
> + VIR_FREE(def->dom->disks[i]);
> + }
No need for {}
> + VIR_FREE(def->dom->disks);
> + def->dom->ndisks = 0;
> + ret = -1;
> + goto cleanup;
This 'goto cleanup' causes an infinite loop !
> + }
> + VBOX_RELEASE(disk);
> + VBOX_RELEASE(storageController);
> + vboxArrayRelease(&mediumAttachments);
> + VBOX_RELEASE(snap);
> + return ret;
> +}
> +#endif
> +
> static char *
> vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> unsigned int flags)
> @@ -6147,6 +6589,10 @@ vboxDomainSnapshotGetXMLDesc(virDomainSnapshotPtr snapshot,
> PRInt64 timestamp;
> PRBool online = PR_FALSE;
> char uuidstr[VIR_UUID_STRING_BUFLEN];
> +#if VBOX_API_VERSION >=4002000
> + PRUint32 memorySize = 0;
> + PRUint32 CPUCount = 0;
> +#endif
>
> virCheckFlags(0, NULL);
>
> @@ -6161,11 +6607,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;
Slightly more usual style is to put '||' on the end of the previous
line, rather than start of the next line.
> if (VIR_STRDUP(def->name, snapshot->name) < 0)
> goto cleanup;
>
> +#if VBOX_API_VERSION >=4002000
> + /* 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));
Nooo, ignoring OOM like this is not ok.[
> + 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"));
Again this isn't ok.
> + 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) {
Need a space in '<0' to make it '< 0' - i think 'make syntax-check'
should complain about this.
> + VIR_DEBUG("Could not get Readonly disks for snapshot");
> + }
> +#endif /* VBOX_API_VERSION >= 4002000 */
> +
> rc = snap->vtbl->GetDescription(snap, &str16);
> if (NS_FAILED(rc)) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -6230,6 +6705,7 @@ 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:
> @@ -6936,6 +7412,8 @@ vboxDomainSnapshotDeleteTree(vboxGlobalData *data,
> return ret;
> }
I've mostly reviewed this for style pointed out a few nitpicks and
one serious bug. Superficially it looks ok functionally, but this
neither vbox or snapshots are something I'm familiar enough with
so will defer to someone else on that.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
More information about the libvir-list
mailing list