[libvirt] [dbus PATCH 01/15] Introduce StorageVol Interface

Katerina Koukiou posted 15 patches 6 years, 11 months ago
There is a newer version of this series
[libvirt] [dbus PATCH 01/15] Introduce StorageVol Interface
Posted by Katerina Koukiou 6 years, 11 months ago
Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
---
 data/Makefile.am                |  3 +-
 data/org.libvirt.StorageVol.xml |  7 +++
 src/Makefile.am                 |  3 +-
 src/connect.c                   |  6 +++
 src/connect.h                   |  1 +
 src/storagevol.c                | 96 +++++++++++++++++++++++++++++++++++++++++
 src/storagevol.h                |  9 ++++
 src/util.c                      | 35 +++++++++++++++
 src/util.h                      | 16 +++++++
 9 files changed, 174 insertions(+), 2 deletions(-)
 create mode 100644 data/org.libvirt.StorageVol.xml
 create mode 100644 src/storagevol.c
 create mode 100644 src/storagevol.h

diff --git a/data/Makefile.am b/data/Makefile.am
index fdec857..5688cab 100644
--- a/data/Makefile.am
+++ b/data/Makefile.am
@@ -24,7 +24,8 @@ interfaces_files = \
 	org.libvirt.Network.xml \
         org.libvirt.NWFilter.xml \
 	org.libvirt.Secret.xml \
-	org.libvirt.StoragePool.xml
+	org.libvirt.StoragePool.xml \
+	org.libvirt.StorageVol.xml
 interfacesdir = $(DBUS_INTERFACES_DIR)
 interfaces_DATA = $(interfaces_files)
 
diff --git a/data/org.libvirt.StorageVol.xml b/data/org.libvirt.StorageVol.xml
new file mode 100644
index 0000000..c72c847
--- /dev/null
+++ b/data/org.libvirt.StorageVol.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/storagevol">
+  <interface name="org.libvirt.StorageVol">
+  </interface>
+</node>
diff --git a/src/Makefile.am b/src/Makefile.am
index 22128c2..539e722 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -12,7 +12,8 @@ DAEMON_SOURCES = \
 	network.c network.h \
 	nwfilter.c nwfilter.h \
 	secret.c secret.h \
-	storagepool.c storagepool.h
+	storagepool.c storagepool.h \
+	storagevol.c storagevol.h
 
 EXTRA_DIST = \
 	$(DAEMON_SOURCES)
diff --git a/src/connect.c b/src/connect.c
index 75ae1cd..1090e3e 100644
--- a/src/connect.c
+++ b/src/connect.c
@@ -5,6 +5,7 @@
 #include "nwfilter.h"
 #include "secret.h"
 #include "storagepool.h"
+#include "storagevol.h"
 #include "util.h"
 
 #include <gio/gunixfdlist.h>
@@ -1612,6 +1613,7 @@ virtDBusConnectFree(virtDBusConnect *connect)
     g_free(connect->nwfilterPath);
     g_free(connect->secretPath);
     g_free(connect->storagePoolPath);
+    g_free(connect->storageVolPath);
     g_free(connect);
 }
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virtDBusConnect, virtDBusConnectFree);
@@ -1679,6 +1681,10 @@ virtDBusConnectNew(virtDBusConnect **connectp,
     if (error && *error)
         return;
 
+    virtDBusStorageVolRegister(connect, error);
+    if (error && *error)
+        return;
+
     *connectp = connect;
     connect = NULL;
 }
