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

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