Re: [libvirt] [PATCH] Add support for Vendor and Model in Storage Pool XML

On Mon, Aug 02, 2010 at 12:44:37PM -0500, Patrick Dignan wrote: 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: 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 ? please add at the end of the structure, most significant information first :-) 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. strdup 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 ! 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/