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

2010/8/5 Patrick Dignan <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?