diff --git a/src/connect.h b/src/connect.h
index fe672ed..341dfc4 100644
--- a/src/connect.h
+++ b/src/connect.h
@@ -17,6 +17,7 @@ struct virtDBusConnect {
     gchar *nwfilterPath;
     gchar *secretPath;
     gchar *storagePoolPath;
+    gchar *storageVolPath;
     virConnectPtr connection;
     GMutex lock;
 
diff --git a/src/storagevol.c b/src/storagevol.c
new file mode 100644
index 0000000..dad7d11
--- /dev/null
+++ b/src/storagevol.c
@@ -0,0 +1,96 @@
+#include "storagevol.h"
+#include "util.h"
+
+#include <libvirt/libvirt.h>
+
+static virtDBusGDBusPropertyTable virtDBusStorageVolPropertyTable[] = {
+    { 0 }
+};
+
+static virtDBusGDBusMethodTable virtDBusStorageVolMethodTable[] = {
+    { 0 }
+};
+
+static gchar **
+virtDBusStorageVolEnumerate(gpointer userData)
+{
+    virtDBusConnect *connect = userData;
+    g_autoptr(virStoragePoolPtr) storagePools = NULL;
+    gint numPools = 0;
+    gint numVols = 0;
+    gint numVolsTotal = 0;
+    gint offset = 0;
+    gchar **ret = NULL;
+
+    if (!virtDBusConnectOpen(connect, NULL))
+        return NULL;
+
+    numPools = virConnectListAllStoragePools(connect->connection,
+                                             &storagePools, 0);
+    if (numPools < 0)
+        return NULL;
+
+    if (numPools == 0)
+        return NULL;
+
+    for (gint i = 0; i < numPools; i++) {
+        g_autoptr(virStorageVolPtr) storageVols = NULL;
+
+        numVols = virStoragePoolListAllVolumes(storagePools[i],
+                                               &storageVols, 0);
+        if (numVols < 0)
+            return NULL;
+
+        if (numVols == 0)
+            return NULL;
+
+        numVolsTotal += numVols;
+    }
+
+    ret = g_new0(gchar *, numVolsTotal + 1);
+
+    for (gint i = 0; i < numPools; i++) {
+        g_autoptr(virStorageVolPtr) storageVols = NULL;
+
+        numVols = virStoragePoolListAllVolumes(storagePools[i],
+                                               &storageVols, 0);
+        if (numVols < 0)
+            return NULL;
+
+        if (numVols == 0)
+            return NULL;
+
+        for (gint j = 0; j < numVols; j++) {
+            ret[offset] = virtDBusUtilBusPathForVirStorageVol(storageVols[j],
+                                                              connect->storageVolPath);
+            offset++;
+        }
+    }
+
+    return ret;
+}
+
+static GDBusInterfaceInfo *interfaceInfo;
+
+void
+virtDBusStorageVolRegister(virtDBusConnect *connect,
+                           GError **error)
+{
+    connect->storageVolPath = g_strdup_printf("%s/storagevol",
+                                              connect->connectPath);
+
+    if (!interfaceInfo) {
+        interfaceInfo = virtDBusGDBusLoadIntrospectData(VIRT_DBUS_STORAGEVOL_INTERFACE,
+                                                        error);
+        if (!interfaceInfo)
+            return;
+    }
+
+    virtDBusGDBusRegisterSubtree(connect->bus,
+                                 connect->storageVolPath,
+                                 interfaceInfo,
+                                 virtDBusStorageVolEnumerate,
+                                 virtDBusStorageVolMethodTable,
+                                 virtDBusStorageVolPropertyTable,
+                                 connect);
+}
diff --git a/src/storagevol.h b/src/storagevol.h
new file mode 100644
index 0000000..e2a893b
--- /dev/null
+++ b/src/storagevol.h
@@ -0,0 +1,9 @@
+#pragma once
+
+#include "connect.h"
+
+#define VIRT_DBUS_STORAGEVOL_INTERFACE "org.libvirt.StorageVol"
+
+void
+virtDBusStorageVolRegister(virtDBusConnect *connect,
+                           GError **error);
diff --git a/src/util.c b/src/util.c
index 1268736..1d11ce6 100644
--- a/src/util.c
+++ b/src/util.c
@@ -417,3 +417,38 @@ virtDBusUtilVirStoragePoolListFree(virStoragePoolPtr *storagePools)
 
     g_free(storagePools);
 }
