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