Re: [libvirt] [PATCH] npiv: Expose fabric_name outside

On Tue, Dec 06, 2011 at 08:42:18PM +0800, Osier Yang wrote: > This patch is to expose the fabric_name of fc_host class, which > might be useful for users who wants to known which fabric the > (v)HBA connects to. > > The patch also adds the missed capabilities' XML schema of scsi_host, > (of course, with fabric_wwn added), and update the documents > (docs/formatnode.html.in) > --- > docs/formatnode.html.in | 7 +++++ > docs/schemas/nodedev.rng | 40 +++++++++++++++++++++++++++++ > src/conf/node_device_conf.c | 3 ++ > src/conf/node_device_conf.h | 1 + > src/node_device/node_device_linux_sysfs.c | 10 +++++++ > 5 files changed, 61 insertions(+), 0 deletions(-) > > diff --git a/docs/formatnode.html.in b/docs/formatnode.html.in > index 126f8de..c04d04d 100644 > --- a/docs/formatnode.html.in > +++ b/docs/formatnode.html.in > @@ -126,6 +126,7 @@ > <dd>A network protocol exposed by the device, where the > attribute<code>type</code> can be "80203" for IEEE > 802.3, or "80211" for various flavors of IEEE 802.11. > +</dd> > </dl> > </dd> > <dt><code>scsi_host</code></dt> > @@ -133,6 +134,12 @@ > <dl> > <dt><code>host</code></dt> > <dd>The SCSI host number.</dd> > +<dt><code>capability</code></dt> > +<dd>Current capabilities include "vports_ops" (indicates > + vport operations are supported) and "fc_host", the later > + implies following sub-elements:<code>wwnn</code>, > +<code>wwpn</code>,<code>fabric_wwn</code>. > +</dd> > </dl> > </dd> > <dt><code>scsi</code></dt> > diff --git a/docs/schemas/nodedev.rng b/docs/schemas/nodedev.rng > index 55191d9..1b9a2d1 100644 > --- a/docs/schemas/nodedev.rng > +++ b/docs/schemas/nodedev.rng > @@ -216,6 +216,35 @@ > </attribute> > </define> > > +<define name='wwn'> > +<data type='string'> > +<param name='pattern'>(0-9a-fA-F){16}</param> > +</data> > +</define> > + > +<define name='capsfchost'> > +<attribute name='type'> > +<value>fc_host</value> > +</attribute> > + > +<element name='wwnn'> > +<ref name='wwn'/> > +</element> > + > +<element name='wwpn'> > +<ref name='wwn'/> > +</element> > + > +<element name='fabric_wwn'> > +<ref name='wwn'/> > +</element> > +</define> > + > +<define name='capsvports'> > +<attribute name='type'> > +<value>vports_ops</value> > +</attribute> > +</define> > > <define name='capscsihost'> > <attribute name='type'> > @@ -225,6 +254,17 @@ > <element name='host'> > <ref name='uint'/> > </element> > + > +<optional> > +<zeroOrMore> > +<element name='capability'> > +<choice> > +<ref name='capsfchost'/> > +<ref name='capsvports'/> > +</choice> > +</element> > +</zeroOrMore> > +</optional> > </define> > > <define name='capscsi'> > diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c > index 9d48ff8..ea6ebde 100644 > --- a/src/conf/node_device_conf.c > +++ b/src/conf/node_device_conf.c > @@ -389,6 +389,8 @@ char *virNodeDeviceDefFormat(const virNodeDeviceDefPtr def) > data->scsi_host.wwnn); > virBufferEscapeString(&buf, "<wwpn>%s</wwpn>\n", > data->scsi_host.wwpn); > + virBufferEscapeString(&buf, "<fabric_wwn>%s</fabric_wwn>\n", > + data->scsi_host.fabric_wwn); > virBufferAddLit(&buf, "</capability>\n"); > } > if (data->scsi_host.flags& VIR_NODE_DEV_CAP_FLAG_HBA_VPORT_OPS) { > @@ -1405,6 +1407,7 @@ void virNodeDevCapsDefFree(virNodeDevCapsDefPtr caps) > case VIR_NODE_DEV_CAP_SCSI_HOST: > VIR_FREE(data->scsi_host.wwnn); > VIR_FREE(data->scsi_host.wwpn); > + VIR_FREE(data->scsi_host.fabric_wwn); > break; > case VIR_NODE_DEV_CAP_SCSI_TARGET: > VIR_FREE(data->scsi_target.name); > diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h > index e317354..e787fc7 100644 > --- a/src/conf/node_device_conf.h > +++ b/src/conf/node_device_conf.h > @@ -141,6 +141,7 @@ struct _virNodeDevCapsDef { > unsigned int host; > char *wwnn; > char *wwpn; > + char *fabric_wwn; > unsigned int flags; > } scsi_host; > struct { > diff --git a/src/node_device/node_device_linux_sysfs.c b/src/node_device/node_device_linux_sysfs.c > index d352800..380be9c 100644 > --- a/src/node_device/node_device_linux_sysfs.c > +++ b/src/node_device/node_device_linux_sysfs.c > @@ -149,10 +149,20 @@ int check_fc_host_linux(union _virNodeDevCapData *d) > retval = -1; > } > > + if (read_wwn(d->scsi_host.host, > + "fabric_name", > +&d->scsi_host.fabric_wwn) == -1) { > + VIR_ERROR(_("Failed to read fabric WWN for host%d"), > + d->scsi_host.host); > + retval = -1; Not your fault, since you're just following existing practice, but the error reporting in these methods seems a little odd, using VIR_ERROR instead of virReportError. Presumably because these codepaths are only ever run in daemon context, not in response to a user API. These days though the daemon code also uses virReportError, so some day we might like to cleanup this code. > + goto out; > + } > + > out: > if (retval == -1) { > VIR_FREE(d->scsi_host.wwnn); > VIR_FREE(d->scsi_host.wwpn); > + VIR_FREE(d->scsi_host.fabric_wwn); > } > VIR_FREE(sysfs_path); > return retval; ACK. The error cleanup can wait till later Daniel