On Thu, Aug 05, 2010 at 11:51:48PM +0200, Matthias Bolte wrote: > 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? Yes, those have to be removed as they are changing public ABI. Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list