On Thu, Oct 10, 2019 at 01:43:09PM +0200, Simon Kobyda wrote:

Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- data/org.libvirt.DomainSnapshot.xml | 7 +++ src/connect.c | 6 +++ src/connect.h | 1 + src/domainsnapshot.c | 79 +++++++++++++++++++++++++++++ src/domainsnapshot.h | 9 ++++ src/meson.build | 1 + src/util.c | 49 ++++++++++++++++++ src/util.h | 16 ++++++ 8 files changed, 168 insertions(+) create mode 100644 data/org.libvirt.DomainSnapshot.xml create mode 100644 src/domainsnapshot.c create mode 100644 src/domainsnapshot.h

diff --git a/data/org.libvirt.DomainSnapshot.xml b/data/org.libvirt.DomainSnapshot.xml new file mode 100644 index 0000000..80f61c6 --- /dev/null +++ b/data/org.libvirt.DomainSnapshot.xml @@ -0,0 +1,7 @@ +<!DOCTYPE node PUBLIC "-//freedesktop//DTD D-BUS Object Introspection 1.0//EN" +"http://www.freedesktop.org/standards/dbus/1.0/introspect.dtd"> + +<node name="/org/libvirt/snapshot">

I would rather name it as domainsnapshot. It's unlikely that we will ever have some other type of snapshot in libvirt but it will at least follow the same naming as in libvirt APIs.

+ <interface name="org.libvirt.DomainSnapshot"> + </interface> +</node> diff --git a/src/connect.c b/src/connect.c index f8f76a2..b40b0ba 100644 --- a/src/connect.c +++ b/src/connect.c @@ -1,5 +1,6 @@ #include "connect.h" #include "domain.h" +#include "domainsnapshot.h" #include "events.h" #include "interface.h" #include "network.h" @@ -2002,6 +2003,7 @@ virtDBusConnectFree(virtDBusConnect *connect) virtDBusConnectClose(connect, TRUE);

