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