+
+virStorageVolPtr
+virtDBusUtilVirStorageVolFromBusPath(virConnectPtr connection,
+                                     const gchar *path,
+                                     const gchar *storageVolPath)
+{
+    g_autofree gchar *key = NULL;
+    gsize prefixLen = strlen(storageVolPath);
+
+    key = virtDBusUtilDecodeStr(path + prefixLen + 1);
+
+    return virStorageVolLookupByKey(connection, key);
+}
+
+gchar *
+virtDBusUtilBusPathForVirStorageVol(virStorageVolPtr storageVol,
+                                    const gchar *storageVolPath)
+{
+    const gchar *key = NULL;
+    g_autofree const gchar *encodedKey = NULL;
+
+    key = virStorageVolGetKey(storageVol);
+    encodedKey = virtDBusUtilEncodeStr(key);
+
+    return g_strdup_printf("%s/%s", storageVolPath, encodedKey);
+}
+
+void
+virtDBusUtilVirStorageVolListFree(virStorageVolPtr *storageVols)
+{
+    for (gint i = 0; storageVols[i] != NULL; i++)
+        virStorageVolFree(storageVols[i]);
+
+    g_free(storageVols);
+}
diff --git a/src/util.h b/src/util.h
index 56e0409..bf08d5d 100644
--- a/src/util.h
+++ b/src/util.h
@@ -133,3 +133,19 @@ virtDBusUtilVirStoragePoolListFree(virStoragePoolPtr *storagePools);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStoragePool, virStoragePoolFree);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStoragePoolPtr,
                               virtDBusUtilVirStoragePoolListFree);
+
+virStorageVolPtr
+virtDBusUtilVirStorageVolFromBusPath(virConnectPtr connection,
+                                     const gchar *path,
+                                     const gchar *storageVolPath);
+
+gchar *
+virtDBusUtilBusPathForVirStorageVol(virStorageVolPtr storageVol,
+                                    const gchar *storageVolPath);
+
+void
+virtDBusUtilVirStorageVolListFree(virStorageVolPtr *storageVols);
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageVol, virStorageVolFree);
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virStorageVolPtr,
+                              virtDBusUtilVirStorageVolListFree);
-- 
2.15.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 01/15] Introduce StorageVol Interface
Posted by Ján Tomko 6 years, 11 months ago
On Tue, Jun 12, 2018 at 11:00:14AM +0200, Katerina Koukiou wrote:
>Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
>---
> data/Makefile.am                |  3 +-
> data/org.libvirt.StorageVol.xml |  7 +++
> src/Makefile.am                 |  3 +-
> src/connect.c                   |  6 +++
> src/connect.h                   |  1 +
> src/storagevol.c                | 96 +++++++++++++++++++++++++++++++++++++++++
> src/storagevol.h                |  9 ++++
> src/util.c                      | 35 +++++++++++++++
> src/util.h                      | 16 +++++++
> 9 files changed, 174 insertions(+), 2 deletions(-)
> create mode 100644 data/org.libvirt.StorageVol.xml
> create mode 100644 src/storagevol.c
> create mode 100644 src/storagevol.h
>
>diff --git a/data/Makefile.am b/data/Makefile.am
>index fdec857..5688cab 100644
>--- a/data/Makefile.am
>+++ b/data/Makefile.am
>@@ -24,7 +24,8 @@ interfaces_files = \
> 	org.libvirt.Network.xml \
>         org.libvirt.NWFilter.xml \
> 	org.libvirt.Secret.xml \
>-	org.libvirt.StoragePool.xml
>+	org.libvirt.StoragePool.xml \
>+	org.libvirt.StorageVol.xml

org.libvirt.StorageVol.xml \
$(NULL)

That way all the future additions can include the trailing slash,
making for nicer diffs.

> interfacesdir = $(DBUS_INTERFACES_DIR)
> interfaces_DATA = $(interfaces_files)
>

>diff --git a/src/Makefile.am b/src/Makefile.am
>index 22128c2..539e722 100644
>--- a/src/Makefile.am
>+++ b/src/Makefile.am
>@@ -12,7 +12,8 @@ DAEMON_SOURCES = \
> 	network.c network.h \
> 	nwfilter.c nwfilter.h \
> 	secret.c secret.h \
>-	storagepool.c storagepool.h
>+	storagepool.c storagepool.h \
>+	storagevol.c storagevol.h

$(NULL) here as well

>
> EXTRA_DIST = \
> 	$(DAEMON_SOURCES)