g_free(connect->domainPath); + g_free(connect->domainSnapshotPath); g_free(connect->interfacePath); g_free(connect->networkPath); g_free(connect->nodeDevPath); @@ -2062,6 +2064,10 @@ virtDBusConnectNew(virtDBusConnect **connectp, if (error && *error) return;

+ virtDBusDomainSnapshotRegister(connect, error); + if (error && *error) + return; + virtDBusInterfaceRegister(connect, error); if (error && *error) return; diff --git a/src/connect.h b/src/connect.h index 829bd57..74c89cf 100644 --- a/src/connect.h +++ b/src/connect.h @@ -13,6 +13,7 @@ struct virtDBusConnect { const gchar *uri; const gchar *connectPath; gchar *domainPath; + gchar *domainSnapshotPath; gchar *interfacePath; gchar *networkPath; gchar *nodeDevPath; diff --git a/src/domainsnapshot.c b/src/domainsnapshot.c new file mode 100644 index 0000000..590cbef --- /dev/null +++ b/src/domainsnapshot.c @@ -0,0 +1,79 @@ +#include "domainsnapshot.h" +#include "util.h" + +#include <libvirt/libvirt.h> + +static virtDBusGDBusPropertyTable virtDBusDomainSnapshotPropertyTable[] = { + { 0 } +}; + +static virtDBusGDBusMethodTable virtDBusDomainSnapshotMethodTable[] = { + { 0 } +}; + +static gchar ** +virtDBusDomainSnapshotEnumerate(gpointer userData) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainPtr) domains = NULL; + gint numDoms = 0; + GPtrArray *list = NULL;

There is no need to use GPtrArray.

+ + if (!virtDBusConnectOpen(connect, NULL)) + return NULL; + + numDoms = virConnectListAllDomains(connect->connection, &domains, 0); + if (numDoms <= 0) + return NULL;

Here you can allocate the return array if strings directly ...

+ + list = g_ptr_array_new(); + + for (gint i = 0; i < numDoms; i++) { + g_autoptr(virDomainSnapshotPtr) domainSnapshots = NULL; + gint numSnaps; + + numSnaps = virDomainListAllSnapshots(domains[i], &domainSnapshots, 0); + if (numSnaps <= 0) + continue; + + for (gint j = 0; j < numSnaps; j++) { + gchar *snapPath = virtDBusUtilBusPathForVirDomainSnapshot(domains[i], + domainSnapshots[j], + connect->domainSnapshotPath); + g_ptr_array_add(list, snapPath);

... and here you can just add the path to the array. In addition the list that you are using is not used after this point.

+ } + } + + gchar **ret = g_new0(gchar *, numDoms + 1); + + for (gint i = 0; i < numDoms; i++) { + ret[i] = virtDBusUtilBusPathForVirDomain(domains[i], + connect->domainPath); + }

Here you call new for loop that will fill the ret value with domain paths instead of domainSnapshot paths.

+ + return ret; +} + +static GDBusInterfaceInfo *interfaceInfo = NULL; + +void +virtDBusDomainSnapshotRegister(virtDBusConnect *connect, + GError **error) +{ + connect->domainSnapshotPath = g_strdup_printf("%s/snapshot", connect->connectPath); + + if (!interfaceInfo) { + interfaceInfo = virtDBusGDBusLoadIntrospectData(VIRT_DBUS_DOMAIN_SNAPSHOT_INTERFACE, + error); + if (!interfaceInfo) + return; + } + + virtDBusGDBusRegisterSubtree(connect->bus, + connect->domainSnapshotPath, + interfaceInfo, + virtDBusDomainSnapshotEnumerate, + virtDBusDomainSnapshotMethodTable, + virtDBusDomainSnapshotPropertyTable, + connect); +} diff --git a/src/domainsnapshot.h b/src/domainsnapshot.h new file mode 100644 index 0000000..7d8a938 --- /dev/null +++ b/src/domainsnapshot.h @@ -0,0 +1,9 @@ +#pragma once + +#include "connect.h" + +#define VIRT_DBUS_DOMAIN_SNAPSHOT_INTERFACE "org.libvirt.DomainSnapshot" + +void +virtDBusDomainSnapshotRegister(virtDBusConnect *connect, + GError **error); diff --git a/src/meson.build b/src/meson.build index de96596..1839c79 100644 --- a/src/meson.build +++ b/src/meson.build @@ -16,6 +16,7 @@ exe_libvirt_dbus = executable( [ 'connect.c', 'domain.c', + 'domainsnapshot.c', 'events.c', 'gdbus.c', 'interface.c', diff --git a/src/util.c b/src/util.c index 103bb29..daf3c2a 100644 --- a/src/util.c +++ b/src/util.c @@ -279,6 +279,55 @@ virtDBusUtilVirDomainListFree(virDomainPtr *domains) g_free(domains); }

+virDomainSnapshotPtr +virtDBusUtilVirDomainSnapshotFromBusPath(virConnectPtr connection, + const gchar *path, + const gchar *domainSnapshotPath) +{ + g_autofree gchar *uuidStr = NULL; + g_autofree gchar *snapshotName = NULL; + g_autoptr(virDomain) domain = NULL; + gsize prefixLen = strlen(domainSnapshotPath) + 1; + gchar** strings = g_strsplit(path + prefixLen, "_", 2); + + uuidStr = virtDBusUtilDecodeStr(strings[0]); + snapshotName = virtDBusUtilDecodeStr(strings[1]); + + domain = virDomainLookupByUUIDString(connection, uuidStr); + + return virDomainSnapshotLookupByName(domain, snapshotName, 0); +} + +gchar * +virtDBusUtilBusPathForVirDomainSnapshot(virDomainPtr domain, + virDomainSnapshotPtr domainSnapshot, + const gchar *domainSnapshotPath) +{ + const gchar *snapshotName = NULL; + gchar uuidStr[VIR_UUID_STRING_BUFLEN] = ""; + g_autofree const gchar *encodedDomainNameStr = NULL; + g_autofree const gchar *encodedSnapshotNameStr = NULL; + + if (virDomainGetUUIDString(domain, uuidStr) < 0) + return virtDBusUtilSetLastVirtError(error);

This check is pointless since the libvirt function virDomainGetUUIDString will never fail, only programming error can make it fail and that's not our case. In addition this function fails to compile as there is no variable error.

+ + snapshotName = virDomainSnapshotGetName(domainSnapshot); + encodedDomainNameStr = virtDBusUtilEncodeStr(uuidStr);

Here you should call virtDBusUtilEncodeUUID as uuid is encoded differently. This would work but is not necessary.

+ encodedSnapshotNameStr = virtDBusUtilEncodeStr(snapshotName); + + return g_strdup_printf("%s/%s_%s", domainSnapshotPath, + encodedDomainNameStr, encodedSnapshotNameStr);

I'm not sure if single '_' is a good separator as that char is used to prefix hex value of encoded characters. For example uuid 00000000-1111-2222-3333-444444444444 is encoded into _00000000_1111_2222_3333_444444444444 by virtDBusUtilEncodeUUID or into 00000000_2d1111_2d2222_2d3333_2d444444444444 by virtDBusUtilEncodeStr As you can see the function virtDBusUtilVirDomainSnapshotFromBusPath whould fail to properly split the string into domainUUID part and snapshotName part in both cases. We need to figure out a different separator. To be on a safe side we can create a macro that will hold a length of the encoded domain UUID which will be always the same and simply us that length to split the domainSnapshotPath into domainUUID and snapshotName. Let's keep the formatting string as "%s/%s_%s" and in the function virtDBusUtilVirDomainSnapshotFromBusPath you can simply do this: /* result of strlen("_00000000_1111_2222_3333_444444444444") */ #define VIRT_DBUS_UUID_LEN = 37 ... virtDBusUtilVirDomainSnapshotFromBusPath(...) { ... g_autofree gchar *tmp = g_strdup(path + prefixLen); tmp[VIRT_DBUS_UUID_LEN] = 0; uuidStr = virtDBusUtilDecodeUUID(tmp); snapshotName = virtDBusUtilDecodeStr(tmp + VIRT_DBUS_UUID_LEN + 1) ... } If the path is like this: "_00000000_1111_2222_3333_444444444444_my_snapshot" It will replace the '_' after the UUID by 0 and you will get two separate strings. Pavel

+} + +void +virtDBusUtilVirDomainSnapshotListFree(virDomainSnapshotPtr* domainSnapshots) +{ + for (gint i = 0; domainSnapshots[i] != NULL; i++) + virDomainSnapshotFree(domainSnapshots[i]); + + g_free(domainSnapshots); +} + virInterfacePtr virtDBusUtilVirInterfaceFromBusPath(virConnectPtr connection, const gchar *path, diff --git a/src/util.h b/src/util.h index b05c2fc..c0b314d 100644 --- a/src/util.h +++ b/src/util.h @@ -65,6 +65,22 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainPtr, virtDBusUtilVirDomainListFree);

G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainStatsRecordPtr, virDomainStatsRecordListFree);

