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@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
