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