>diff --git a/src/storagevol.c b/src/storagevol.c
>new file mode 100644
>index 0000000..dad7d11
>--- /dev/null
>+++ b/src/storagevol.c
>@@ -0,0 +1,96 @@
>+#include "storagevol.h"
>+#include "util.h"
>+
>+#include <libvirt/libvirt.h>
>+
>+static virtDBusGDBusPropertyTable virtDBusStorageVolPropertyTable[] = {
>+    { 0 }
>+};
>+
>+static virtDBusGDBusMethodTable virtDBusStorageVolMethodTable[] = {
>+    { 0 }
>+};
>+
>+static gchar **
>+virtDBusStorageVolEnumerate(gpointer userData)
>+{
>+    virtDBusConnect *connect = userData;
>+    g_autoptr(virStoragePoolPtr) storagePools = NULL;
>+    gint numPools = 0;
>+    gint numVols = 0;
>+    gint numVolsTotal = 0;
>+    gint offset = 0;
>+    gchar **ret = NULL;
>+
>+    if (!virtDBusConnectOpen(connect, NULL))
>+        return NULL;
>+
>+    numPools = virConnectListAllStoragePools(connect->connection,
>+                                             &storagePools, 0);
>+    if (numPools < 0)
>+        return NULL;
>+
>+    if (numPools == 0)
>+        return NULL;
>+
>+    for (gint i = 0; i < numPools; i++) {
>+        g_autoptr(virStorageVolPtr) storageVols = NULL;
>+
>+        numVols = virStoragePoolListAllVolumes(storagePools[i],
>+                                               &storageVols, 0);
>+        if (numVols < 0)
>+            return NULL;
>+
>+        if (numVols == 0)
>+            return NULL;
>+
>+        numVolsTotal += numVols;
>+    }
>+
>+    ret = g_new0(gchar *, numVolsTotal + 1);
>+
>+    for (gint i = 0; i < numPools; i++) {
>+        g_autoptr(virStorageVolPtr) storageVols = NULL;
>+
>+        numVols = virStoragePoolListAllVolumes(storagePools[i],
>+                                               &storageVols, 0);

We don't need to list the volumes twice, not only it seems inefficient,
the number of volumes can change in the meantime, leading to possible
invalid memory access.

Possibly an APPEND_ELEMENT macro similar to what we have in libvirt,
so that we can test it separately in test_util.c.

>+        if (numVols < 0)
>+            return NULL;

This would leak ret.

>+
>+        if (numVols == 0)
>+            return NULL;
>+
>+        for (gint j = 0; j < numVols; j++) {
>+            ret[offset] = virtDBusUtilBusPathForVirStorageVol(storageVols[j],
>+                                                              connect->storageVolPath);
>+            offset++;
>+        }
>+    }
>+
>+    return ret;
>+}
>+

>diff --git a/src/util.c b/src/util.c
>index 1268736..1d11ce6 100644
>--- a/src/util.c
>+++ b/src/util.c
>@@ -417,3 +417,38 @@ virtDBusUtilVirStoragePoolListFree(virStoragePoolPtr *storagePools)
>
>     g_free(storagePools);
> }
>+
>+virStorageVolPtr
>+virtDBusUtilVirStorageVolFromBusPath(virConnectPtr connection,
>+                                     const gchar *path,
>+                                     const gchar *storageVolPath)
>+{
>+    g_autofree gchar *key = NULL;
>+    gsize prefixLen = strlen(storageVolPath);
>+
>+    key = virtDBusUtilDecodeStr(path + prefixLen + 1);

In other cases, the + 1 is a part of the prefixLen.

>+
>+    return virStorageVolLookupByKey(connection, key);
>+}
>+

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [dbus PATCH 01/15] Introduce StorageVol Interface
Posted by Pavel Hrdina 6 years, 11 months ago
On Wed, Jun 13, 2018 at 04:21:33PM +0200, Ján Tomko wrote:
> On Tue, Jun 12, 2018 at 11:00:14AM +0200, Katerina Koukiou wrote:
> > Signed-off-by: Katerina Koukiou <kkoukiou@redhat.com>
> > ---
> > data/Makefile.am                |  3 +-
> > data/org.libvirt.StorageVol.xml |  7 +++
> > src/Makefile.am                 |  3 +-
> > src/connect.c                   |  6 +++
> > src/connect.h                   |  1 +
> > src/storagevol.c                | 96 +++++++++++++++++++++++++++++++++++++++++
> > src/storagevol.h                |  9 ++++
> > src/util.c                      | 35 +++++++++++++++
> > src/util.h                      | 16 +++++++
> > 9 files changed, 174 insertions(+), 2 deletions(-)
> > create mode 100644 data/org.libvirt.StorageVol.xml
> > create mode 100644 src/storagevol.c
> > create mode 100644 src/storagevol.h

