[libvirt] [PATCH v7 2/4] Add vbox_snapshot_conf struct

[Daniel P. Berrange]

On Fri, Apr 18, 2014 at 11:51:32AM +0200, Yohan BELLEGUIC wrote:
> This structure contains the data to be saved in the VirtualBox XML file
> and can be manipulated with severals exposed functions.
> The structure is created by vboxSnapshotLoadVboxFile taking the
> machine XML file.
> It also can rewrite the XML by using vboxSnapshotSaveVboxFile.
> ---
>  po/POTFILES.in                |    1 +
>  src/Makefile.am               |    1 +
>  src/vbox/vbox_snapshot_conf.c | 1383 +++++++++++++++++++++++++++++++++++++++++
>  src/vbox/vbox_snapshot_conf.h |  100 +++
>  4 files changed, 1485 insertions(+)
>  create mode 100644 src/vbox/vbox_snapshot_conf.c
>  create mode 100644 src/vbox/vbox_snapshot_conf.h


> +/*Stores VirtualBox xml hard disk information
> +A hard disk can have a parent and children*/
> +typedef struct _vboxSnapshotXmlHardDisk vboxSnapshotXmlHardDisk;
> +typedef vboxSnapshotXmlHardDisk *vboxSnapshotXmlHardDiskPtr;
> +struct _vboxSnapshotXmlHardDisk {

XML is really only the serialization format - in terms of data we're
talking about snapshot configuration. So I'd suggest all these structs
should use  'Conf' in their name rather than 'Xml'.

Since this is a brand new file, I think it would be nice if the structs
and functions in this particular .c/.h file could use our latest preferred
naming convention which is to prefix a 'vir' and the driver name on all
things. So in this case, I'd suggest that all the structs be called

   virVBoxSnapshotConfXXXXXX

This will make it clearer which functions are libvirt internal code
vs part of the vbox API

> +    vboxSnapshotXmlHardDiskPtr parent;
> +    char *uuid;
> +    char *location;
> +    char *format;
> +    char *type;
> +    size_t nchildren;
> +    vboxSnapshotXmlHardDiskPtr *children;
> +};
> +
> +/*Stores Virtualbox xml media registry information
> +We separate disks from other media to manipulate them*/
> +typedef struct _vboxSnapshotXmlMediaRegistry vboxSnapshotXmlMediaRegistry;
> +typedef vboxSnapshotXmlMediaRegistry *vboxSnapshotXmlMediaRegistryPtr;
> +struct _vboxSnapshotXmlMediaRegistry {
> +    size_t ndisks;
> +    vboxSnapshotXmlHardDiskPtr *disks;
> +    size_t notherMedia;
> +    char **otherMedia;
> +};
> +
> +/*Stores VirtualBox xml snapshot information
> +A snapshot can have a parent and children*/
> +typedef struct _vboxSnapshotXmlSnapshot vboxSnapshotXmlSnapshot;
> +typedef vboxSnapshotXmlSnapshot *vboxSnapshotXmlSnapshotPtr;
> +struct _vboxSnapshotXmlSnapshot {
> +    vboxSnapshotXmlSnapshotPtr parent;
> +    char *uuid;
> +    char *name;
> +    char *timeStamp;
> +    char *description;
> +    char *hardware;
> +    char *storageController;
> +    size_t nchildren;
> +    vboxSnapshotXmlSnapshotPtr *children;
> +};
> +
> +/*Stores VirtualBox xml Machine information*/
> +typedef struct _vboxSnapshotXmlMachine vboxSnapshotXmlMachine;
> +typedef vboxSnapshotXmlMachine *vboxSnapshotXmlMachinePtr;
> +struct _vboxSnapshotXmlMachine {
> +    char *uuid;
> +    char *name;
> +    char *currentSnapshot;
> +    char *snapshotFolder;
> +    int currentStateModified;
> +    char *lastStateChange;
> +    vboxSnapshotXmlMediaRegistryPtr mediaRegistry;
> +    char *hardware;
> +    char *extraData;
> +    vboxSnapshotXmlSnapshotPtr snapshot;
> +    char *storageController;
> +};
> +
> +vboxSnapshotXmlMachinePtr vboxSnapshotLoadVboxFile(const char *filePath, char *machineLocation);
> +int addSnapshotToXmlMachine(vboxSnapshotXmlSnapshotPtr snapshot, vboxSnapshotXmlMachinePtr machine, char *snapshotParentName);
> +int addHardDiskToMediaRegistry(vboxSnapshotXmlHardDiskPtr hardDisk, vboxSnapshotXmlMediaRegistryPtr mediaRegistry, char *parentHardDiskId);
> +int removeSnapshot(vboxSnapshotXmlMachinePtr machine, char *snapshotName);
> +int removeHardDisk(vboxSnapshotXmlMediaRegistryPtr mediaRegistry,char *uuid);
> +int vboxSnapshotSaveVboxFile(vboxSnapshotXmlMachinePtr machine, const char *filePath);
> +int isCurrentSnapshot(vboxSnapshotXmlMachinePtr machine, char *snapshotName);
> +int getRWDisksPathsFromLibvirtXML(char *filePath, char ***realReadWriteDisksPath);
> +int getRODisksPathsFromLibvirtXML(char *filePath, char ***realReadOnlyDisksPath);
> +char *hardDiskUuidByLocation(vboxSnapshotXmlMachinePtr machine, char *location);
> +size_t diskListToOpen(vboxSnapshotXmlMachinePtr machine, vboxSnapshotXmlHardDiskPtr **hardDiskToOpen, char *location);
> +int removeFakeDisks(vboxSnapshotXmlMachinePtr machine);
> +int diskIsInMediaRegistry(vboxSnapshotXmlMachinePtr machine, char *location);
> +vboxSnapshotXmlHardDiskPtr hardDiskPtrByLocation(vboxSnapshotXmlMachinePtr machine, char *location);
> +vboxSnapshotXmlSnapshotPtr snapshotByName(vboxSnapshotXmlSnapshotPtr snapshot, char *snapshotName);

And all of these functions (and any other static functions in the .c
file) shoudl also have  virVBoxSnapshotConf as the prefix in their
names.

Since you have now nicely separated the XML parsing and formatting
code, it'd be preferrable if you also wrote a test case for this.

The general idea would be that you have a bunch of XML files in a
directory  tests/vboxsnapshotxmldata/*.xml, then create a file
tests/vboxsnapshotxmltest.c. In this test, you would first call
the function which parses XML, to give you a virVBoxSnapshotConfigMachinePtr
instance, and then imediately call the function to turn this struct
instance back into an XML document. Then compare the original XML doc
with the XML doc you've just created.

This lets us validate various important things before we merge it

 - The code is correctly round-tripping all data through parser & formatter
   and not loosing or mangling anything

 - There are no memory leaks in the code 
      eg valgrind --leak-check=full tests/vboxsnapshotxmltest

 - The code doesn't crash upon OOM errors [1]

      eg ./configure --enable-test-oom
      make
      VIR_TEST_OOM=1 tests/vboxsnapshotxmltest

and most of all ensures we don't cause regressions in the future.

Regards,
Daniel

[1] http://libvirt.org/internals/oomtesting.html
-- 
|: 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 :|