[libvirt] [PATCH] introducing <source> <name> (for logical storage pools)
David Lively
dlively at virtualiron.com
Thu Aug 21 14:17:49 UTC 2008
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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: storage-source-name.patch
Type: text/x-patch
Size: 5627 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20080821/61f32102/attachment-0001.bin>
More information about the libvir-list
mailing list