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>