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

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 >