[libvirt] [PATCH v2 02/12] storage: Introduce virStoragePoolObjNew

[Michal Privoznik]

On 08/24/2017 03:08 PM, John Ferlan wrote:
> Create/use a helper to perform object allocation.
> 
> Adjust storagevolxml2argvtest.c in order to use the allocator and
> setting of the obj->def.
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/virstorageobj.c       | 31 +++++++++++++++++++++----------
>  src/conf/virstorageobj.h       |  3 +++
>  src/libvirt_private.syms       |  1 +
>  tests/storagevolxml2argvtest.c | 20 +++++++++++---------
>  4 files changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c
> index ebda9fe..eb7664c 100644
> --- a/src/conf/virstorageobj.c
> +++ b/src/conf/virstorageobj.c
> @@ -37,6 +37,26 @@
>  VIR_LOG_INIT("conf.virstorageobj");
>  
>  
> +virStoragePoolObjPtr
> +virStoragePoolObjNew(void)
> +{
> +    virStoragePoolObjPtr obj;
> +
> +    if (VIR_ALLOC(obj) < 0)
> +        return NULL;
> +
> +    if (virMutexInit(&obj->lock) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("cannot initialize mutex"));
> +        VIR_FREE(obj);
> +        return NULL;
> +    }
> +    virStoragePoolObjLock(obj);
> +    obj->active = 0;
> +    return obj;
> +}
> +
> +
>  virStoragePoolDefPtr
>  virStoragePoolObjGetDef(virStoragePoolObjPtr obj)
>  {
> @@ -421,17 +441,8 @@ virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools,
>          return obj;
>      }
>  
> -    if (VIR_ALLOC(obj) < 0)
> -        return NULL;
> -
> -    if (virMutexInit(&obj->lock) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                       _("cannot initialize mutex"));
> -        VIR_FREE(obj);
> +    if (!(obj = virStoragePoolObjNew()))
>          return NULL;
> -    }
> -    virStoragePoolObjLock(obj);
> -    obj->active = 0;
>  
>      if (VIR_APPEND_ELEMENT_COPY(pools->objs, pools->count, obj) < 0) {
>          virStoragePoolObjUnlock(obj);
> diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h
> index a5ed8f8..401c4d5 100644
> --- a/src/conf/virstorageobj.h
> +++ b/src/conf/virstorageobj.h
> @@ -70,6 +70,9 @@ typedef bool
>  (*virStoragePoolObjListFilter)(virConnectPtr conn,
>                                 virStoragePoolDefPtr def);
>  
> +virStoragePoolObjPtr
> +virStoragePoolObjNew(void);
> +
>  virStoragePoolDefPtr
>  virStoragePoolObjGetDef(virStoragePoolObjPtr obj);
>  
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 8bb1cbc..fc884fa 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1071,6 +1071,7 @@ virStoragePoolObjListFree;
>  virStoragePoolObjLoadAllConfigs;
>  virStoragePoolObjLoadAllState;
>  virStoragePoolObjLock;
> +virStoragePoolObjNew;
>  virStoragePoolObjNumOfStoragePools;
>  virStoragePoolObjNumOfVolumes;
>  virStoragePoolObjRemove;
> diff --git a/tests/storagevolxml2argvtest.c b/tests/storagevolxml2argvtest.c
> index 9e43204..2ee3dba 100644
> --- a/tests/storagevolxml2argvtest.c
> +++ b/tests/storagevolxml2argvtest.c
> @@ -50,18 +50,19 @@ testCompareXMLToArgvFiles(bool shouldFail,
>      virConnectPtr conn;
>  
>      virStorageVolDefPtr vol = NULL, inputvol = NULL;
> -    virStoragePoolDefPtr pool = NULL;
> +    virStoragePoolDefPtr def = NULL;
>      virStoragePoolDefPtr inputpool = NULL;
> -    virStoragePoolObj poolobj = {.def = NULL };
> -
> +    virStoragePoolObjPtr obj = NULL;
>  
>      if (!(conn = virGetConnect()))
>          goto cleanup;
>  
> -    if (!(pool = virStoragePoolDefParseFile(poolxml)))
> +    if (!(def = virStoragePoolDefParseFile(poolxml)))
>          goto cleanup;
>  
> -    poolobj.def = pool;
> +    if (!(obj = virStoragePoolObjNew()))
> +        goto cleanup;
> +    virStoragePoolObjSetDef(obj, def);
>  
>      if (inputpoolxml) {
>          if (!(inputpool = virStoragePoolDefParseFile(inputpoolxml)))
> @@ -71,17 +72,17 @@ testCompareXMLToArgvFiles(bool shouldFail,
>      if (inputvolxml)
>          parse_flags |= VIR_VOL_XML_PARSE_NO_CAPACITY;
>  
> -    if (!(vol = virStorageVolDefParseFile(pool, volxml, parse_flags)))
> +    if (!(vol = virStorageVolDefParseFile(def, volxml, parse_flags)))
>          goto cleanup;
>  
>      if (inputvolxml &&
>          !(inputvol = virStorageVolDefParseFile(inputpool, inputvolxml, 0)))
>          goto cleanup;
>  
> -    testSetVolumeType(vol, pool);
> +    testSetVolumeType(vol, def);
>      testSetVolumeType(inputvol, inputpool);
>  
> -    cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, &poolobj, vol,
> +    cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, obj, vol,
>                                                     inputvol, flags,
>                                                     create_tool, imgformat,
>                                                     NULL);
> @@ -102,12 +103,13 @@ testCompareXMLToArgvFiles(bool shouldFail,
>      ret = 0;
>  
>   cleanup:
> -    virStoragePoolDefFree(pool);
> +    virStoragePoolDefFree(def);
>      virStoragePoolDefFree(inputpool);
>      virStorageVolDefFree(vol);
>      virStorageVolDefFree(inputvol);
>      virCommandFree(cmd);
>      VIR_FREE(actualCmdline);
> +    virStoragePoolObjUnlock(obj);

Almost. Firstly, @def is now part of @obj so it shouldn't be freed
separately. Secondly, @obj is leaked. ACK if you squash this in:

diff --git i/tests/Makefile.am w/tests/Makefile.am
index 1f8c4cd42..813888575 100644
--- i/tests/Makefile.am
+++ w/tests/Makefile.am
@@ -885,7 +885,10 @@ storagevolxml2argvtest_SOURCES = \
     testutils.c testutils.h
 storagevolxml2argvtest_LDADD = \
        $(LIBXML_LIBS) \
-       ../src/libvirt_driver_storage_impl.la $(LDADDS)
+       ../src/libvirt_driver_storage_impl.la \
+       ../src/libvirt_conf.la \
+       ../src/libvirt_util.la \
+       $(LDADDS)
 
 else ! WITH_STORAGE
 EXTRA_DIST += storagevolxml2argvtest.c
diff --git i/tests/storagevolxml2argvtest.c w/tests/storagevolxml2argvtest.c
index 2ee3dbad9..1b3003216 100644
--- i/tests/storagevolxml2argvtest.c
+++ w/tests/storagevolxml2argvtest.c
@@ -60,8 +60,10 @@ testCompareXMLToArgvFiles(bool shouldFail,
     if (!(def = virStoragePoolDefParseFile(poolxml)))
         goto cleanup;
 
-    if (!(obj = virStoragePoolObjNew()))
+    if (!(obj = virStoragePoolObjNew())) {
+        virStoragePoolDefFree(def);
         goto cleanup;
+    }
     virStoragePoolObjSetDef(obj, def);
 
     if (inputpoolxml) {
@@ -103,13 +105,13 @@ testCompareXMLToArgvFiles(bool shouldFail,
     ret = 0;
 
  cleanup:
-    virStoragePoolDefFree(def);
     virStoragePoolDefFree(inputpool);
     virStorageVolDefFree(vol);
     virStorageVolDefFree(inputvol);
     virCommandFree(cmd);
     VIR_FREE(actualCmdline);
     virStoragePoolObjUnlock(obj);
+    virStoragePoolObjFree(obj);
     virObjectUnref(conn);
     return ret;
 }


Alternatively, you can leave the virStoragePoolDefFree() call as is and
just set @def = NULL right after virStoragePoolObjSetDef(). I'll leave it
up to you which one you prefer.

Michal