+virDomainSnapshotPtr +virtDBusUtilVirDomainSnapshotFromBusPath(virConnectPtr connection, + const gchar *path, + const gchar *domainSnapshotPath); + +gchar * +virtDBusUtilBusPathForVirDomainSnapshot(virDomainPtr domain, + virDomainSnapshotPtr domainSnapshot, + const gchar *domainSnapshotPath); + +void +virtDBusUtilVirDomainSnapshotListFree(virDomainSnapshotPtr* domainSnapshots); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSnapshot, virDomainSnapshotFree); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virDomainSnapshotPtr, virtDBusUtilVirDomainSnapshotListFree); + virInterfacePtr virtDBusUtilVirInterfaceFromBusPath(virConnectPtr connection, const gchar *path, -- 2.21.0

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Thu, Oct 10, 2019 at 01:43:10PM +0200, Simon Kobyda wrote:

Signed-off-by: Simon Kobyda <skobyda@redhat.com> --- data/org.libvirt.Domain.xml | 26 ++++ data/org.libvirt.DomainSnapshot.xml | 34 +++++ src/domain.c | 158 +++++++++++++++++++++ src/domainsnapshot.c | 205 +++++++++++++++++++++++++++- tests/libvirttest.py | 14 ++ tests/meson.build | 1 + tests/test_domain.py | 8 ++ tests/test_snapshot.py | 43 ++++++ tests/xmldata.py | 6 + 9 files changed, 494 insertions(+), 1 deletion(-) create mode 100755 tests/test_snapshot.py

diff --git a/data/org.libvirt.Domain.xml b/data/org.libvirt.Domain.xml index b4ed495..f03faf8 100644 --- a/data/org.libvirt.Domain.xml +++ b/data/org.libvirt.Domain.xml @@ -350,6 +350,12 @@ <arg name="flags" type="u" direction="in"/> <arg name="ifaces" type="a(ssa(isu))" direction="out"/> </method> + <method name="ListDomainSnapshots"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainListAllSnapshots"/> + <arg name="flags" type="u" direction="in"/> + <arg name="snaps" type="ao" direction="out"/>

No need to shorten the name, I would use "domainSnapshots".

+ </method> <method name="ManagedSave"> <annotation name="org.gtk.GDBus.DocString" value="See https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainManagedSave"/> @@ -589,6 +595,26 @@ value="See https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainShutdownFlags"/> <arg name="flags" type="u" direction="in"/> </method> + <method name="SnapshotCurrent"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotCurrent"/> + <arg name="flags" type="u" direction="in"/> + <arg name="domainSnapshot" type="o" direction="out"/> + </method> + <method name="SnapshotCreateXML"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotCreateXML"/> + <arg name="xml" type="s" direction="in"/> + <arg name="flags" type="u" direction="in"/> + <arg name="domainSnapshot" type="o" direction="out"/> + </method> + <method name="SnapshotLookupByName"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotLookupByName"/> + <arg name="name" type="s" direction="in"/> + <arg name="flags" type="u" direction="in"/> + <arg name="domainSnapshot" type="o" direction="out"/> + </method> <method name="Suspend"> <annotation name="org.gtk.GDBus.DocString" value="See https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainSuspend"/> diff --git a/data/org.libvirt.DomainSnapshot.xml b/data/org.libvirt.DomainSnapshot.xml index 80f61c6..463654f 100644 --- a/data/org.libvirt.DomainSnapshot.xml +++ b/data/org.libvirt.DomainSnapshot.xml @@ -3,5 +3,39 @@

<node name="/org/libvirt/snapshot"> <interface name="org.libvirt.DomainSnapshot"> + <method name="Delete"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotDelete"/> + <arg name="flags" type="u" direction="in"/> + </method> + <method name="GetParent"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotGetParent"/> + <arg name="flags" type="u" direction="in"/> + <arg name="parent" type="o" direction="out"/>

I would use "domainSnapshot" here ...

+ </method> + <method name="GetXMLDesc"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotGetXMLDesc"/> + <arg name="flags" type="u" direction="in"/> + <arg name="xml" type="s" direction="out"/> + </method> + <method name="IsCurrent"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotIsCurrent"/> + <arg name="flags" type="u" direction="in"/> + <arg name="current" type="b" direction="out"/>

... here ...

+ </method> + <method name="ListChildren"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainSnapshotListAllChildren"/> + <arg name="flags" type="u" direction="in"/> + <arg name="snaps" type="ao" direction="out"/>

... and here.

+ </method> + <method name="Revert"> + <annotation name="org.gtk.GDBus.DocString" + value="See https://libvirt.org/html/libvirt-libvirt-domain-snapshot.html#virDomainRevertToSnapshot"/> + <arg name="flags" type="u" direction="in"/> + </method> </interface> </node> diff --git a/src/domain.c b/src/domain.c index 10fa8de..3fc5bab 100644 --- a/src/domain.c +++ b/src/domain.c @@ -1857,6 +1857,50 @@ virtDBusDomainInjectNMI(GVariant *inArgs, virtDBusUtilSetLastVirtError(error); }

+static void +virtDBusDomainListDomainSnapshots(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomain) domain = NULL; + g_autoptr(virDomainSnapshotPtr) domainSnapshots = NULL; + guint flags; + GVariantBuilder builder; + GVariant *gdomainSnapshots; + + g_variant_get(inArgs, "(u)", &flags); + + domain = virtDBusDomainGetVirDomain(connect, objectPath, + error);

This fits into single line.

+ if (!domain) + return; + + if (!virtDBusConnectOpen(connect, error)) + return; + + if (virDomainListAllSnapshots(domain, &domainSnapshots, flags) < 0) + return virtDBusUtilSetLastVirtError(error); + + g_variant_builder_init(&builder, G_VARIANT_TYPE("ao")); + + for (gint i = 0; domainSnapshots[i]; i++) { + g_autofree gchar *path = NULL; + path = virtDBusUtilBusPathForVirDomainSnapshot(domain, + domainSnapshots[i], + connect->domainSnapshotPath); + + g_variant_builder_add(&builder, "o", path); + } + + gdomainSnapshots = g_variant_builder_end(&builder); + *outArgs = g_variant_new_tuple(&gdomainSnapshots, 1); +} + static void virtDBusDomainInterfaceAddresses(GVariant *inArgs, GUnixFDList *inFDs G_GNUC_UNUSED, @@ -2966,6 +3010,116 @@ virtDBusDomainShutdown(GVariant *inArgs, virtDBusUtilSetLastVirtError(error); }

+static void +virtDBusDomainSnapshotCurrent(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autofree gchar *path = NULL; + g_autoptr(virDomain) domain = NULL; + g_autoptr(virDomainSnapshot) snapshot = NULL; + guint flags; + + g_variant_get(inArgs, "(u)", &flags); + + domain = virtDBusDomainGetVirDomain(connect, objectPath, + error);

This fits into single line.

+ if (!domain) + return; + + if (!virtDBusConnectOpen(connect, error)) + return; + + snapshot = virDomainSnapshotCurrent(domain, flags); + if (!snapshot) + return virtDBusUtilSetLastVirtError(error); + + path = virtDBusUtilBusPathForVirDomainSnapshot(domain, + snapshot, + connect->domainSnapshotPath); + + *outArgs = g_variant_new("(o)", path); +} + +static void +virtDBusDomainSnapshotCreateXML(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autofree gchar *path = NULL; + g_autoptr(virDomain) domain = NULL; + g_autoptr(virDomainSnapshot) snapshot = NULL; + guint flags; + const gchar *xml; + + g_variant_get(inArgs, "(su)", &xml, &flags);

This would leak `xml`. In order to get pointer to memory and not full copy of the string you need to use "(&su)".

+ + domain = virtDBusDomainGetVirDomain(connect, objectPath, + error);

This fits into single line.

+ if (!domain) + return; + + if (!virtDBusConnectOpen(connect, error)) + return; + + snapshot = virDomainSnapshotCreateXML(domain, xml, flags); + if (!snapshot) + return virtDBusUtilSetLastVirtError(error); + + path = virtDBusUtilBusPathForVirDomainSnapshot(domain, + snapshot, + connect->domainSnapshotPath); + + *outArgs = g_variant_new("(o)", path); +} + +static void +virtDBusDomainSnapshotLookupByName(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autofree gchar *path = NULL; + g_autoptr(virDomain) domain = NULL; + g_autoptr(virDomainSnapshot) snapshot = NULL; + guint flags; + const gchar *name; + + g_variant_get(inArgs, "(su)", &name, &flags);

Same as for the `xml`. You have to use "(&su)" in order to not get pointer for the `name`.

+ + domain = virtDBusDomainGetVirDomain(connect, objectPath, + error);

This fits into single line.

+ if (!domain) + return; + + if (!virtDBusConnectOpen(connect, error)) + return; + + snapshot = virDomainSnapshotLookupByName(domain, name, flags); + if (!snapshot) + return virtDBusUtilSetLastVirtError(error); + + path = virtDBusUtilBusPathForVirDomainSnapshot(domain, + snapshot, + connect->domainSnapshotPath); + + *outArgs = g_variant_new("(o)", path); +} + static void virtDBusDomainSuspend(GVariant *inArgs G_GNUC_UNUSED, GUnixFDList *inFDs G_GNUC_UNUSED, @@ -3095,6 +3249,7 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] = { { "HasManagedSaveImage", virtDBusDomainHasManagedSaveImage }, { "InjectNMI", virtDBusDomainInjectNMI }, { "InterfaceAddresses", virtDBusDomainInterfaceAddresses }, + { "ListDomainSnapshots", virtDBusDomainListDomainSnapshots }, { "ManagedSave", virtDBusDomainManagedSave }, { "ManagedSaveRemove", virtDBusDomainManagedSaveRemove }, { "MemoryPeek", virtDBusDomainMemoryPeek }, @@ -3132,6 +3287,9 @@ static virtDBusGDBusMethodTable virtDBusDomainMethodTable[] = { { "SetSchedulerParameters", virtDBusDomainSetSchedulerParameters }, { "SetTime", virtDBusDomainSetTime }, { "SetUserPassword", virtDBusDomainSetUserPassword }, + { "SnapshotCurrent", virtDBusDomainSnapshotCurrent }, + { "SnapshotCreateXML", virtDBusDomainSnapshotCreateXML }, + { "SnapshotLookupByName", virtDBusDomainSnapshotLookupByName }, { "Shutdown", virtDBusDomainShutdown }, { "Suspend", virtDBusDomainSuspend }, { "Undefine", virtDBusDomainUndefine }, diff --git a/src/domainsnapshot.c b/src/domainsnapshot.c index 590cbef..3f5f5a3 100644 --- a/src/domainsnapshot.c +++ b/src/domainsnapshot.c @@ -1,14 +1,217 @@ #include "domainsnapshot.h" +#include "domain.h" #include "util.h"

#include <libvirt/libvirt.h>

+static void +virtDBusDomainSnapshotDelete(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainSnapshot) domainSnapshot = NULL; + guint flags; + + g_variant_get(inArgs, "(u)", &flags); + + domainSnapshot = virtDBusUtilVirDomainSnapshotFromBusPath(connect->connection, + objectPath, + connect->domainSnapshotPath); + if (!domainSnapshot) + return; + + if (virDomainSnapshotDelete(domainSnapshot, flags) < 0) + return virtDBusUtilSetLastVirtError(error); +} + +static void +virtDBusDomainSnapshotGetParent(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error)

Wrong indentation here.

+{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainSnapshot) domainSnapshot = NULL; + g_autoptr(virDomainSnapshot) parent = NULL; + guint flags; + g_autofree gchar *domainName = NULL; + g_autoptr(virDomain) domain = NULL; + gsize prefixLen = strlen(connect->domainSnapshotPath) + 1; + gchar** strings = g_strsplit(objectPath + prefixLen, "_", 2); + g_autofree gchar *parentPath = NULL; + + g_variant_get(inArgs, "(u)", &flags); + + domainSnapshot = virtDBusUtilVirDomainSnapshotFromBusPath(connect->connection, + objectPath, + connect->domainSnapshotPath); + if (!domainSnapshot) + return; + + parent = virDomainSnapshotGetParent(domainSnapshot, flags); + if (!parent) + return virtDBusUtilSetLastVirtError(error); + + domainName = virtDBusUtilDecodeStr(strings[0]); + domain = virDomainLookupByName(connect->connection, domainName);

Why not simply calling virDomainSnapshotGetDomain here?

+ parentPath = virtDBusUtilBusPathForVirDomainSnapshot(domain, + parent, + connect->domainSnapshotPath); + + *outArgs = g_variant_new("(o)", parentPath); +} + +static void +virtDBusDomainSnapshotGetXMLDesc(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainSnapshot) domainSnapshot = NULL; + guint flags; + g_autofree gchar *xml = NULL; + + g_variant_get(inArgs, "(u)", &flags); + + domainSnapshot = virtDBusUtilVirDomainSnapshotFromBusPath(connect->connection, + objectPath, + connect->domainSnapshotPath); + if (!domainSnapshot) + return; + + xml = virDomainSnapshotGetXMLDesc(domainSnapshot, flags); + if (!xml) + return virtDBusUtilSetLastVirtError(error); + + *outArgs = g_variant_new("(s)", xml); +} + +static void +virtDBusDomainSnapshotIsCurrent(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainSnapshot) domainSnapshot = NULL; + guint flags; + gint isCurrent; + + g_variant_get(inArgs, "(u)", &flags); + + domainSnapshot = virtDBusUtilVirDomainSnapshotFromBusPath(connect->connection, + objectPath, + connect->domainSnapshotPath); + if (!domainSnapshot) + return; + + isCurrent = virDomainSnapshotIsCurrent(domainSnapshot, flags); + if (isCurrent < 0) + return virtDBusUtilSetLastVirtError(error); + + *outArgs = g_variant_new("(b)", !!isCurrent); +} + +static void +virtDBusDomainSnapshotListAllChildren(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainSnapshot) domainSnapshot = NULL; + g_autoptr(virDomainSnapshotPtr) snaps = NULL; + guint flags; + GVariantBuilder builder; + GVariant *gdomainSnapshots; + g_autofree gchar *domainName = NULL; + g_autoptr(virDomain) domain = NULL; + gsize prefixLen = strlen(connect->domainSnapshotPath) + 1; + gchar** strings = g_strsplit(objectPath + prefixLen, "_", 2); + + g_variant_get(inArgs, "(u)", &flags); + + domainSnapshot = virtDBusUtilVirDomainSnapshotFromBusPath(connect->connection, + objectPath, + connect->domainSnapshotPath); + if (!domainSnapshot) + return; + + if (virDomainSnapshotListAllChildren(domainSnapshot, &snaps, flags) < 0) + return virtDBusUtilSetLastVirtError(error); + + g_variant_builder_init(&builder, G_VARIANT_TYPE("ao")); + + domainName = virtDBusUtilDecodeStr(strings[0]); + domain = virDomainLookupByName(connect->connection, domainName);

