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 <dlively@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list