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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list