[...]

> > diff --git a/src/storagevol.c b/src/storagevol.c
> > new file mode 100644
> > index 0000000..dad7d11
> > --- /dev/null
> > +++ b/src/storagevol.c
> > @@ -0,0 +1,96 @@
> > +#include "storagevol.h"
> > +#include "util.h"
> > +
> > +#include <libvirt/libvirt.h>
> > +
> > +static virtDBusGDBusPropertyTable virtDBusStorageVolPropertyTable[] = {
> > +    { 0 }
> > +};
> > +
> > +static virtDBusGDBusMethodTable virtDBusStorageVolMethodTable[] = {
> > +    { 0 }
> > +};
> > +
> > +static gchar **
> > +virtDBusStorageVolEnumerate(gpointer userData)
> > +{
> > +    virtDBusConnect *connect = userData;
> > +    g_autoptr(virStoragePoolPtr) storagePools = NULL;
> > +    gint numPools = 0;
> > +    gint numVols = 0;
> > +    gint numVolsTotal = 0;
> > +    gint offset = 0;
> > +    gchar **ret = NULL;
> > +
> > +    if (!virtDBusConnectOpen(connect, NULL))
> > +        return NULL;
> > +
> > +    numPools = virConnectListAllStoragePools(connect->connection,
> > +                                             &storagePools, 0);
> > +    if (numPools < 0)
> > +        return NULL;
> > +
> > +    if (numPools == 0)
> > +        return NULL;
> > +
> > +    for (gint i = 0; i < numPools; i++) {
> > +        g_autoptr(virStorageVolPtr) storageVols = NULL;
> > +
> > +        numVols = virStoragePoolListAllVolumes(storagePools[i],
> > +                                               &storageVols, 0);
> > +        if (numVols < 0)
> > +            return NULL;
> > +
> > +        if (numVols == 0)
> > +            return NULL;
> > +
> > +        numVolsTotal += numVols;
> > +    }
> > +
> > +    ret = g_new0(gchar *, numVolsTotal + 1);
> > +
> > +    for (gint i = 0; i < numPools; i++) {
> > +        g_autoptr(virStorageVolPtr) storageVols = NULL;
> > +
> > +        numVols = virStoragePoolListAllVolumes(storagePools[i],
> > +                                               &storageVols, 0);
> 
> We don't need to list the volumes twice, not only it seems inefficient,
> the number of volumes can change in the meantime, leading to possible
> invalid memory access.
> 
> Possibly an APPEND_ELEMENT macro similar to what we have in libvirt,
> so that we can test it separately in test_util.c.

Or we can use GPtrArray.

> 
> > +        if (numVols < 0)
> > +            return NULL;
> 
> This would leak ret.
> 
> > +
> > +        if (numVols == 0)
> > +            return NULL;

I thing that in both cases we should call 'continue' instead to use
best-effort approach.

The loop can look like this:

    list = g_ptr_array_new();

    for (gint i = 0; i < numPools; i++) {
        g_autoptr(virStorageVolPtr) storageVols = NULL;
        gint numVols;

        numVols = virStoragePoolListAllVolumes(storagePools[i],
                                               &storageVols, 0);
        if (numVols <= 0)
            continue;

        for (gint j = 0; j < numVols; j++) {
            gchar *volPath = virtDBusUtilBusPathForVirStorageVol(storageVols[j],
                                                                 connect->storageVolPath);
            g_ptr_array_add(list, volPath);
        }
    }

    if (list->len > 0)
        g_ptr_array_add(list, NULL);

    return (gchar **)g_ptr_array_free(list, FALSE);

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