Re: [libvirt] [PATCH] introducing <source> <name> (for logical storage pools)

[David Lively <dlively virtualiron com>]

Same patch, resubmitted after fixing allocation issue you pointed out.
Looking more closely, I notice it was leaking when pool/source/name was
specified.  Just added a strdup for the other case (when
pool/source/name defaults to pool/name) and a VIR_FREE in the
destructor.

Dave

On Tue, 2008-08-12 at 05:21 -0400, Daniel Veillard wrote:
> On Fri, Aug 08, 2008 at 03:17:52PM -0400, David Lively wrote:
> > Hi Folks -
> >   
> >   This small patch is a proposed prerequisite for the storage pool
> > discovery patch I submitted last week.
> > 
> >   Daniel B proposed having storage pool discovery return a bunch of XML
> > storage <source> elements, rather than full <pool> elements (which
> > contain "target-dependent" details like the local pool name and device
> > or mount path -- things which don't need to be decided unless/until the
> > pool is defined).  
> > 
> >   I like his suggestion a lot, and I think it addresses the final
> > API-definition problem keeping storage pool discovery out of libvirt.
> > But ... it doesn't quite work for logical storage pools because those
> > are created from the <pool> <name> element (which is the same as the
> > volume group name).
> 
>   I will let Daniel B comment on the pure storage aspects of the patch.
> The patch looks fine to me except for one thing:
> 
> [...]
> > +    if (options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) {
> > +        ret->source.name = virXPathString(conn, "string(/pool/source/name)", ctxt);
> > +        if (ret->source.name == NULL) {
> > +            /* source name defaults to pool name */
> > +            ret->source.name = ret->name;
> > +        }
> > +    }
> >  
> 
>   I'm vary of allocation issues there.
> Basically the patch adds a new string field to the record. But I could not see
> any deallocation addition in the patch, and the field seems to only be 
> set by copying another value. Maybe this is fine now, but if we ever update
> the field in a different way (which I would expect at some point in the code 
> evolution) then we will have a leak.
>   So instead of copying the string pointer directly, I suggest to do a string
> dup and in the freeing part of the record , do the VIR_FREE call for it.
> 
>   Otherwise this looks fine to me
> 
> Daniel
> 

dThis patch introduces a new <source> element <name>, which is distinct
from the pool name.  <name> is used only by the logical storage
backend, and represents the volume group name.  For convenience and
back-compatibility, <source> <name> defaults to the pool name if not
specified when the pool is defined, and <pool> <name> defaults to the
source name if not specified.  There is no requirement that pool and
source name be the same, though.

Signed-off-by: David Lively <dlively virtualiron com>

iff --git a/src/storage_backend.h b/src/storage_backend.h
index 797ca01..422f26c 100644
--- a/src/storage_backend.h
+++ b/src/storage_backend.h
@@ -53,6 +53,7 @@ enum {
     VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE  = (1<<1),
     VIR_STORAGE_BACKEND_POOL_SOURCE_DIR     = (1<<2),
     VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER = (1<<3),
+    VIR_STORAGE_BACKEND_POOL_SOURCE_NAME    = (1<<4),
 };
 
 typedef struct _virStorageBackendPoolOptions virStorageBackendPoolOptions;
diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
index 9a0c27f..8d6dcc7 100644
--- a/src/storage_backend_logical.c
+++ b/src/storage_backend_logical.c
@@ -80,7 +80,7 @@ virStorageBackendLogicalSetActive(virConnectPtr conn,
 
     cmdargv[0] = VGCHANGE;
     cmdargv[1] = on ? "-ay" : "-an";
-    cmdargv[2] = pool->def->name;
+    cmdargv[2] = pool->def->source.name;
     cmdargv[3] = NULL;
 
     if (virRun(conn, (char**)cmdargv, NULL) < 0)
@@ -211,7 +211,7 @@ virStorageBackendLogicalFindLVs(virConnectPtr conn,
         LVS, "--separator", ":", "--noheadings", "--units", "b",
         "--unbuffered", "--nosuffix", "--options",
         "lv_name,uuid,devices,seg_size,vg_extent_size",
-        pool->def->name, NULL
+        pool->def->source.name, NULL
     };
 
     int exitstatus;
@@ -286,7 +286,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn,
     }
 
     vgargv[n++] = VGCREATE;
