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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list