Re: [libvirt] [PATCH] introducing source name (for logical storage pools)
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 [EMAIL PROTECTED] 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/
[EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] introducing source name (for logical storage pools)
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/ [EMAIL PROTECTED] | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] introducing source name (for logical storage pools)
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
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));
}
}
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] introducing source name (for logical storage pools)
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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] introducing source name (for logical storage pools)
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
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.
Signed-off-by: David Lively [EMAIL PROTECTED]
diff --git a/src/storage_backend.h b/src/storage_backend.h
index 797ca01..422f26c 100644
--- a/src/storage_backend.h
+++ b/src/storage_backend.h
@@ -53,6 +53,7 @@ enum {
VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE = (11),
VIR_STORAGE_BACKEND_POOL_SOURCE_DIR = (12),
VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER = (13),
+VIR_STORAGE_BACKEND_POOL_SOURCE_NAME= (14),
};
typedef struct _virStorageBackendPoolOptions virStorageBackendPoolOptions;
diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
index 0c4f6a5..382078b 100644
--- a/src/storage_backend_logical.c
+++ b/src/storage_backend_logical.c
@@ -80,7 +80,7 @@ virStorageBackendLogicalSetActive(virConnectPtr conn,
cmdargv[0] = VGCHANGE;
cmdargv[1] = on ? -ay : -an;
-cmdargv[2] = pool-def-name;
+cmdargv[2] = pool-def-source.name;
cmdargv[3] = NULL;
if (virRun(conn, cmdargv, NULL) 0)
@@ -213,7 +213,7 @@ virStorageBackendLogicalFindLVs(virConnectPtr conn,
LVS, --separator, :, --noheadings, --units, b,
--unbuffered, --nosuffix, --options,
lv_name,uuid,devices,seg_size,vg_extent_size,
-pool-def-name, NULL
+pool-def-source.name, NULL
};
int exitstatus;
@@ -288,7 +288,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn,
}
vgargv[n++] = VGCREATE;
-vgargv[n++] = pool-def-name;
+vgargv[n++] = pool-def-source.name;
pvargv[0] = PVCREATE;
pvargv[2] = NULL;
@@ -365,7 +365,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn,
const char *prog[] = {
VGS, --separator, :, --noheadings, --units, b, --unbuffered,
--nosuffix, --options, vg_size,vg_free,
-pool-def-name, NULL
+pool-def-source.name, NULL
};
int exitstatus;
@@ -419,7 +419,7 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn,
unsigned int flags ATTRIBUTE_UNUSED)
{
const char *cmdargv[] = {
-VGREMOVE, -f, pool-def-name, NULL
+VGREMOVE, -f, pool-def-source.name, NULL
};
if (virRun(conn,
Re: [libvirt] [PATCH] introducing source name (for logical storage pools)
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: sourcenameVolGroup00/name/source sourcenameVolGroup01/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 sourcedirectory/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, sourcedirectory/dev/VolGroup00/directory/source sourcedirectory/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 :| -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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/
[EMAIL PROTECTED] | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[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:
sourcenameVolGroup00/name/source
sourcenameVolGroup01/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 ...
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.
Signed-off-by: David Lively [EMAIL PROTECTED]
diff --git a/src/storage_backend.h b/src/storage_backend.h
index 797ca01..422f26c 100644
--- a/src/storage_backend.h
+++ b/src/storage_backend.h
@@ -53,6 +53,7 @@ enum {
VIR_STORAGE_BACKEND_POOL_SOURCE_DEVICE = (11),
VIR_STORAGE_BACKEND_POOL_SOURCE_DIR = (12),
VIR_STORAGE_BACKEND_POOL_SOURCE_ADAPTER = (13),
+VIR_STORAGE_BACKEND_POOL_SOURCE_NAME= (14),
};
typedef struct _virStorageBackendPoolOptions virStorageBackendPoolOptions;
diff --git a/src/storage_backend_logical.c b/src/storage_backend_logical.c
index 0c4f6a5..382078b 100644
--- a/src/storage_backend_logical.c
+++ b/src/storage_backend_logical.c
@@ -80,7 +80,7 @@ virStorageBackendLogicalSetActive(virConnectPtr conn,
cmdargv[0] = VGCHANGE;
cmdargv[1] = on ? -ay : -an;
-cmdargv[2] = pool-def-name;
+cmdargv[2] = pool-def-source.name;
cmdargv[3] = NULL;
if (virRun(conn, cmdargv, NULL) 0)
@@ -213,7 +213,7 @@ virStorageBackendLogicalFindLVs(virConnectPtr conn,
LVS, --separator, :, --noheadings, --units, b,
--unbuffered, --nosuffix, --options,
lv_name,uuid,devices,seg_size,vg_extent_size,
-pool-def-name, NULL
+pool-def-source.name, NULL
};
int exitstatus;
@@ -288,7 +288,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn,
}
vgargv[n++] = VGCREATE;
-vgargv[n++] = pool-def-name;
+vgargv[n++] = pool-def-source.name;
pvargv[0] = PVCREATE;
pvargv[2] = NULL;
@@ -365,7 +365,7 @@ virStorageBackendLogicalRefreshPool(virConnectPtr conn,
const char *prog[] = {
VGS, --separator, :, --noheadings, --units, b, --unbuffered,
--nosuffix, --options, vg_size,vg_free,
-pool-def-name, NULL
+pool-def-source.name, NULL
};
int exitstatus;
@@ -419,7 +419,7 @@ virStorageBackendLogicalDeletePool(virConnectPtr conn,
unsigned int flags ATTRIBUTE_UNUSED)
{
const char *cmdargv[] = {
-VGREMOVE, -f, pool-def-name, NULL
+VGREMOVE, -f, pool-def-source.name, NULL
};
if (virRun(conn, cmdargv, NULL) 0)
@@ -535,6 +535,7 @@ virStorageBackend virStorageBackendLogical = {
.deleteVol = virStorageBackendLogicalDeleteVol,
.poolOptions = {
+.flags = VIR_STORAGE_BACKEND_POOL_SOURCE_NAME,
.formatFromString =
