[libvirt] [libvirt-dbus] [PATCH 1/2] Introduce Domain Snapshot Interface
Pavel Hrdina
phrdina at redhat.com
Mon Oct 21 12:40:05 UTC 2019
On Thu, Oct 10, 2019 at 01:43:09PM +0200, Simon Kobyda wrote:
> Signed-off-by: Simon Kobyda <skobyda at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191021/3db49e59/attachment-0001.sig>
More information about the libvir-list
mailing list