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

On Tue, Jun 12, 2018 at 11:00:14AM +0200, Katerina Koukiou wrote: > Signed-off-by: Katerina Koukiou <kkoukiou(a)redhat.com&gt; > --- > 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.

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