2010/8/5 Patrick Dignan <pat_dignan@xxxxxxxx>:
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list