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

[Pavel Hrdina]

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 at 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
-------------- 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/20180614/59a20897/attachment-0001.sig>