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

On Thu, Aug 05, 2010 at 11:51:48PM +0200, Matthias Bolte wrote: > 2010/8/5 Patrick Dignan&lt;pat_dignan(a)dell.com&gt;: >> 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