[libvirt] [PATCH] introducing <source> <name> (for logical storage pools)
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). This patch introduces a new <source> element <name>, which is distinct from the pool name. <name> is used only by the logical storage backend, and represents the volume group name. For convenience and back-compatibility, <source> <name> defaults to the pool name if not specified when the pool is defined, and <pool> <name> defaults to the source name if not specified. There is no requirement that pool and source name be the same, though. While admittedly it seems a little weird, it does allow more flexibility (pool name not tied to vol group name), and allows <source> to fully specify a logical pool source, as it does for the other pool types[*]. Defaulting the source name from the pool name means all existing pool XML definitions continue to work. Defaulting the pool name from the source name is simply a matter of convenience (but perhaps a step too far?) If this is accepted, logical pool discovery can then return a bunch of sources like: <source><name>VolGroup00</name></source> <source><name>VolGroup01</name></source> Please note I'll be on vacation next week (Aug 11-15), so I may not be responding until the following week. Thanks, Dave [*] Well ... almost. Note that directory pools have a similar issue -- the "source" of the pool is given by the <target> <path> -- there's no <source>. I suppose implementing directory pool discovery (for the sake of completeness) means addressing this as well, maybe via some similar mechanism ...
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@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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
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
David Lively <dlively@virtualiron.com> wrote:
Oops - that was against an old base. Sorry. Here's the new one.
Good timing. I was in the process of replying, after having done the merge and add-conn-arg bit.
Also fixed a few other issues ...
ACK. I compared the result of my merge/tweaks and your new patch and see you fixed everything I saw. The only difference was that when I inserted "conn, " and it pushed past the 80-col limit, I split the line: diff --git a/src/storage_conf.c b/src/storage_conf.c index e49f684..74c3f1e 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -328,3 +329,4 @@ virStoragePoolDefParseDoc(virConnectPtr conn, if (options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) { - ret->source.name = virXPathString(conn, "string(/pool/source/name)", ctxt); + ret->source.name = virXPathString(conn, "string(/pool/source/name)", + ctxt); if (ret->source.name == NULL) {
Hi Jim - I've attached a (very) small incremental patch (i.e., to be applied after the one you've already merged) that addresses a couple things I noticed missing: (a) documents the new <source> <name> element in formatstorage.html.in (b) adds --source-name to the (optional) args for virsh pool-define-as I've also attached a new version of the full patch containing this change, in case that's easier. Thanks, Dave On Thu, 2008-08-21 at 20:55 +0200, Jim Meyering wrote:
David Lively <dlively@virtualiron.com> wrote:
Oops - that was against an old base. Sorry. Here's the new one.
Good timing. I was in the process of replying, after having done the merge and add-conn-arg bit.
Also fixed a few other issues ...
ACK. I compared the result of my merge/tweaks and your new patch and see you fixed everything I saw. The only difference was that when I inserted "conn, " and it pushed past the 80-col limit, I split the line:
diff --git a/src/storage_conf.c b/src/storage_conf.c index e49f684..74c3f1e 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -328,3 +329,4 @@ virStoragePoolDefParseDoc(virConnectPtr conn, if (options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) { - ret->source.name = virXPathString(conn, "string(/pool/source/name)", ctxt); + ret->source.name = virXPathString(conn, "string(/pool/source/name)", + ctxt); if (ret->source.name == NULL) {
David Lively <dlively@virtualiron.com> wrote:
I've attached a (very) small incremental patch (i.e., to be applied after the one you've already merged) that addresses a couple things I noticed missing: (a) documents the new <source> <name> element in formatstorage.html.in (b) adds --source-name to the (optional) args for virsh pool-define-as
I've also attached a new version of the full patch containing this change, in case that's easier.
Hi Dave, That looks fine. The only nit I saw was in the context:
diff --git a/src/storage_conf.c b/src/storage_conf.c ... { + ret->name = virXPathString(conn, "string(/pool/name)", ctxt); + if (ret->name == NULL && + options->flags & VIR_STORAGE_BACKEND_POOL_SOURCE_NAME) + ret->name = virXPathString(conn, "string(/pool/source/name)", ctxt); + if (ret->name == NULL) { virStorageReportError(conn, VIR_ERR_XML_ERROR, "%s", _("missing name element"));
There, it'd be clearer to diagnose with something like "missing pool source device name", since there are a few other "name" elements. Not a show-stopper at all, but it'd be great if you could add a unit test or two, so this new code gets some regular coverage.
On Tue, 2008-09-02 at 08:50 +0200, Jim Meyering wrote:
There, it'd be clearer to diagnose with something like "missing pool source device name", since there are a few other "name" elements.
I see Daniel V fixed this (and other) error messages to be less ambiguous when he committed it earlier this morning.
Not a show-stopper at all, but it'd be great if you could add a unit test or two, so this new code gets some regular coverage.
Are there any existing storage pool tests? I can't find them. I should also add some unit tests for the storage pool discovery stuff.
On Fri, Aug 29, 2008 at 03:49:27PM -0400, David Lively wrote:
Hi Jim - I've attached a (very) small incremental patch (i.e., to be applied after the one you've already merged) that addresses a couple things I noticed missing: (a) documents the new <source> <name> element in formatstorage.html.in (b) adds --source-name to the (optional) args for virsh pool-define-as
I've also attached a new version of the full patch containing this change, in case that's easier.
Okidoc, I finally added this in CVS, i just had to do a bit of porting since the XPath lookup function have an extra argument, but nothing hard. I also changed some of the error message to provide more context because as Jim pointed out they were a bit too generic. thanks a lot ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Thanks Daniel. I just merged in your changes. You seem to be missing a small incremental change (checking the strdup return value for NULL), attached. Dave On Tue, 2008-09-02 at 16:17 +0200, Daniel Veillard wrote:
On Fri, Aug 29, 2008 at 03:49:27PM -0400, David Lively wrote:
Hi Jim - I've attached a (very) small incremental patch (i.e., to be applied after the one you've already merged) that addresses a couple things I noticed missing: (a) documents the new <source> <name> element in formatstorage.html.in (b) adds --source-name to the (optional) args for virsh pool-define-as
I've also attached a new version of the full patch containing this change, in case that's easier.
Okidoc, I finally added this in CVS, i just had to do a bit of porting since the XPath lookup function have an extra argument, but nothing hard. I also changed some of the error message to provide more context because as Jim pointed out they were a bit too generic.
thanks a lot !
Daniel
On Tue, Sep 02, 2008 at 11:34:46AM -0400, David Lively wrote:
Thanks Daniel. I just merged in your changes. You seem to be missing a small incremental change (checking the strdup return value for NULL), attached.
yes, mea-culpa ! That what happens when looking a too old mail, but i wanted to be sure your patches would not get lost so i kept them as 'unread' in my mailer, problem is that i didn't realized they were deprecated. Jim pointed that missing piece,
diff --git a/src/storage_conf.c b/src/storage_conf.c index 2f6093b..37a2040 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -331,6 +331,8 @@ virStoragePoolDefParseDoc(virConnectPtr conn, if (ret->source.name == NULL) { /* source name defaults to pool name */ ret->source.name = strdup(ret->name); + if (ret->source.name == NULL) + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("pool name")); } }
Hum, I'm just wondering, shouldn't we go to cleanup too on strdup error instead of continuing there ? sorry for the extra work, and thank you ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel Veillard <veillard@redhat.com> wrote:
On Tue, Sep 02, 2008 at 11:34:46AM -0400, David Lively wrote:
Thanks Daniel. I just merged in your changes. You seem to be missing a small incremental change (checking the strdup return value for NULL), attached.
yes, mea-culpa ! That what happens when looking a too old mail, but i wanted to be sure your patches would not get lost so i kept them as 'unread' in my mailer, problem is that i didn't realized they were deprecated. Jim pointed that missing piece,
diff --git a/src/storage_conf.c b/src/storage_conf.c index 2f6093b..37a2040 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -331,6 +331,8 @@ virStoragePoolDefParseDoc(virConnectPtr conn, if (ret->source.name == NULL) { /* source name defaults to pool name */ ret->source.name = strdup(ret->name); + if (ret->source.name == NULL) + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("pool name")); } }
Hum, I'm just wondering, shouldn't we go to cleanup too on strdup error instead of continuing there ?
You're probably right. However, technically, it looks like having a NULL source.name there is tolerable, since all derefs (at least in that file) first check for non-NULL. But if a small strdup like that fails, I don't see much point in trying to continue. If that's the intent, then it deserves a comment explaining why this failure case is different from most(all?) of the others in the vicinity.
On Tue, 2008-09-02 at 17:54 +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
diff --git a/src/storage_conf.c b/src/storage_conf.c index 2f6093b..37a2040 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -331,6 +331,8 @@ virStoragePoolDefParseDoc(virConnectPtr conn, if (ret->source.name == NULL) { /* source name defaults to pool name */ ret->source.name = strdup(ret->name); + if (ret->source.name == NULL) + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("pool name")); } }
Hum, I'm just wondering, shouldn't we go to cleanup too on strdup error instead of continuing there ?
You're probably right. However, technically, it looks like having a NULL source.name there is tolerable, since all derefs (at least in that file) first check for non-NULL. But if a small strdup like that fails, I don't see much point in trying to continue.
If that's the intent, then it deserves a comment explaining why this failure case is different from most(all?) of the others in the vicinity.
Daniel is right. I meant to cleanup and exit (goto cleanup) in this case ... Thanks, Dave
On Tue, Sep 02, 2008 at 11:59:35AM -0400, David Lively wrote:
On Tue, 2008-09-02 at 17:54 +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
diff --git a/src/storage_conf.c b/src/storage_conf.c index 2f6093b..37a2040 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -331,6 +331,8 @@ virStoragePoolDefParseDoc(virConnectPtr conn, if (ret->source.name == NULL) { /* source name defaults to pool name */ ret->source.name = strdup(ret->name); + if (ret->source.name == NULL) + virStorageReportError(conn, VIR_ERR_NO_MEMORY, "%s", _("pool name")); } }
Hum, I'm just wondering, shouldn't we go to cleanup too on strdup error instead of continuing there ?
You're probably right. However, technically, it looks like having a NULL source.name there is tolerable, since all derefs (at least in that file) first check for non-NULL. But if a small strdup like that fails, I don't see much point in trying to continue.
If that's the intent, then it deserves a comment explaining why this failure case is different from most(all?) of the others in the vicinity.
Daniel is right. I meant to cleanup and exit (goto cleanup) in this case ...
Okay, commited then ! Thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Thu, Aug 21, 2008 at 10:17:49AM -0400, David Lively wrote:
Oops - that was against an old base. Sorry. Here's the new one. Also fixed a few other issues ...
Ok, this gets my vote to commit. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Aug 08, 2008 at 03:17:52PM -0400, David Lively wrote:
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).
Yep, I half-expected that format decision to come back & haunt me :-(
???This patch introduces a new <source> element <name>, which is distinct from the pool name. <name> is used only by the logical storage backend, and represents the volume group name. For convenience and back-compatibility, <source> <name> defaults to the pool name if not specified when the pool is defined, and <pool> <name> defaults to the source name if not specified. There is no requirement that pool and source name be the same, though.
While admittedly it seems a little weird, it does allow more flexibility (pool name not tied to vol group name), and allows <source> to fully specify a logical pool source, as it does for the other pool types[*]. Defaulting the source name from the pool name means all existing pool XML definitions continue to work. Defaulting the pool name from the source name is simply a matter of convenience (but perhaps a step too far?)
If this is accepted, logical pool discovery can then return a bunch of sources like: <source><name>VolGroup00</name></source> <source><name>VolGroup01</name></source>
Please note I'll be on vacation next week (Aug 11-15), so I may not be responding until the following week.
Thanks, Dave
[*] Well ... almost. Note that directory pools have a similar issue -- the "source" of the pool is given by the <target> <path> -- there's no <source>. I suppose implementing directory pool discovery (for the sake of completeness) means addressing this as well, maybe via some similar mechanism ...
We already have a <source><directory>....</directory></source> element we use for NFS exports. I didn't bother with it for directory pools because it'd be duplicating info from the target path, but in retrospect we should use it for directory pools. Perhaps we could even use it for the LVM pools instead of adding a new <name> element - eg, <source><directory>/dev/VolGroup00</directory></source> <source><directory>/dev/VolGroup01</directory></source> I don't feel too strongly about this though - either name or directory in the source would do the job for LVM pools nicely. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, 2008-08-15 at 10:49 +0100, Daniel Berrange wrote:
On Fri, Aug 08, 2008 at 03:17:52PM -0400, David Lively wrote:
[*] Well ... almost. Note that directory pools have a similar issue -- the "source" of the pool is given by the <target> <path> -- there's no <source>. I suppose implementing directory pool discovery (for the sake of completeness) means addressing this as well, maybe via some similar mechanism ...
We already have a <source><directory>....</directory></source> element we use for NFS exports. I didn't bother with it for directory pools because it'd be duplicating info from the target path, but in retrospect we should use it for directory pools.
Sounds good. Perhaps (for back-compatibility) it should default to the target path if not specified? (And maybe target path should default to source directory if it's not specified??)
Perhaps we could even use it for the LVM pools instead of adding a new <name> element - eg,
<source><directory>/dev/VolGroup00</directory></source> <source><directory>/dev/VolGroup01</directory></source>
I don't feel too strongly about this though - either name or directory in the source would do the job for LVM pools nicely.
I don't think source directory makes sense for LVM. The LVM target path is really a target-dependent convention controlled by the local LVM (or udev) configuration. It's not specified anywhere in the LVM metadata on disk (unlike VG name, which is specified). So I think we need to stick with source name. I'll fix the allocation issue Daniel V pointed out and resubmit shortly. Then I'll work these decisions into the storage pool discovery patch and resubmit that. And I'll submit another (small) patch to allow (and properly default) source directory for directory pools. Dave
participants (5)
-
Daniel Berrange -
Daniel P. Berrange
-
Daniel Veillard -
David Lively -
Jim Meyering