[libvirt] [PATCH] Add support for Vendor and Model in Storage Pool XML
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); }
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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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 :|
On 08/05/2010 07:24 AM, Daniel P. Berrange 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
On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote: 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); }
2010/8/5 Patrick Dignan <pat_dignan@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
On Thu, Aug 05, 2010 at 11:51:48PM +0200, Matthias Bolte wrote:
2010/8/5 Patrick Dignan <pat_dignan@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 :|
On Thu, Aug 05, 2010 at 11:51:48PM +0200, Matthias Bolte wrote:
On 08/05/2010 07:24 AM, Daniel P. Berrange 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
On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote: 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
2010/8/5 Patrick Dignan<pat_dignan@dell.com>: 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,
On 08/06/2010 03:16 AM, Daniel P. Berrange wrote: 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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@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 :|
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@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
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake -
Matthias Bolte -
Patrick Dignan