Again no need to do this magic, you can simply call virDomainSnapshotGetDomain here.

+ for (gint i = 0; snaps[i]; i++) { + g_autofree gchar *path = NULL; + path = virtDBusUtilBusPathForVirDomainSnapshot(domain, + snaps[i], + connect->domainSnapshotPath); + + g_variant_builder_add(&builder, "o", path); + } + + gdomainSnapshots = g_variant_builder_end(&builder); + *outArgs = g_variant_new_tuple(&gdomainSnapshots, 1); +} + +static void +virtDBusDomainSnapshotRevert(GVariant *inArgs, + GUnixFDList *inFDs G_GNUC_UNUSED, + const gchar *objectPath, + gpointer userData, + GVariant **outArgs G_GNUC_UNUSED, + GUnixFDList **outFDs G_GNUC_UNUSED, + GError **error) +{ + virtDBusConnect *connect = userData; + g_autoptr(virDomainSnapshot) domainSnapshot = NULL; + guint flags; + + g_variant_get(inArgs, "(u)", &flags); + + domainSnapshot = virtDBusUtilVirDomainSnapshotFromBusPath(connect->connection, + objectPath, + connect->domainSnapshotPath); + if (!domainSnapshot) + return; + + if (virDomainRevertToSnapshot(domainSnapshot, flags) < 0) + return virtDBusUtilSetLastVirtError(error); +} + static virtDBusGDBusPropertyTable virtDBusDomainSnapshotPropertyTable[] = { { 0 } };

