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).

+ 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; + } + }

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 >

There, it'd be clearer to diagnose with something like "missing pool source device name", since there are a few other "name" elements.

Not a show-stopper at all, but it'd be great if you could add a unit test or two, so this new code gets some regular coverage.

On Fri, Aug 29, 2008 at 03:49:27PM -0400, David Lively wrote: > Hi Jim - > I've attached a (very) small incremental patch (i.e., to be applied > after the one you've already merged) that addresses a couple things I > noticed missing: > (a) documents the new <source> <name> element in formatstorage.html.in > (b) adds --source-name to the (optional) args for virsh pool-define-as > > I've also attached a new version of the full patch containing this > change, in case that's easier. Okidoc, I finally added this in CVS, i just had to do a bit of porting since the XPath lookup function have an extra argument, but nothing hard. I also changed some of the error message to provide more context because as Jim pointed out they were a bit too generic. thanks a lot ! Daniel

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).

???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. While admittedly it seems a little weird, it does allow more flexibility (pool name not tied to vol group name), and allows <source> to fully specify a logical pool source, as it does for the other pool types[*]. Defaulting the source name from the pool name means all existing pool XML definitions continue to work. Defaulting the pool name from the source name is simply a matter of convenience (but perhaps a step too far?) If this is accepted, logical pool discovery can then return a bunch of sources like: <source><name>VolGroup00</name></source> <source><name>VolGroup01</name></source>

Please note I'll be on vacation next week (Aug 11-15), so I may not be responding until the following week. Thanks, Dave [*] Well ... almost. Note that directory pools have a similar issue -- the "source" of the pool is given by the <target> <path> -- there's no <source>. I suppose implementing directory pool discovery (for the sake of completeness) means addressing this as well, maybe via some similar mechanism ...