-    vgargv[n++] = pool->def->name;
+    vgargv[n++] = pool->def->source.name;
 
     pvargv[0] = PVCREATE;
     pvargv[2] = NULL;
@@ -361,7 +361,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn,
     const char *prog[] = {
         VGS, "--separator", ":", "--noheadings", "--units", "b", "--unbuffered",
         "--nosuffix", "--options", "vg_size,vg_free",
-        pool->def->name, NULL
+        pool->def->source.name, NULL
     };
     int exitstatus;
 
@@ -415,7 +415,7 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn,
                                    unsigned int flags ATTRIBUTE_UNUSED)
 {
     const char *cmdargv[] = {
-        VGREMOVE, "-f", pool->def->name, NULL
+        VGREMOVE, "-f", pool->def->source.name, NULL
     };
 
     if (virRun(conn, (char**)cmdargv, NULL) < 0)
@@ -531,6 +531,7 @@ virStorageBackend virStorageBackendLogical = {
     .deleteVol = virStorageBackendLogicalDeleteVol,
 
     .poolOptions = {
+        .flags = VIR_STORAGE_BACKEND_POOL_SOURCE_NAME,
         .formatFromString = virStorageBackendLogicalPoolFormatFromString,
         .formatToString = virStorageBackendLogicalPoolFormatToString,
     },
diff --git a/src/storage_conf.c b/src/storage_conf.c
index e6278ab..7f84bac 100644
--- a/src/storage_conf.c
+++ b/src/storage_conf.c
@@ -96,6 +96,7 @@ virStoragePoolDefFree(virStoragePoolDefPtr def) {
     }
     VIR_FREE(def->source.devices);
     VIR_FREE(def->source.dir);
+    VIR_FREE(def->source.name);
 
     if (def->source.authType == VIR_STORAGE_POOL_AUTH_CHAP) {
         VIR_FREE(def->source.auth.chap.login);
@@ -248,7 +249,11 @@ virStoragePoolDefParseDoc(virConnectPtr conn,
         goto cleanup;
     }
 
-    if ((ret->name = virXPathString("string(/pool/name)", ctxt)) == NULL) {
+    ret->name = virXPathString("string(/pool/name)", ctxt);
+    if (ret->name == NULL &&
+        options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME)
+        ret->name = virXPathString("string(/pool/source/name)", ctxt);
+    if (ret->name == NULL) {
         virStorageReportError(conn, VIR_ERR_XML_ERROR,
                               "%s", _("missing name element"));
         goto cleanup;
@@ -320,6 +325,13 @@ virStoragePoolDefParseDoc(virConnectPtr conn,
             goto cleanup;
         }
     }
+    if (options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) {
+        ret->source.name = virXPathString("string(/pool/source/name)", ctxt);
+        if (ret->source.name == NULL) {
+            /* source name defaults to pool name */
+	    ret->source.name = strdup(ret->name);
+        }
+    }
 
 
     authType = virXPathString("string(/pool/source/auth/@type)", ctxt);
@@ -466,6 +478,9 @@ virStoragePoolDefFormat(virConnectPtr conn,
     if ((options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER) &&
         def->source.adapter)
         virBufferVSprintf(&buf,"    <adapter name='%s'/>\n", def->source.adapter);
+    if ((options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) &&
+        def->source.name)
+        virBufferVSprintf(&buf,"    <name>%s</name>\n", def->source.name);
 
     if (options->formatToString) {
         const char *format = (options->formatToString)(conn, def->source.format);
diff --git a/src/storage_conf.h b/src/storage_conf.h
index e3577b5..3500039 100644
--- a/src/storage_conf.h
+++ b/src/storage_conf.h
@@ -174,6 +174,9 @@ struct _virStoragePoolSource {
     /* Or an adapter */
     char *adapter;
 
+    /* Or a name */
+    char *name;
+
     int authType;       /* virStoragePoolAuthType */
     union {
         virStoragePoolAuthChap chap;