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
--
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard | virtualization library http://libvirt.org/
veillard(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/