12:44 p.m.
Hi all,
I wrote a patch to add support for listing the Vendor and Model of a
storage pool in the storage pool XML. This would allow vendor
extensions of specific devices. The patch includes a test for the new
attributes as well. I'd appreciate some feedback on it, and would like
get it merged when it's ready. Patch added at the bottom of the message.
Best,
Patrick Dignan
diff -Naurb libvirt-0.8.2.mod/docs/schemas/storagepool.rng
libvirt-0.8.2.ml/docs/schemas/storagepool.rng
--- libvirt-0.8.2.mod/docs/schemas/storagepool.rng 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/docs/schemas/storagepool.rng 2010-07-30
17:19:32.835531455 -0500
@@ -8,6 +8,10 @@
<define name='pool'>
<element name='pool'>
+ <optional>
+ <ref name='poolvendor'/>
+ <ref name='poolmodel'/>
+ </optional>
<choice>
<ref name='pooldir'/>
<ref name='poolfs'/>
@@ -103,6 +107,18 @@
<ref name='target'/>
</define>
+ <define name='poolvendor'>
+ <attribute name='vendor'>
+ <text/>
+ </attribute>
+ </define>
+
+ <define name='poolmodel'>
+ <attribute name='model'>
+ <text/>
+ </attribute>
+ </define>
+
<define name='commonmetadata'>
<element name='name'>
<ref name='name'/>
diff -Naurb libvirt-0.8.2.mod/include/libvirt/libvirt.h
libvirt-0.8.2.ml/include/libvirt/libvirt.h
--- libvirt-0.8.2.mod/include/libvirt/libvirt.h 2010-07-22
10:26:05.000000000 -0500
+++ libvirt-0.8.2.ml/include/libvirt/libvirt.h 2010-07-30
11:56:48.726415607 -0500
@@ -1141,6 +1141,8 @@
typedef struct _virStoragePoolInfo virStoragePoolInfo;
struct _virStoragePoolInfo {
+ char *vendor;
+ char *model;
int state; /* virStoragePoolState flags */
unsigned long long capacity; /* Logical size bytes */
unsigned long long allocation; /* Current allocation bytes */
diff -Naurb libvirt-0.8.2.mod/src/conf/storage_conf.c
libvirt-0.8.2.ml/src/conf/storage_conf.c
--- libvirt-0.8.2.mod/src/conf/storage_conf.c 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/src/conf/storage_conf.c 2010-08-02
11:52:48.618533010 -0500
@@ -298,6 +298,12 @@
VIR_FREE(def->name);
+ if (def->vendor != "generic")
+ VIR_FREE(def->vendor);
+
+ if (def->model != "generic")
+ VIR_FREE(def->model);
+
virStoragePoolSourceFree(&def->source);
VIR_FREE(def->target.path);
@@ -601,6 +607,8 @@
virStoragePoolDefPtr ret;
xmlNodePtr source_node;
char *type = NULL;
+ char *vendor = NULL;
+ char *model = NULL;
char *uuid = NULL;
char *tmppath;
@@ -616,6 +624,22 @@
goto cleanup;
}
+ /* Get the vendor and model from the XML and set them in the struct */
+ vendor = virXPathString("string(./@vendor)", ctxt);
+ model = virXPathString("string(./@model)", ctxt);
+
+ if (vendor != NULL) {
+ ret->vendor = vendor;
+ } else {
+ ret->vendor = "generic";
+ }
+
+ if (model != NULL) {
+ ret->model = model;
+ } else {
+ ret->model = "generic";
+ }
+
xmlFree(type);
type = NULL;
@@ -861,7 +885,13 @@
"%s", _("unexpected pool type"));
goto cleanup;
}
+
+ if (def->vendor == NULL || def->model == NULL ||
STREQ(def->vendor,"generic") || STREQ(def->model,"generic")) {
virBufferVSprintf(&buf, "<pool type='%s'>\n", type);
+ } else {
+ virBufferVSprintf(&buf, "<pool type='%s' vendor='%s'
model='%s'>\n", type, def->vendor, def->model);
+ }
+
virBufferVSprintf(&buf," <name>%s</name>\n",
def->name);
virUUIDFormat(def->uuid, uuid);
diff -Naurb libvirt-0.8.2.mod/src/conf/storage_conf.h
libvirt-0.8.2.ml/src/conf/storage_conf.h
--- libvirt-0.8.2.mod/src/conf/storage_conf.h 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/src/conf/storage_conf.h 2010-07-29
16:37:28.333458251 -0500
@@ -256,6 +256,8 @@
char *name;
unsigned char uuid[VIR_UUID_BUFLEN];
int type; /* virStoragePoolType */
+ char *vendor;
+ char *model;
unsigned long long allocation;
unsigned long long capacity;
diff -Naurb
libvirt-0.8.2.mod/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
---
libvirt-0.8.2.mod/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
1969-12-31 18:00:00.000000000 -0600
+++
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
2010-08-02 12:00:37.015538583 -0500
@@ -0,0 +1,17 @@
+<pool type='iscsi' vendor='test-vendor'
model='test-model'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+ <source>
+ <host name="iscsi.example.com"/>
+ <device path="demo-target"/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
+ </source>
+ <target>
+ <path>/dev/disk/by-path</path>
+ <permissions>
+ <mode>0700</mode>
+ <owner>0</owner>
+ <group>0</group>
+ </permissions>
+ </target>
+</pool>
diff -Naurb
libvirt-0.8.2.mod/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
---
libvirt-0.8.2.mod/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
1969-12-31 18:00:00.000000000 -0600
+++
libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
2010-08-02 12:16:19.571537734 -0500
@@ -0,0 +1,20 @@
+<pool type='iscsi' vendor='test-vendor'
model='test-model'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+ <capacity>0</capacity>
+ <allocation>0</allocation>
+ <available>0</available>
+ <source>
+ <host name='iscsi.example.com'/>
+ <device path='demo-target'/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
+ </source>
+ <target>
+ <path>/dev/disk/by-path</path>
+ <permissions>
+ <mode>0700</mode>
+ <owner>0</owner>
+ <group>0</group>
+ </permissions>
+ </target>
+</pool>
diff -Naurb libvirt-0.8.2.mod/tests/storagepoolxml2xmltest.c
libvirt-0.8.2.ml/tests/storagepoolxml2xmltest.c
--- libvirt-0.8.2.mod/tests/storagepoolxml2xmltest.c 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/tests/storagepoolxml2xmltest.c 2010-08-02
12:32:47.838532225 -0500
@@ -96,6 +96,7 @@
DO_TEST("pool-scsi");
DO_TEST("pool-mpath");
DO_TEST("pool-iscsi-multiiqn");
+ DO_TEST("pool-iscsi-vendor-model");
return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
}
2:38 p.m.
On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote:
Hi all,
I wrote a patch to add support for listing the Vendor and Model of a
storage pool in the storage pool XML. This would allow vendor
extensions of specific devices. The patch includes a test for the
new attributes as well. I'd appreciate some feedback on it, and
would like get it merged when it's ready. Patch added at the bottom
of the message.
Okay, principle doesn't sound bad. Though it would be better if such
things were autodetected instead of relying on user input. Assuming it's
not possible and noting that the patch does nothing except defining the
construct (i.e. would probably have to be applied in a serie containing
the real extensions later), let's make a first review:
diff -Naurb libvirt-0.8.2.mod/docs/schemas/storagepool.rng
libvirt-0.8.2.ml/docs/schemas/storagepool.rng
--- libvirt-0.8.2.mod/docs/schemas/storagepool.rng 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/docs/schemas/storagepool.rng 2010-07-30
17:19:32.835531455 -0500
@@ -8,6 +8,10 @@
<define name='pool'>
<element name='pool'>
+ <optional>
+ <ref name='poolvendor'/>
+ <ref name='poolmodel'/>
+ </optional>
<choice>
<ref name='pooldir'/>
<ref name='poolfs'/>
@@ -103,6 +107,18 @@
<ref name='target'/>
</define>
+ <define name='poolvendor'>
+ <attribute name='vendor'>
+ <text/>
+ </attribute>
+ </define>
+
+ <define name='poolmodel'>
+ <attribute name='model'>
+ <text/>
+ </attribute>
+ </define>
Hum ... since the constructs mandates that attributes are either
both present or both absent, it sounds more logical to have a single
optional ref with the 2 attributes definitions, instead of 2 definitions
encapsuled in a single optional block.
vendor and model are probably non empty too right ?
<define name='commonmetadata'>
<element name='name'>
<ref name='name'/>
diff -Naurb libvirt-0.8.2.mod/include/libvirt/libvirt.h
libvirt-0.8.2.ml/include/libvirt/libvirt.h
--- libvirt-0.8.2.mod/include/libvirt/libvirt.h 2010-07-22
10:26:05.000000000 -0500
+++ libvirt-0.8.2.ml/include/libvirt/libvirt.h 2010-07-30
11:56:48.726415607 -0500
@@ -1141,6 +1141,8 @@
typedef struct _virStoragePoolInfo virStoragePoolInfo;
struct _virStoragePoolInfo {
+ char *vendor;
+ char *model;
int state; /* virStoragePoolState flags */
unsigned long long capacity; /* Logical size bytes */
unsigned long long allocation; /* Current allocation bytes */
please add at the end of the structure, most significant information
first :-)
diff -Naurb libvirt-0.8.2.mod/src/conf/storage_conf.c
libvirt-0.8.2.ml/src/conf/storage_conf.c
--- libvirt-0.8.2.mod/src/conf/storage_conf.c 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/src/conf/storage_conf.c 2010-08-02
11:52:48.618533010 -0500
@@ -298,6 +298,12 @@
VIR_FREE(def->name);
+ if (def->vendor != "generic")
+ VIR_FREE(def->vendor);
+
+ if (def->model != "generic")
+ VIR_FREE(def->model);
+
virStoragePoolSourceFree(&def->source);
that's no good IMHO, it assumes the compiler/linker will
merge the identical strings present in the library to be the same
pointer address. Using STREQ for comparison is the right thing to do
and always allocate with strdup() that way we know the string is
always allocated dynamically.
In that case you can even remove the test.
VIR_FREE(def->target.path);
@@ -601,6 +607,8 @@
virStoragePoolDefPtr ret;
xmlNodePtr source_node;
char *type = NULL;
+ char *vendor = NULL;
+ char *model = NULL;
char *uuid = NULL;
char *tmppath;
@@ -616,6 +624,22 @@
goto cleanup;
}
+ /* Get the vendor and model from the XML and set them in the struct */
+ vendor = virXPathString("string(./@vendor)", ctxt);
+ model = virXPathString("string(./@model)", ctxt);
+
+ if (vendor != NULL) {
+ ret->vendor = vendor;
+ } else {
+ ret->vendor = "generic";
strdup
+ }
+
+ if (model != NULL) {
+ ret->model = model;
+ } else {
+ ret->model = "generic";
strdup
+ }
+
I also don't see why the absence of information from the user isn't
informations which should be preserved, instead of magic values
preallocated. IMHO keep the fields NULL if the user didn't provided
them. If later there is a way to do an automatic lookup you won't have
to change the library behaviour to plug it in !
xmlFree(type);
type = NULL;
@@ -861,7 +885,13 @@
"%s", _("unexpected pool type"));
goto cleanup;
}
+
+ if (def->vendor == NULL || def->model == NULL ||
STREQ(def->vendor,"generic") || STREQ(def->model,"generic")) {
no need for the generic test either in that case
> virBufferVSprintf(&buf, "<pool type='%s'>\n",
type);
> + } else {
> + virBufferVSprintf(&buf, "<pool type='%s'
vendor='%s'
> model='%s'>\n", type, def->vendor, def->model);
+ }
+
> virBufferVSprintf(&buf," <name>%s</name>\n",
def->name);
>
> virUUIDFormat(def->uuid, uuid);
> diff -Naurb libvirt-0.8.2.mod/src/conf/storage_conf.h
> libvirt-0.8.2.ml/src/conf/storage_conf.h
> --- libvirt-0.8.2.mod/src/conf/storage_conf.h 2010-06-16
> 17:27:22.000000000 -0500
> +++ libvirt-0.8.2.ml/src/conf/storage_conf.h 2010-07-29
> 16:37:28.333458251 -0500
> @@ -256,6 +256,8 @@
> char *name;
> unsigned char uuid[VIR_UUID_BUFLEN];
> int type; /* virStoragePoolType */
> + char *vendor;
> + char *model;
>
> unsigned long long allocation;
> unsigned long long capacity;
> diff -Naurb libvirt-0.8.2.mod/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
> --- libvirt-0.8.2.mod/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
> 1969-12-31 18:00:00.000000000 -0600
> +++ libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
> 2010-08-02 12:00:37.015538583 -0500
> @@ -0,0 +1,17 @@
> +<pool type='iscsi' vendor='test-vendor'
model='test-model'>
> + <name>virtimages</name>
> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
> + <source>
> + <host name="iscsi.example.com"/>
> + <device path="demo-target"/>
> + <auth type='chap' login='foobar' passwd='frobbar'/>
> + </source>
> + <target>
> + <path>/dev/disk/by-path</path>
> + <permissions>
> + <mode>0700</mode>
> + <owner>0</owner>
> + <group>0</group>
> + </permissions>
> + </target>
> +</pool>
> diff -Naurb libvirt-0.8.2.mod/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
> --- libvirt-0.8.2.mod/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
> 1969-12-31 18:00:00.000000000 -0600
> +++ libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-model.xml
> 2010-08-02 12:16:19.571537734 -0500
> @@ -0,0 +1,20 @@
> +<pool type='iscsi' vendor='test-vendor'
model='test-model'>
> + <name>virtimages</name>
> + <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
> + <capacity>0</capacity>
> + <allocation>0</allocation>
> + <available>0</available>
> + <source>
> + <host name='iscsi.example.com'/>
> + <device path='demo-target'/>
> + <auth type='chap' login='foobar' passwd='frobbar'/>
> + </source>
> + <target>
> + <path>/dev/disk/by-path</path>
> + <permissions>
> + <mode>0700</mode>
> + <owner>0</owner>
> + <group>0</group>
> + </permissions>
> + </target>
> +</pool>
> diff -Naurb libvirt-0.8.2.mod/tests/storagepoolxml2xmltest.c
> libvirt-0.8.2.ml/tests/storagepoolxml2xmltest.c
> --- libvirt-0.8.2.mod/tests/storagepoolxml2xmltest.c 2010-06-16
> 17:27:22.000000000 -0500
> +++ libvirt-0.8.2.ml/tests/storagepoolxml2xmltest.c 2010-08-02
> 12:32:47.838532225 -0500
> @@ -96,6 +96,7 @@
> DO_TEST("pool-scsi");
> DO_TEST("pool-mpath");
> DO_TEST("pool-iscsi-multiiqn");
> + DO_TEST("pool-iscsi-vendor-model");
>
> return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
> }
bonus point for adding a test case, but there is some revamp, a bit of
doucumentation, and it really doesn't make much sense to commit alone
with a use case for it :-)
thanks !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
7:24 a.m.
On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote:
Hi all,
I wrote a patch to add support for listing the Vendor and Model of a
storage pool in the storage pool XML. This would allow vendor
extensions of specific devices. The patch includes a test for the new
attributes as well. I'd appreciate some feedback on it, and would like
get it merged when it's ready. Patch added at the bottom of the message.
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
2010-08-02 12:00:37.015538583 -0500
@@ -0,0 +1,17 @@
+<pool type='iscsi' vendor='test-vendor'
model='test-model'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
We should avoid adding attributes to the top level <pool> tag.
The vendor/model is a piece of metadata associated with the
pool source, so it should be under
+ <source>
+ <host name="iscsi.example.com"/>
+ <device path="demo-target"/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
<vendor name='test-vendor'/>
<product name='test-product'/>
Also NB I called it 'product' instead of 'model', since that
matches the terminology we use in the node device XML format
+ </source>
+ <target>
+ <path>/dev/disk/by-path</path>
+ <permissions>
+ <mode>0700</mode>
+ <owner>0</owner>
+ <group>0</group>
+ </permissions>
+ </target>
+</pool>
REgards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:45 p.m.
On 08/05/2010 07:24 AM, Daniel P. Berrange wrote:
On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote:
> Hi all,
>
> I wrote a patch to add support for listing the Vendor and Model of a
> storage pool in the storage pool XML. This would allow vendor
> extensions of specific devices. The patch includes a test for the new
> attributes as well. I'd appreciate some feedback on it, and would like
> get it merged when it's ready. Patch added at the bottom of the message.
> libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
> 2010-08-02 12:00:37.015538583 -0500
> @@ -0,0 +1,17 @@
> +<pool type='iscsi' vendor='test-vendor'
model='test-model'>
> +<name>virtimages</name>
> +<uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
We should avoid adding attributes to the top level<pool> tag.
The vendor/model is a piece of metadata associated with the
pool source, so it should be under
Makes sense, I originally had it there, but
decided it made more sense
up top. Fixed in the newest patch
> +<source>
> +<host name="iscsi.example.com"/>
> +<device path="demo-target"/>
> +<auth type='chap' login='foobar' passwd='frobbar'/>
<vendor name='test-vendor'/>
<product name='test-product'/>
Also NB I called it 'product' instead of 'model', since that
matches the terminology we use in the node device XML format
Switched the naming there.
> +</source>
> +<target>
> +<path>/dev/disk/by-path</path>
> +<permissions>
> +<mode>0700</mode>
> +<owner>0</owner>
> +<group>0</group>
> +</permissions>
> +</target>
> +</pool>
REgards,
Daniel
I've prepped a new patch which should fix the issues brought up by both
Daniels. It's attached at the bottom.
Best,
Patrick Dignan
diff -Naurb libvirt-0.8.2.orig/docs/schemas/storagepool.rng
libvirt-0.8.2.ml/docs/schemas/storagepool.rng
--- libvirt-0.8.2.orig/docs/schemas/storagepool.rng 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/docs/schemas/storagepool.rng 2010-08-05
12:16:31.703536682 -0500
@@ -103,6 +103,19 @@
<ref name='target'/>
</define>
+ <define name='sourceinfovendor'>
+ <element name='vendor'>
+ <attribute name='name'>
+ <text/>
+ </attribute>
+ </element>
+ <element name='product'>
+ <attribute name='name'>
+ <text/>
+ </attribute>
+ </element>
+ </define>
+
<define name='commonmetadata'>
<element name='name'>
<ref name='name'/>
@@ -272,6 +285,9 @@
<value>ocfs2</value>
</choice>
</attribute>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</optional>
</define>
@@ -286,6 +302,9 @@
<value>nfs</value>
</choice>
</attribute>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</optional>
</define>
@@ -307,6 +326,9 @@
<value>lvm2</value>
</choice>
</attribute>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</optional>
</define>
@@ -321,6 +343,9 @@
<value>lvm2</value>
</choice>
</attribute>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</optional>
</define>
@@ -330,13 +355,20 @@
<optional>
<element name='source'>
<empty/>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</optional>
</define>
+
<define name='sourcefs'>
<element name='source'>
<ref name='sourceinfodev'/>
<ref name='sourcefmtfs'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</define>
@@ -345,6 +377,9 @@
<ref name='sourceinfohost'/>
<ref name='sourceinfodir'/>
<ref name='sourcefmtnetfs'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</define>
@@ -359,6 +394,9 @@
</optional>
</oneOrMore>
<ref name='sourcefmtlogical'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</define>
@@ -366,6 +404,9 @@
<element name='source'>
<ref name='sourceinfodev'/>
<ref name='sourcefmtdisk'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</define>
@@ -379,12 +420,19 @@
<optional>
<ref name='sourceinfoauth'/>
</optional>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
</element>
</define>
<define name='sourcescsi'>
<element name='source'>
<ref name='sourceinfoadapter'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
+
</element>
</define>
diff -Naurb libvirt-0.8.2.orig/include/libvirt/libvirt.h
libvirt-0.8.2.ml/include/libvirt/libvirt.h
--- libvirt-0.8.2.orig/include/libvirt/libvirt.h 2010-07-22
10:26:05.000000000 -0500
+++ libvirt-0.8.2.ml/include/libvirt/libvirt.h 2010-08-02
14:57:49.903530315 -0500
@@ -1145,6 +1145,8 @@
unsigned long long capacity; /* Logical size bytes */
unsigned long long allocation; /* Current allocation bytes */
unsigned long long available; /* Remaining free space bytes */
+ char *vendor;
+ char *model;
};
typedef virStoragePoolInfo *virStoragePoolInfoPtr;
diff -Naurb libvirt-0.8.2.orig/src/conf/storage_conf.c
libvirt-0.8.2.ml/src/conf/storage_conf.c
--- libvirt-0.8.2.orig/src/conf/storage_conf.c 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/src/conf/storage_conf.c 2010-08-05
13:31:32.469533098 -0500
@@ -284,6 +284,8 @@
VIR_FREE(source->name);
VIR_FREE(source->adapter);
VIR_FREE(source->initiator.iqn);
+ VIR_FREE(source->vendor);
+ VIR_FREE(source->product);
if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
VIR_FREE(source->auth.chap.login);
@@ -393,6 +395,8 @@
char *authType = NULL;
int nsource, i;
virStoragePoolOptionsPtr options;
+ char *vendor;
+ char *product;
relnode = ctxt->node;
ctxt->node = node;
@@ -465,6 +469,18 @@
goto cleanup;
}
+ vendor = virXPathString("string(./vendor/@name)", ctxt);
+
+ if (vendor != NULL) {
+ source->vendor = strdup(vendor);
+ }
+
+ product = virXPathString("string(./product/@name)", ctxt);
+
+ if (product != NULL) {
+ source->product = strdup(product);
+ }
+
ret = 0;
cleanup:
ctxt->node = relnode;
@@ -838,6 +854,12 @@
virBufferVSprintf(buf," <auth type='chap' login='%s'
passwd='%s'/>\n",
src->auth.chap.login,
src->auth.chap.passwd);
+
+ if (src->vendor != NULL && src->product != NULL) {
+ virBufferVSprintf(buf," <vendor name='%s'/>\n",
src->vendor);
+ virBufferVSprintf(buf," <product name='%s'/>\n",
src->product);
+ }
+
virBufferAddLit(buf," </source>\n");
return 0;
diff -Naurb libvirt-0.8.2.orig/src/conf/storage_conf.h
libvirt-0.8.2.ml/src/conf/storage_conf.h
--- libvirt-0.8.2.orig/src/conf/storage_conf.h 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/src/conf/storage_conf.h 2010-08-05
10:53:41.034539304 -0500
@@ -237,6 +237,12 @@
virStoragePoolAuthChap chap;
} auth;
+ /* Vendor of the the source */
+ char *vendor;
+
+ /* Product name of the source*/
+ char *product;
+
int format; /* Pool type specific format such as filesystem type,
or lvm version, etc */
};
diff -Naurb
libvirt-0.8.2.orig/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
---
libvirt-0.8.2.orig/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
1969-12-31 18:00:00.000000000 -0600
+++
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
2010-08-05 10:55:29.097539286 -0500
@@ -0,0 +1,19 @@
+<pool type='iscsi'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+ <source>
+ <host name="iscsi.example.com"/>
+ <device path="demo-target"/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
+ <vendor name='test-vendor'/>
+ <product name='test-model'/>
+ </source>
+ <target>
+ <path>/dev/disk/by-path</path>
+ <permissions>
+ <mode>0700</mode>
+ <owner>0</owner>
+ <group>0</group>
+ </permissions>
+ </target>
+</pool>
diff -Naurb
libvirt-0.8.2.orig/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
---
libvirt-0.8.2.orig/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
1969-12-31 18:00:00.000000000 -0600
+++
libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
2010-08-05 10:56:50.137535457 -0500
@@ -0,0 +1,22 @@
+<pool type='iscsi'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+ <capacity>0</capacity>
+ <allocation>0</allocation>
+ <available>0</available>
+ <source>
+ <host name='iscsi.example.com'/>
+ <device path='demo-target'/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
+ <vendor name='test-vendor'/>
+ <product name='test-model'/>
+ </source>
+ <target>
+ <path>/dev/disk/by-path</path>
+ <permissions>
+ <mode>0700</mode>
+ <owner>0</owner>
+ <group>0</group>
+ </permissions>
+ </target>
+</pool>
diff -Naurb libvirt-0.8.2.orig/tests/storagepoolxml2xmltest.c
libvirt-0.8.2.ml/tests/storagepoolxml2xmltest.c
--- libvirt-0.8.2.orig/tests/storagepoolxml2xmltest.c 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/tests/storagepoolxml2xmltest.c 2010-08-05
11:11:20.823541487 -0500
@@ -96,6 +96,7 @@
DO_TEST("pool-scsi");
DO_TEST("pool-mpath");
DO_TEST("pool-iscsi-multiiqn");
+ DO_TEST("pool-iscsi-vendor-product");
return (ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
}
4:51 p.m.
2010/8/5 Patrick Dignan <pat_dignan(a)dell.com>:
On 08/05/2010 07:24 AM, Daniel P. Berrange wrote:
>
> On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote:
>>
>> Hi all,
>>
>> I wrote a patch to add support for listing the Vendor and Model of a
>> storage pool in the storage pool XML. This would allow vendor
>> extensions of specific devices. The patch includes a test for the new
>> attributes as well. I'd appreciate some feedback on it, and would like
>> get it merged when it's ready. Patch added at the bottom of the message.
>> libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
>> 2010-08-02 12:00:37.015538583 -0500
>> @@ -0,0 +1,17 @@
>> +<pool type='iscsi' vendor='test-vendor'
model='test-model'>
>> +<name>virtimages</name>
>> +<uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
>
> We should avoid adding attributes to the top level<pool> tag.
> The vendor/model is a piece of metadata associated with the
> pool source, so it should be under
Makes sense, I originally had it there, but decided it made more sense up
top. Fixed in the newest patch
>>
>> +<source>
>> +<host name="iscsi.example.com"/>
>> +<device path="demo-target"/>
>> +<auth type='chap' login='foobar'
passwd='frobbar'/>
>
> <vendor name='test-vendor'/>
> <product name='test-product'/>
>
> Also NB I called it 'product' instead of 'model', since that
> matches the terminology we use in the node device XML format
>
Switched the naming there.
>>
>> +</source>
>> +<target>
>> +<path>/dev/disk/by-path</path>
>> +<permissions>
>> +<mode>0700</mode>
>> +<owner>0</owner>
>> +<group>0</group>
>> +</permissions>
>> +</target>
>> +</pool>
>
> REgards,
> Daniel
I've prepped a new patch which should fix the issues brought up by both
Daniels. It's attached at the bottom.
Best,
Patrick Dignan
diff -Naurb libvirt-0.8.2.orig/include/libvirt/libvirt.h
libvirt-0.8.2.ml/include/libvirt/libvirt.h
--- libvirt-0.8.2.orig/include/libvirt/libvirt.h 2010-07-22
10:26:05.000000000 -0500
+++ libvirt-0.8.2.ml/include/libvirt/libvirt.h 2010-08-02
14:57:49.903530315 -0500
@@ -1145,6 +1145,8 @@
unsigned long long capacity; /* Logical size bytes */
unsigned long long allocation; /* Current allocation bytes */
unsigned long long available; /* Remaining free space bytes */
+ char *vendor;
+ char *model;
};
Why do you add new members to the virStoragePoolInfo struct? Also
libvirt.h is generated from libvirt.h.in.
It seems that those members are a leftover from previous versions of
this patch and can be removed entirely?
typedef virStoragePoolInfo *virStoragePoolInfoPtr;
diff -Naurb libvirt-0.8.2.orig/src/conf/storage_conf.c
libvirt-0.8.2.ml/src/conf/storage_conf.c
--- libvirt-0.8.2.orig/src/conf/storage_conf.c 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/src/conf/storage_conf.c 2010-08-05
13:31:32.469533098 -0500
@@ -284,6 +284,8 @@
VIR_FREE(source->name);
VIR_FREE(source->adapter);
VIR_FREE(source->initiator.iqn);
+ VIR_FREE(source->vendor);
+ VIR_FREE(source->product);
if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) {
VIR_FREE(source->auth.chap.login);
@@ -393,6 +395,8 @@
char *authType = NULL;
int nsource, i;
virStoragePoolOptionsPtr options;
+ char *vendor;
+ char *product;
relnode = ctxt->node;
ctxt->node = node;
@@ -465,6 +469,18 @@
goto cleanup;
}
+ vendor = virXPathString("string(./vendor/@name)", ctxt);
+
+ if (vendor != NULL) {
+ source->vendor = strdup(vendor);
OOM check is missing here:
if (source->vendor == NULL) {
virReportOOMerror();
goto cleanup;
}
+ }
+
+ product = virXPathString("string(./product/@name)", ctxt);
+
+ if (product != NULL) {
+ source->product = strdup(product);
OOM check is missing here too.
+ }
+
Actually, why are you strdup'ing the result of virXPathString at all?
virXPathString gives you an strdup'ed string already. So strdup'ing it
again leaks the first instance of the string, because you miss to free
it.
This can be simplified to:
source->vendor = virXPathString("string(./vendor/@name)", ctxt);
source->product = virXPathString("string(./product/@name)", ctxt);
ret = 0;
cleanup:
ctxt->node = relnode;
@@ -838,6 +854,12 @@
virBufferVSprintf(buf," <auth type='chap' login='%s'
There are actually 4 spaces between " and < in that line. It seems
that your mail client has mangled the diff. Please attach the diff as
text file instead of pasting it into the mail body, or even better use
a git clone (instead of the 0.8.2 tarball) as base for your patch and
use git-send-email to mail the commit to the list.
passwd='%s'/>\n",
src->auth.chap.login,
src->auth.chap.passwd);
+
+ if (src->vendor != NULL && src->product != NULL) {
+ virBufferVSprintf(buf," <vendor name='%s'/>\n",
src->vendor);
+ virBufferVSprintf(buf," <product name='%s'/>\n",
src->product);
You expect that vendor _and_ product are specified, but you don't
verify that in the parsing function. So you either need to enforce
this in the parsing function or split the formatting into two separate
if statements here.
libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
2010-08-05 10:55:29.097539286 -0500
@@ -0,0 +1,19 @@
+<pool type='iscsi'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+ <source>
+ <host name="iscsi.example.com"/>
+ <device path="demo-target"/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
+ <vendor name='test-vendor'/>
+ <product name='test-model'/>
Change test-model to test-product for consistence.
libvirt-0.8.2.ml/tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
2010-08-05 10:56:50.137535457 -0500
@@ -0,0 +1,22 @@
+<pool type='iscsi'>
+ <name>virtimages</name>
+ <uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
+ <capacity>0</capacity>
+ <allocation>0</allocation>
+ <available>0</available>
+ <source>
+ <host name='iscsi.example.com'/>
+ <device path='demo-target'/>
+ <auth type='chap' login='foobar' passwd='frobbar'/>
+ <vendor name='test-vendor'/>
+ <product name='test-model'/>
Change test-model to test-product for consistence here too.
Matthias
3:16 a.m.
On Thu, Aug 05, 2010 at 11:51:48PM +0200, Matthias Bolte wrote:
2010/8/5 Patrick Dignan <pat_dignan(a)dell.com>:
> On 08/05/2010 07:24 AM, Daniel P. Berrange wrote:
>>
>> On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote:
>>>
>>> Hi all,
>>>
>>> I wrote a patch to add support for listing the Vendor and Model of a
>>> storage pool in the storage pool XML. This would allow vendor
>>> extensions of specific devices. The patch includes a test for the new
>>> attributes as well. I'd appreciate some feedback on it, and would like
>>> get it merged when it's ready. Patch added at the bottom of the
message.
>>> libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
>>> 2010-08-02 12:00:37.015538583 -0500
>>> @@ -0,0 +1,17 @@
>>> +<pool type='iscsi' vendor='test-vendor'
model='test-model'>
>>> +<name>virtimages</name>
>>> +<uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
>>
>> We should avoid adding attributes to the top level<pool> tag.
>> The vendor/model is a piece of metadata associated with the
>> pool source, so it should be under
>
> Makes sense, I originally had it there, but decided it made more sense up
> top. Fixed in the newest patch
>>>
>>> +<source>
>>> +<host name="iscsi.example.com"/>
>>> +<device path="demo-target"/>
>>> +<auth type='chap' login='foobar'
passwd='frobbar'/>
>>
>> <vendor name='test-vendor'/>
>> <product name='test-product'/>
>>
>> Also NB I called it 'product' instead of 'model', since that
>> matches the terminology we use in the node device XML format
>>
> Switched the naming there.
>>>
>>> +</source>
>>> +<target>
>>> +<path>/dev/disk/by-path</path>
>>> +<permissions>
>>> +<mode>0700</mode>
>>> +<owner>0</owner>
>>> +<group>0</group>
>>> +</permissions>
>>> +</target>
>>> +</pool>
>>
>> REgards,
>> Daniel
>
> I've prepped a new patch which should fix the issues brought up by both
> Daniels. It's attached at the bottom.
>
> Best,
>
> Patrick Dignan
>
> diff -Naurb libvirt-0.8.2.orig/include/libvirt/libvirt.h
> libvirt-0.8.2.ml/include/libvirt/libvirt.h
> --- libvirt-0.8.2.orig/include/libvirt/libvirt.h 2010-07-22
> 10:26:05.000000000 -0500
> +++ libvirt-0.8.2.ml/include/libvirt/libvirt.h 2010-08-02
> 14:57:49.903530315 -0500
> @@ -1145,6 +1145,8 @@
> unsigned long long capacity; /* Logical size bytes */
> unsigned long long allocation; /* Current allocation bytes */
> unsigned long long available; /* Remaining free space bytes */
> + char *vendor;
> + char *model;
> };
Why do you add new members to the virStoragePoolInfo struct? Also
libvirt.h is generated from libvirt.h.in.
It seems that those members are a leftover from previous versions of
this patch and can be removed entirely?
Yes, those have to be removed as they are changing public ABI.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
2:32 p.m.
On 08/06/2010 03:16 AM, Daniel P. Berrange wrote:
On Thu, Aug 05, 2010 at 11:51:48PM +0200, Matthias Bolte wrote:
> 2010/8/5 Patrick Dignan<pat_dignan(a)dell.com>:
>> On 08/05/2010 07:24 AM, Daniel P. Berrange wrote:
>>> On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote:
>>>> Hi all,
>>>>
>>>> I wrote a patch to add support for listing the Vendor and Model of a
>>>> storage pool in the storage pool XML. This would allow vendor
>>>> extensions of specific devices. The patch includes a test for the new
>>>> attributes as well. I'd appreciate some feedback on it, and would
like
>>>> get it merged when it's ready. Patch added at the bottom of the
message.
>>>> libvirt-0.8.2.ml/tests/storagepoolxml2xmlin/pool-iscsi-vendor-model.xml
>>>> 2010-08-02 12:00:37.015538583 -0500
>>>> @@ -0,0 +1,17 @@
>>>> +<pool type='iscsi' vendor='test-vendor'
model='test-model'>
>>>> +<name>virtimages</name>
>>>> +<uuid>e9392370-2917-565e-692b-d057f46512d6</uuid>
>>> We should avoid adding attributes to the top level<pool> tag.
>>> The vendor/model is a piece of metadata associated with the
>>> pool source, so it should be under
>> Makes sense, I originally had it there, but decided it made more sense up
>> top. Fixed in the newest patch
>>>> +<source>
>>>> +<host name="iscsi.example.com"/>
>>>> +<device path="demo-target"/>
>>>> +<auth type='chap' login='foobar'
passwd='frobbar'/>
>>> <vendor name='test-vendor'/>
>>> <product name='test-product'/>
>>>
>>> Also NB I called it 'product' instead of 'model', since that
>>> matches the terminology we use in the node device XML format
>>>
>> Switched the naming there.
>>>> +</source>
>>>> +<target>
>>>> +<path>/dev/disk/by-path</path>
>>>> +<permissions>
>>>> +<mode>0700</mode>
>>>> +<owner>0</owner>
>>>> +<group>0</group>
>>>> +</permissions>
>>>> +</target>
>>>> +</pool>
>>> REgards,
>>> Daniel
>> I've prepped a new patch which should fix the issues brought up by both
>> Daniels. It's attached at the bottom.
>>
>> Best,
>>
>> Patrick Dignan
>>
>> diff -Naurb libvirt-0.8.2.orig/include/libvirt/libvirt.h
>> libvirt-0.8.2.ml/include/libvirt/libvirt.h
>> --- libvirt-0.8.2.orig/include/libvirt/libvirt.h 2010-07-22
>> 10:26:05.000000000 -0500
>> +++ libvirt-0.8.2.ml/include/libvirt/libvirt.h 2010-08-02
>> 14:57:49.903530315 -0500
>> @@ -1145,6 +1145,8 @@
>> unsigned long long capacity; /* Logical size bytes */
>> unsigned long long allocation; /* Current allocation bytes */
>> unsigned long long available; /* Remaining free space bytes */
>> + char *vendor;
>> + char *model;
>> };
> Why do you add new members to the virStoragePoolInfo struct? Also
> libvirt.h is generated from libvirt.h.in.
>
> It seems that those members are a leftover from previous versions of
> this patch and can be removed entirely?
Yes, those have to be removed as they are changing public ABI.
Regards,
Daniel
Somehow the git-send-email patch didn't make it to the mailing list,
though my reply to it did...I've attached the patch here, hopefully this
one doesn't get flattened.
I decided to allow vendor and product to be set independently of each
other in order to allow greater flexibility at the possible expense of
namespace clashes.
Best,
Patrick Dignan
3:09 p.m.
On 08/09/2010 01:32 PM, Patrick Dignan wrote:
Somehow the git-send-email patch didn't make it to the mailing
list,
though my reply to it did...I've attached the patch here, hopefully this
one doesn't get flattened.
Much nicer.
I decided to allow vendor and product to be set independently of each
other in order to allow greater flexibility at the possible expense of
namespace clashes.
+ <define name='sourceinfovendor'>
+ <optional>
+ <element name='vendor'>
+ <attribute name='name'>
+ <text/>
+ </attribute>
+ </element>
+ </optional>
+ <optional>
+ <element name='product'>
+ <attribute name='name'>
+ <text/>
+ </attribute>
+ </element>
+ </optional>
+ </define>
So, the sourceinfovendor definition can be completely empty; yet...
@@ -272,6 +289,9 @@
<value>ocfs2</value>
</choice>
</attribute>
+ <optional>
+ <ref name='sourceinfovendor'/>
+ </optional>
...we always mark it as optional. Do we need the <optional> at all the
callers, if the <ref> itself can be empty? I don't know enough about
.rng format to answer this question definitively.
<ref name='sourceinfodev'/>
<ref name='sourcefmtfs'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
Spacing is weird here - only need to indent by +2, not +4, for
consistency with the rest of the file.
But the rest of the patch looked sane to me. I'll wait for someone with
more .rng experience to chime in, though.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
12:06 p.m.
On Mon, Aug 09, 2010 at 02:32:55PM -0500, Patrick Dignan wrote:
Somehow the git-send-email patch didn't make it to the mailing
list,
though my reply to it did...I've attached the patch here, hopefully this
one doesn't get flattened.
I decided to allow vendor and product to be set independently of each
other in order to allow greater flexibility at the possible expense of
namespace clashes.
From 3a2139c87a1d1c92a8bf947ecc5b431f0f67906c Mon Sep 17 00:00:00
2001
From: Patrick Dignan <pat_dignan(a)dell.com>
Date: Fri, 6 Aug 2010 11:08:03 -0500
Subject: [PATCH] Add support for vendor/product
---
docs/schemas/storagepool.rng | 52 ++++++++++++++++++++
src/conf/storage_conf.c | 14 +++++
src/conf/storage_conf.h | 6 ++
.../pool-iscsi-vendor-product.xml | 19 +++++++
.../pool-iscsi-vendor-product.xml | 22 ++++++++
tests/storagepoolxml2xmltest.c | 1 +
6 files changed, 114 insertions(+), 0 deletions(-)
create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
@@ -838,6 +843,15 @@ virStoragePoolSourceFormat(virBufferPtr buf,
virBufferVSprintf(buf," <auth type='chap' login='%s'
passwd='%s'/>\n",
src->auth.chap.login,
src->auth.chap.passwd);
+
+ if (src->vendor != NULL) {
+ virBufferVSprintf(buf," <vendor name='%s'/>\n",
src->vendor);
+ }
+
+ if (src->product != NULL) {
+ virBufferVSprintf(buf," <product name='%s'/>\n",
src->product);
+ }
+
virBufferAddLit(buf," </source>\n");
The only change is that these should use virBufferEscapeString
since vendor/product strings might contain <, > or & characters
which would make the XML unhappy
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:44 p.m.
On 08/17/2010 12:06 PM, Daniel P. Berrange wrote:
On Mon, Aug 09, 2010 at 02:32:55PM -0500, Patrick Dignan wrote:
> Somehow the git-send-email patch didn't make it to the mailing list,
> though my reply to it did...I've attached the patch here, hopefully this
> one doesn't get flattened.
>
> I decided to allow vendor and product to be set independently of each
> other in order to allow greater flexibility at the possible expense of
> namespace clashes.
> From 3a2139c87a1d1c92a8bf947ecc5b431f0f67906c Mon Sep 17 00:00:00 2001
> From: Patrick Dignan<pat_dignan(a)dell.com>
> Date: Fri, 6 Aug 2010 11:08:03 -0500
> Subject: [PATCH] Add support for vendor/product
>
> ---
> docs/schemas/storagepool.rng | 52 ++++++++++++++++++++
> src/conf/storage_conf.c | 14 +++++
> src/conf/storage_conf.h | 6 ++
> .../pool-iscsi-vendor-product.xml | 19 +++++++
> .../pool-iscsi-vendor-product.xml | 22 ++++++++
> tests/storagepoolxml2xmltest.c | 1 +
> 6 files changed, 114 insertions(+), 0 deletions(-)
> create mode 100644 tests/storagepoolxml2xmlin/pool-iscsi-vendor-product.xml
> create mode 100644 tests/storagepoolxml2xmlout/pool-iscsi-vendor-product.xml
>
> @@ -838,6 +843,15 @@ virStoragePoolSourceFormat(virBufferPtr buf,
> virBufferVSprintf(buf,"<auth type='chap' login='%s'
passwd='%s'/>\n",
> src->auth.chap.login,
> src->auth.chap.passwd);
> +
> + if (src->vendor != NULL) {
> + virBufferVSprintf(buf,"<vendor name='%s'/>\n",
src->vendor);
> + }
> +
> + if (src->product != NULL) {
> + virBufferVSprintf(buf,"<product name='%s'/>\n",
src->product);
> + }
> +
> virBufferAddLit(buf,"</source>\n");
The only change is that these should use virBufferEscapeString
since vendor/product strings might contain<,> or& characters
which would make the XML unhappy
Regards,
Daniel
Ok, so I've updated that. Hopefully all is well with this patch!
Best,
Patrick Dignan
5:09 p.m.
On 08/17/2010 11:44 AM, Patrick Dignan wrote:
>> + if (src->product != NULL) {
>> + virBufferVSprintf(buf,"<product name='%s'/>\n",
src->product);
>> + }
>> +
>> virBufferAddLit(buf,"</source>\n");
> The only change is that these should use virBufferEscapeString
> since vendor/product strings might contain<,> or& characters
> which would make the XML unhappy
>
>
> Regards,
> Daniel
Ok, so I've updated that. Hopefully all is well with this patch!
Almost. The .rng file had some whitespace issues. Also, the
sourcefmtfs reference only occurs as part of the sourcefs element, so
only the latter needed the optional sourceinfovendor. And 'make
syntax-check' didn't like your use of "the the" in a comment nor your
trailing spaces.
Meanwhile, you missed documentation in docs/formatstorage.html.in. I'm
getting more serious about blocking patches to docs/schemas without the
corresponding changes to docs/format*html.in, because even if it's hard
to do up front, it's much harder to do long after the fact; but until we
have a strong precedence of that action in the git history, I can see
how you overlooked it.
But those were minor enough to have my ACK; I didn't mind touching this
up since it was your first submission, but hopefully don't have to spend
as much cleanup work on future patches from you. Here's what I squashed
in before pushing (along with an AUTHORS update not listed here).
diff --git i/docs/formatstorage.html.in w/docs/formatstorage.html.in
index 5c1d36c..91f70a3 100644
--- i/docs/formatstorage.html.in
+++ w/docs/formatstorage.html.in
@@ -70,6 +70,8 @@
<source>
<host name="iscsi.example.com"/>
<device path="demo-target"/>
+ <vendor name="Acme"/>
+ <product name="model"/&t;
</source>
...</pre>
@@ -108,6 +110,16 @@
type, or network filesystem type, or partition table type, or
LVM metadata type. All drivers are required to have a default
value for this, so it is optional. <span class="since">Since
0.4.1</span></dd>
+
+ <dt><code>vendor</code></dt>
+ <dd>Provides optional information about the vendor of the
+ storage device. This contains a single
+ attribute <code>name</code> whose value is backend
+ specific. <span class="since">Since
0.8.4</span></dd>
+ <dt><code>product</code></dt>
+ <dd>Provides an optional product name of the storage device.
+ This contains a single attribute <code>name</code> whose value
+ is backend specific. <span class="since">Since
0.8.4</span></dd>
</dl>
<h3><a name="StoragePoolTarget">Target
elements</a></h3>
diff --git i/docs/schemas/storagepool.rng w/docs/schemas/storagepool.rng
index a8a3f36..54eb802 100644
--- i/docs/schemas/storagepool.rng
+++ w/docs/schemas/storagepool.rng
@@ -289,9 +289,6 @@
<value>ocfs2</value>
</choice>
</attribute>
- <optional>
- <ref name='sourceinfovendor'/>
- </optional>
</element>
</optional>
</define>
@@ -371,7 +368,7 @@
<ref name='sourceinfodev'/>
<ref name='sourcefmtfs'/>
<optional>
- <ref name='sourceinfovendor'/>
+ <ref name='sourceinfovendor'/>
</optional>
</element>
</define>
@@ -382,7 +379,7 @@
<ref name='sourceinfodir'/>
<ref name='sourcefmtnetfs'/>
<optional>
- <ref name='sourceinfovendor'/>
+ <ref name='sourceinfovendor'/>
</optional>
</element>
</define>
@@ -399,7 +396,7 @@
</oneOrMore>
<ref name='sourcefmtlogical'/>
<optional>
- <ref name='sourceinfovendor'/>
+ <ref name='sourceinfovendor'/>
</optional>
</element>
</define>
@@ -408,8 +405,8 @@
<element name='source'>
<ref name='sourceinfodev'/>
<ref name='sourcefmtdisk'/>
- <optional>
- <ref name='sourceinfovendor'/>
+ <optional>
+ <ref name='sourceinfovendor'/>
</optional>
</element>
</define>
@@ -425,7 +422,7 @@
<ref name='sourceinfoauth'/>
</optional>
<optional>
- <ref name='sourceinfovendor'/>
+ <ref name='sourceinfovendor'/>
</optional>
</element>
</define>
@@ -434,7 +431,7 @@
<element name='source'>
<ref name='sourceinfoadapter'/>
<optional>
- <ref name='sourceinfovendor'/>
+ <ref name='sourceinfovendor'/>
</optional>
</element>
diff --git i/src/conf/storage_conf.h w/src/conf/storage_conf.h
index 5f17b5a..fef0a46 100644
--- i/src/conf/storage_conf.h
+++ w/src/conf/storage_conf.h
@@ -237,7 +237,7 @@ struct _virStoragePoolSource {
virStoragePoolAuthChap chap;
} auth;
- /* Vendor of the the source */
+ /* Vendor of the source */
char *vendor;
/* Product name of the source*/
diff --git i/src/conf/storage_conf.c w/src/conf/storage_conf.c
index cf6271e..168243f 100644
--- i/src/conf/storage_conf.c
+++ w/src/conf/storage_conf.c
@@ -844,14 +844,14 @@ virStoragePoolSourceFormat(virBufferPtr buf,
src->auth.chap.login,
src->auth.chap.passwd);
- if (src->vendor != NULL) {
+ if (src->vendor != NULL) {
virBufferEscapeString(buf," <vendor name='%s'/>\n",
src->vendor);
}
-
+
if (src->product != NULL) {
virBufferEscapeString(buf," <product name='%s'/>\n",
src->product);
- }
-
+ }
+
virBufferAddLit(buf," </source>\n");
return 0;
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:54 p.m.
On 08/05/2010 12:45 PM, Patrick Dignan wrote:
diff -Naurb libvirt-0.8.2.orig/docs/schemas/storagepool.rng
Ouch - this flattened the whitespace to the point that it makes
reviewing harder to do. Could you consider resending, either via 'git
send-email' or by attaching the output of 'git format-patch', such that
your indentation is preserved?
libvirt-0.8.2.ml/docs/schemas/storagepool.rng
--- libvirt-0.8.2.orig/docs/schemas/storagepool.rng 2010-06-16
17:27:22.000000000 -0500
+++ libvirt-0.8.2.ml/docs/schemas/storagepool.rng 2010-08-05
12:16:31.703536682 -0500
@@ -103,6 +103,19 @@
<ref name='target'/>
</define>
+ <define name='sourceinfovendor'>
+ <element name='vendor'>
+ <attribute name='name'>
+ <text/>
+ </attribute>
+ </element>
+ <element name='product'>
+ <attribute name='name'>
+ <text/>
+ </attribute>
+ </element>
+ </define>
Would it ever make sense to have only vendor or product, rather than to
always require both elements or neither? If so, you either need two
<define>s or some more .rng magic around the two <element>s.
@@ -465,6 +469,18 @@
goto cleanup;
}
+ vendor = virXPathString("string(./vendor/@name)", ctxt);
+
+ if (vendor != NULL) {
+ source->vendor = strdup(vendor);
virXPathString returns malloc'd memory. No need to strdup it; just use
it as is - that will avoid a memory leak, since you didn't call
VIR_FREE(vendor) in the cleanup: label.
+ }
+
+ product = virXPathString("string(./product/@name)", ctxt);
+
+ if (product != NULL) {
+ source->product = strdup(product);
+ }
+
Back to the mutually-essential question - per your current .rng, you
should issue an error here if (product==NULL)!=(vendor==NULL). But is
that really essential, or does it make sense that you could have one and
not the other?
ret = 0;
cleanup:
ctxt->node = relnode;
@@ -838,6 +854,12 @@
virBufferVSprintf(buf," <auth type='chap' login='%s'
passwd='%s'/>\n",
src->auth.chap.login,
src->auth.chap.passwd);
+
+ if (src->vendor != NULL && src->product != NULL) {
+ virBufferVSprintf(buf," <vendor name='%s'/>\n",
src->vendor);
+ virBufferVSprintf(buf," <product name='%s'/>\n",
src->product);
+ }
More code that assumes both elements will either be present or absent
together.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
5212
days inactive
5229
days old
11 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Matthias Bolte -
Patrick Dignan