static virtDBusGDBusMethodTable virtDBusDomainSnapshotMethodTable[] = { - { 0 } + { "Delete", virtDBusDomainSnapshotDelete }, + { "GetParent", virtDBusDomainSnapshotGetParent }, + { "GetXMLDesc", virtDBusDomainSnapshotGetXMLDesc }, + /* Needs to be method since it takes 'flags' parameter */

I don't think we need to have this comment, but I'll leave it up to you to keep it or not.

+ { "IsCurrent", virtDBusDomainSnapshotIsCurrent }, + { "ListChildren", virtDBusDomainSnapshotListAllChildren }, + { "Revert", virtDBusDomainSnapshotRevert }, };

static gchar ** diff --git a/tests/libvirttest.py b/tests/libvirttest.py index a442196..8462fc3 100644 --- a/tests/libvirttest.py +++ b/tests/libvirttest.py @@ -112,6 +112,20 @@ class BaseTestClass(): """ return self.node_device_create()

+ @pytest.fixture + def snapshot_create(self): + """ Fixture to create simple snapshot the test driver

This reads weird, there is something missing in that sentence.

+ + This fixture should be used in the setup of every test manipulating + with snaphots. + """ + _, test_domain = self.get_test_domain() + interface_obj = dbus.Interface(test_domain, 'org.libvirt.Domain')

This cannot work as the `test_domain` is already an dbus.Interface object.

+ path = interface_obj.SnapshotCreateXML(xmldata.minimal_snapshot_xml, 0)

Here you should be able to use test_domain.SnapshotCreateXML() directly.

+ obj = self.bus.get_object('org.libvirt', path) + interface_obj = dbus.Interface(obj, 'org.libvirt.DomainSnapshot') + return interface_obj

No need to use variable for this, you can return the interface object directly.

+ @pytest.fixture def storage_volume_create(self): """ Fixture to create dummy storage volume on the test driver diff --git a/tests/meson.build b/tests/meson.build index b6d4bd5..0ad5cdb 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -22,6 +22,7 @@ test('test_util', test_exec, suite: 'unit') python_tests = [ 'test_connect.py', 'test_domain.py', + 'test_snapshot.py', 'test_interface.py', 'test_network.py', 'test_nodedev.py', diff --git a/tests/test_domain.py b/tests/test_domain.py index b5879b4..fdb5aa4 100755 --- a/tests/test_domain.py +++ b/tests/test_domain.py @@ -2,6 +2,7 @@

import dbus import libvirttest +import xmldata

DBUS_EXCEPTION_MISSING_FUNCTION = 'this function is not supported by the connection driver'

@@ -160,6 +161,13 @@ class TestDomain(libvirttest.BaseTestClass): pinInfo = domain.GetVcpuPinInfo(0) assert pinInfo == pinInfo_expected

+ def test_snapshot(self): + obj, domain = self.get_test_domain() + domain.SnapshotCreateXML(xmldata.minimal_snapshot_xml, 0) + assert isinstance(domain.SnapshotCurrent(0), dbus.ObjectPath) + assert isinstance(domain.SnapshotLookupByName("my_snapshot", 0), dbus.ObjectPath) + assert isinstance(domain.ListDomainSnapshots(0), dbus.Array)

if __name__ == '__main__': libvirttest.run() + diff --git a/tests/test_snapshot.py b/tests/test_snapshot.py new file mode 100755 index 0000000..e9cc5b9 --- /dev/null +++ b/tests/test_snapshot.py @@ -0,0 +1,43 @@ +#!/usr/bin/env python3 + +import dbus +import libvirttest +import pytest + +EXCEPTION_NO_PARENT = 'does not have a parent' + +@pytest.mark.usefixtures("snapshot_create") +class TestSnapshot(libvirttest.BaseTestClass): + """ Tests for methods and properties of the Snapshot snapshot + """ + + def test_snapshot_delete(self, snapshot_create): + snapshot_obj = snapshot_create + snapshot_obj.Delete(0) + + def test_snapshot_get_parent(self, snapshot_create): + snapshot_obj = snapshot_create + try: + snapshot_obj.GetParent(0) + except dbus.exceptions.DBusException as e: + if not any(EXCEPTION_NO_PARENT in arg for arg in e.args): + raise e + + def test_snapshot_get_xml(self, snapshot_create): + snapshot_obj = snapshot_create + assert isinstance(snapshot_obj.GetXMLDesc(0), dbus.String) + + def test_snapshot_get_is_current(self, snapshot_create): + snapshot_obj = snapshot_create + assert isinstance(snapshot_obj.IsCurrent(0), dbus.Boolean) + + def test_snapshot_list_children(self, snapshot_create): + snapshot_obj = snapshot_create + assert isinstance(snapshot_obj.ListChildren(0), dbus.Array) + + def test_snapshot_revert(self, snapshot_create): + snapshot_obj = snapshot_create + snapshot_obj.Revert(0) + +if __name__ == '__main__': + libvirttest.run() diff --git a/tests/xmldata.py b/tests/xmldata.py index 8075052..0146ccf 100644 --- a/tests/xmldata.py +++ b/tests/xmldata.py @@ -54,6 +54,12 @@ minimal_node_device_xml = ''' </device> '''

+minimal_snapshot_xml = ''' +<domainsnapshot> + <name>my_snapshot</name> +</domainsnapshot> +''' + minimal_storage_pool_xml = ''' <pool type='dir'> <name>foo</name>

Otherwise looks good. Pavel