Re: [libvirt] [PATCH 1/1] Implement NPIV
On Wed, Apr 08, 2009 at 05:53:57PM -0400, Dave Allan wrote:
Daniel P. Berrange wrote:
>>diff --git a/src/storage_conf.h b/src/storage_conf.h
>>index 4e35ccb..cfd8b14 100644
>>--- a/src/storage_conf.h
>>+++ b/src/storage_conf.h
>>@@ -192,6 +192,12 @@ struct _virStoragePoolSource {
>> /* Or an adapter */
>> char *adapter;
>>
>>+ /* And an optional WWPN */
>>+ char *wwpn;
>>+
>>+ /* And an optional WWNN */
>>+ char *wwnn;
>>+
>
>Since adapter, wwpn and wwnn are all associated with each other, I
>think we should put them all into a union here. eg, replace the
>current
>
> char *adapter;
>
>with
>
> union {
> char *name;
> char *wwpn;
> char *wwnn;
> } adapter;
I don't have completely working code yet, but here's what I'm thinking:
We can't use a union because wwpn and wwnn will always be in use at the
same time, but I agree that we're starting to collect a bunch of related
information about adapters that clutters up the pool source struct.
I've created a struct along the lines of virStoragePoolSourceHost to
contain this information. It will need some additional fields.
I don't know what I was thinking when I wrote 'union'. I absolutely
meant an anonymous 'struct'. A named struct as you show below is
even better
/*
* For adapter based pools, info on the relevant adapter
*/
typedef struct _virStoragePoolSourceAdapter virStoragePoolSourceAdapter;
typedef virStoragePoolSourceAdapter *virStoragePoolSourceAdapterPtr;
struct _virStoragePoolSourceAdapter {
char *name;
char *wwpn;
char *wwnn;
};
Since we might be creating a virtual adapter on a physical adapter that
we specify by wwpn/wwnn, we'll need two places to specify wwpn/wwnn in
the XML. I propose:
Hmm, yes, this is what made me think originally that the storage pool
may not be the right place to put the creation/deletion of the vport.
Working through the steps for using NPIV in our APIs...
- list of HBAs == virNodeListDevices() with cap='scsi_host'
- For each HBA
- Query XML of the virNodeDevicePtr object
- Look for 'vport' capability flag
You now have an virNodeDevicePtr object which you know is able to
create/delete vports, lets say its name is 'host3'.
- Define config for a new vport, giving <parent> as the name of
virNodeDevicePtr we just obtained
<device>
<parent>host3</parent>
<capability type='scsi_host'>
<capability type='fc'>
<wwpn>88889999aaaabbbb</wwpn>
<wwnn>4444555566667777</wwnn>
</capability>
</capability>
</device>
- Create the device with virNodeDeviceCreate(char *xml);
- Use the device by defining a SCSI storage pool using the
<host></host> property of the device we just created
This keeps the notion of hierarchy / parent+child relationship between
HBAs just witin the node device APIs, and avoids replicating it in the
storage APIs.
I still think it would be worth adding a 'wwpn' and 'wwnn' to the
SCSI storage pool XML config - to be used as an alternative to the
'host' parameter, since wwpn+wwnn are guarenteed stable across
machines, whereas 'host' is only unique within scope of a single
machine and even changes across boot.
This does of course mean swe need to add new virNodeDeviceCreate
and virNodeDeviceDestroy APIs, but I think it is worth doing that
and they can be useful for other areas too. For example, it would
allow us to have a means to create loopback devices, or create
NBD devices (via qemu-nbd), which are useful things todo if you
want to get into the innards of file based disks
<pool type="scsi">
<name>npiv</name>
<source>
<adapter name="host6" wwpn="0000111122223333"
wwnn="4444555566667777"/>
<vport wwpn="88889999aaaabbbb" wwnn="ccccddddeeeeffff"/>
So, we'd keep the <adapter> line as you show, but would
not need the <vport> line, because the <adater> line would
refer to the wwpn/wwn/host of the vport itself
</source>
<target>
<path>/dev/disk/by-id</path>
</target>
</pool>
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|