Oops - that was against an old base. Sorry. Here's the new one.
Also fixed a few other issues ...
Dave
On Mon, 2008-08-18 at 12:12 -0400, David Lively wrote:
> 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
> >
This 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 <[EMAIL PROTECTED]>
diff --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 0c4f6a5..382078b 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, cmdargv, NULL) < 0)
@@ -213,7 +213,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;
@@ -288,7 +288,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn,
}
vgargv[n++] = VGCREATE;
- vgargv[n++] = pool->def->name;
+ vgargv[n++] = pool->def->source.name;
pvargv[0] = PVCREATE;
pvargv[2] = NULL;
@@ -365,7 +365,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;
@@ -419,7 +419,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, cmdargv, NULL) < 0)
@@ -535,6 +535,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 05b68af..e49f684 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(conn, "string(/pool/name)", ctxt)) == NULL) {
+ ret->name = virXPathString(conn, "string(/pool/name)", ctxt);
+ if (ret->name == NULL &&
+ options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME)
+ ret->name = virXPathString(conn, "string(/pool/source/name)", ctxt);
+ if (ret->name == NULL) {
virStorageReportError(conn, VIR_ERR_XML_ERROR,
"%s", _("missing name element"));
goto cleanup;
@@ -320,6 +325,15 @@ virStoragePoolDefParseDoc(virConnectPtr conn,
goto cleanup;
}
}
+ 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 = strdup(ret->name);
+ if (ret->source.name == NULL)
+ virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("pool name"));
+ }
+ }
authType = virXPathString(conn, "string(/pool/source/auth/@type)", ctxt);
@@ -499,6 +513,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;
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
