12:52 p.m.
Disallow providing the wwnn/wwpn of the HBA in the adapter XML:
<adapter type='fc_host' [parent='scsi_hostN']
wwnn='HBA_wwnn'
wwpn='HBA_wwpn'/>
This should be considered a configuration error since a vHBA
would not be created. In order to use the HBA as the backing the
following XML should be used:
<adapter type='scsi_host' name='scsi_hostN'/>
So add a check prior to the checkParent call to validate that
the provided wwnn/wwpn resolves to a vHBA and not an HBA.
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
This used to be patch 4 of the series:
https://www.redhat.com/archives/libvir-list/2017-July/msg00837.html
which while the series was in it's 3rd review cycle, this particular
patch was new to the series and thus becomes a v2 since the three other
patches from the series were pushed.
docs/formatstorage.html.in | 27 +++++++++--------
src/storage/storage_backend_scsi.c | 62 +++++++++++++++++++++++++++++++++-----
2 files changed, 69 insertions(+), 20 deletions(-)
diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in
index 4946ddf..27578e8 100644
--- a/docs/formatstorage.html.in
+++ b/docs/formatstorage.html.in
@@ -207,18 +207,21 @@
</dl>
<dl>
<dt><code>wwnn</code> and
<code>wwpn</code></dt>
- <dd>The "World Wide Node Name" (<code>wwnn</code>)
and "World Wide
- Port Name" (<code>wwpn</code>) are used by the
"fc_host" adapter
- to uniquely identify the device in the Fibre Channel storage fabric
- (the device can be either a HBA or vHBA). Both wwnn and wwpn should
- be specified. Use the command 'virsh nodedev-dumpxml' to determine
- how to set the values for the wwnn/wwpn of a (v)HBA. The wwnn and
- wwpn have very specific numerical format requirements based on the
- hypervisor being used, thus care should be taken if you decide to
- generate your own to follow the standards; otherwise, the pool
- will fail to start with an opaque error message indicating failure
- to write to the vport_create file during vport create/delete due
- to "No such file or directory".
+ <dd>The required "World Wide Node Name"
(<code>wwnn</code>) and
+ "World Wide Port Name" (<code>wwpn</code>) are used by
the
+ "fc_host" adapter to uniquely identify the vHBA device in the
+ Fibre Channel storage fabric. If the vHBA device already exists
+ as a Node Device, then libvirt will use it; otherwise, the vHBA
+ will be created using the provided values. It is considered a
+ configuration error use the values from the HBA as those would
+ be for a "scsi_host" <code>type</code> pool instead.
The
+ <code>wwnn</code> and <code>wwpn</code> have very
specific
+ format requirements based on the hypervisor being used, thus
+ care should be taken if you decide to generate your own to
+ follow the standards; otherwise, the pool will fail to start
+ with an opaque error message indicating failure to write to
+ the vport_create file during vport create/delete due to
+ "No such file or directory".
<span class="since">Since 1.0.4</span>
</dd>
</dl>
diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
index af12889..8892822 100644
--- a/src/storage/storage_backend_scsi.c
+++ b/src/storage/storage_backend_scsi.c
@@ -211,21 +211,66 @@ getAdapterName(virStorageAdapterPtr adapter)
}
+/**
+ * @name: Name from a wwnn/wwpn lookup
+ *
+ * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
+ * an HBA as that should be a configuration error. It's only possible to
+ * use an existing wwnn/wwpn of a vHBA because that's what someone would
+ * have created using the node device create functionality. Using the
+ * HBA "just because" it has a wwnn/wwpn and the characteristics of a
+ * vHBA is just not valid
+ *
+ * Returns the @scsi_host_name to be used or NULL on errror
+ */
+static char *
+checkName(const char *name)
+{
+ char *scsi_host_name = NULL;
+ unsigned int host_num;
+
+ if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ return NULL;
+
+ if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("host name '%s' is not properly formatted"),
+ name);
+ goto error;
+ }
+
+ /* If scsi_host_name is vport capable, then it's an HBA. This is
+ * a configuration error as the wwnn/wwpn should only be for a vHBA */
+ if (virVHBAIsVportCapable(NULL, host_num)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("the wwnn/wwpn for '%s' are assigned to an
HBA"),
+ scsi_host_name);
+ goto error;
+ }
+
+ return scsi_host_name;
+
+ error:
+ VIR_FREE(scsi_host_name);
+ return NULL;
+}
+
+
/*
* Using the host# name found via wwnn/wwpn lookup in the fc_host
* sysfs tree to get the parent 'scsi_host#' to ensure it matches.
*/
static bool
checkParent(virConnectPtr conn,
- const char *name,
+ const char *scsi_host_name,
const char *parent_name)
{
unsigned int host_num;
- char *scsi_host_name = NULL;
char *vhba_parent = NULL;
bool retval = false;
- VIR_DEBUG("conn=%p, name=%s, parent_name=%s", conn, name, parent_name);
+ VIR_DEBUG("conn=%p, scsi_host_name=%s, parent_name=%s",
+ conn, scsi_host_name, parent_name);
/* autostarted pool - assume we're OK */
if (!conn)
@@ -245,9 +290,6 @@ checkParent(virConnectPtr conn,
goto cleanup;
}
- if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
- goto cleanup;
-
if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
goto cleanup;
@@ -255,7 +297,7 @@ checkParent(virConnectPtr conn,
virReportError(VIR_ERR_XML_ERROR,
_("Parent attribute '%s' does not match parent
'%s' "
"determined for the '%s' wwnn/wwpn lookup."),
- parent_name, vhba_parent, name);
+ parent_name, vhba_parent, scsi_host_name);
goto cleanup;
}
@@ -263,7 +305,6 @@ checkParent(virConnectPtr conn,
cleanup:
VIR_FREE(vhba_parent);
- VIR_FREE(scsi_host_name);
return retval;
}
@@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
virStorageAdapterFCHostPtr fchost)
{
char *name = NULL;
+ char *scsi_host_name = NULL;
virStoragePoolFCRefreshInfoPtr cbdata = NULL;
virThread thread;
int ret = -1;
@@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
* this pool and we don't have to create the vHBA
*/
if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+ if (!(scsi_host_name = checkName(name)))
+ goto cleanup;
+
/* If a parent was provided, let's make sure the 'name' we've
* retrieved has the same parent. If not this will cause failure. */
if (!fchost->parent || checkParent(conn, name, fchost->parent))
@@ -333,6 +378,7 @@ createVport(virConnectPtr conn,
cleanup:
VIR_FREE(name);
+ VIR_FREE(scsi_host_name);
return ret;
}
--
2.9.4
2:45 a.m.
+/**
+ * @name: Name from a wwnn/wwpn lookup
+ *
+ * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
+ * an HBA as that should be a configuration error. It's only possible to
+ * use an existing wwnn/wwpn of a vHBA because that's what someone would
+ * have created using the node device create functionality. Using the
+ * HBA "just because" it has a wwnn/wwpn and the characteristics of a
+ * vHBA is just not valid
+ *
+ * Returns the @scsi_host_name to be used or NULL on errror
+ */
+static char *
+checkName(const char *name)
+{
+ char *scsi_host_name = NULL;
+ unsigned int host_num;
+
+ if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
+ return NULL;
+
+ if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("host name '%s' is not properly formatted"),
+ name);
+ goto error;
+ }
+
+ /* If scsi_host_name is vport capable, then it's an HBA. This is
+ * a configuration error as the wwnn/wwpn should only be for a vHBA */
+ if (virVHBAIsVportCapable(NULL, host_num)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("the wwnn/wwpn for '%s' are assigned to an
HBA"),
+ scsi_host_name);
+ goto error;
+ }
+
+ return scsi_host_name;
+
+ error:
+ VIR_FREE(scsi_host_name);
+ return NULL;
+}
+
...
@@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
virStorageAdapterFCHostPtr fchost)
{
char *name = NULL;
+ char *scsi_host_name = NULL;
virStoragePoolFCRefreshInfoPtr cbdata = NULL;
virThread thread;
int ret = -1;
@@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
* this pool and we don't have to create the vHBA
*/
if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
+ if (!(scsi_host_name = checkName(name)))
+ goto cleanup;
Ok, so I learn from my mistakes, do you plan on building upon this in the
foreseeable future with a follow-up series you haven't sent yet, but have on a
local branch? Because if not, I don't really see a point in returning a string
which only gets free'd on the function exit - checkName should probably become
something like "isVHBA" and return boolean. And even if you do have a follow-up
on a local branch, I still think that converting the return value should be
part of that series, solely because until then, @scsi_host_name wouldn't have
any meaning.
Erik
6:36 a.m.
On 07/25/2017 03:45 AM, Erik Skultety wrote:
> +/**
> + * @name: Name from a wwnn/wwpn lookup
> + *
> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
> + * an HBA as that should be a configuration error. It's only possible to
> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
> + * have created using the node device create functionality. Using the
> + * HBA "just because" it has a wwnn/wwpn and the characteristics of a
> + * vHBA is just not valid
> + *
> + * Returns the @scsi_host_name to be used or NULL on errror
> + */
> +static char *
> +checkName(const char *name)
> +{
> + char *scsi_host_name = NULL;
> + unsigned int host_num;
> +
> + if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> + return NULL;
> +
> + if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("host name '%s' is not properly
formatted"),
> + name);
> + goto error;
> + }
> +
> + /* If scsi_host_name is vport capable, then it's an HBA. This is
> + * a configuration error as the wwnn/wwpn should only be for a vHBA */
> + if (virVHBAIsVportCapable(NULL, host_num)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("the wwnn/wwpn for '%s' are assigned to an
HBA"),
> + scsi_host_name);
> + goto error;
> + }
> +
> + return scsi_host_name;
> +
> + error:
> + VIR_FREE(scsi_host_name);
> + return NULL;
> +}
> +
...
> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
> virStorageAdapterFCHostPtr fchost)
> {
> char *name = NULL;
> + char *scsi_host_name = NULL;
> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
> virThread thread;
> int ret = -1;
> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
> * this pool and we don't have to create the vHBA
> */
> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
> + if (!(scsi_host_name = checkName(name)))
> + goto cleanup;
Ok, so I learn from my mistakes, do you plan on building upon this in the
foreseeable future with a follow-up series you haven't sent yet, but have on a
local branch? Because if not, I don't really see a point in returning a string
which only gets free'd on the function exit - checkName should probably become
something like "isVHBA" and return boolean. And even if you do have a
follow-up
on a local branch, I still think that converting the return value should be
part of that series, solely because until then, @scsi_host_name wouldn't have
any meaning.
Erik
No this is it - I want to stop thinking about NPIV for a while... I
returned the 'scsi_host_name' string because in my mind I had passed it
to checkParent, but apparently I didn't do that, sigh.
Does that make more sense now?
John
7:49 a.m.
On Tue, Jul 25, 2017 at 07:36:48AM -0400, John Ferlan wrote:
On 07/25/2017 03:45 AM, Erik Skultety wrote:
>> +/**
>> + * @name: Name from a wwnn/wwpn lookup
>> + *
>> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
>> + * an HBA as that should be a configuration error. It's only possible to
>> + * use an existing wwnn/wwpn of a vHBA because that's what someone would
>> + * have created using the node device create functionality. Using the
>> + * HBA "just because" it has a wwnn/wwpn and the characteristics of
a
>> + * vHBA is just not valid
>> + *
>> + * Returns the @scsi_host_name to be used or NULL on errror
s/errror/error
...
>> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
>> virStorageAdapterFCHostPtr fchost)
>> {
>> char *name = NULL;
>> + char *scsi_host_name = NULL;
>> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
>> virThread thread;
>> int ret = -1;
>> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
>> * this pool and we don't have to create the vHBA
>> */
>> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn)))
{
>> + if (!(scsi_host_name = checkName(name)))
>> + goto cleanup;
>
> Ok, so I learn from my mistakes, do you plan on building upon this in the
> foreseeable future with a follow-up series you haven't sent yet, but have on a
> local branch? Because if not, I don't really see a point in returning a string
> which only gets free'd on the function exit - checkName should probably become
> something like "isVHBA" and return boolean. And even if you do have a
follow-up
> on a local branch, I still think that converting the return value should be
> part of that series, solely because until then, @scsi_host_name wouldn't have
> any meaning.
>
> Erik
>
No this is it - I want to stop thinking about NPIV for a while... I
returned the 'scsi_host_name' string because in my mind I had passed it
to checkParent, but apparently I didn't do that, sigh.
Does that make more sense now?
Indeed, to be honest I missed the connection between name and scsi_host_name
and thought, yeah ok makes sense (probably have seen quite a lot NPIV lately
myself)....now that I see it a bit more clearly, it still left me wondering if
it wouldn't make more sense to move the scsi_host_name formatting part to
createVport (you free it here after all) right after you pass @name to
checkName/isVHBA (whatever we settle on):
- you don't need to format it prior to your checkName, since backwards
compatibility takes care of eating "hostX" nicely
- you also don't need to report any error when virSCSIHostGetNumber fails,
since one gets formatted already and it didn't bring much value
-> then, right after that call you actually format the "scsi_" name since
you
really need it to traverse the list of devices and find the parent in
checkParent.
Erik
John
8:55 a.m.
On 07/25/2017 08:49 AM, Erik Skultety wrote:
On Tue, Jul 25, 2017 at 07:36:48AM -0400, John Ferlan wrote:
>
>
> On 07/25/2017 03:45 AM, Erik Skultety wrote:
>>> +/**
>>> + * @name: Name from a wwnn/wwpn lookup
>>> + *
>>> + * Validate that the @name fetched from the wwnn/wwpn is a vHBA and not
>>> + * an HBA as that should be a configuration error. It's only possible
to
>>> + * use an existing wwnn/wwpn of a vHBA because that's what someone
would
>>> + * have created using the node device create functionality. Using the
>>> + * HBA "just because" it has a wwnn/wwpn and the characteristics
of a
>>> + * vHBA is just not valid
>>> + *
>>> + * Returns the @scsi_host_name to be used or NULL on errror
s/errror/error
...
OK
>>> @@ -275,6 +316,7 @@ createVport(virConnectPtr conn,
>>> virStorageAdapterFCHostPtr fchost)
>>> {
>>> char *name = NULL;
>>> + char *scsi_host_name = NULL;
>>> virStoragePoolFCRefreshInfoPtr cbdata = NULL;
>>> virThread thread;
>>> int ret = -1;
>>> @@ -288,6 +330,9 @@ createVport(virConnectPtr conn,
>>> * this pool and we don't have to create the vHBA
>>> */
>>> if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn,
fchost->wwpn))) {
>>> + if (!(scsi_host_name = checkName(name)))
>>> + goto cleanup;
>>
>> Ok, so I learn from my mistakes, do you plan on building upon this in the
>> foreseeable future with a follow-up series you haven't sent yet, but have on
a
>> local branch? Because if not, I don't really see a point in returning a
string
>> which only gets free'd on the function exit - checkName should probably
become
>> something like "isVHBA" and return boolean. And even if you do have a
follow-up
>> on a local branch, I still think that converting the return value should be
>> part of that series, solely because until then, @scsi_host_name wouldn't
have
>> any meaning.
>>
>> Erik
>>
>
> No this is it - I want to stop thinking about NPIV for a while... I
> returned the 'scsi_host_name' string because in my mind I had passed it
> to checkParent, but apparently I didn't do that, sigh.
>
> Does that make more sense now?
Indeed, to be honest I missed the connection between name and scsi_host_name
and thought, yeah ok makes sense (probably have seen quite a lot NPIV lately
myself)....now that I see it a bit more clearly, it still left me wondering if
it wouldn't make more sense to move the scsi_host_name formatting part to
createVport (you free it here after all) right after you pass @name to
That's fine...
checkName/isVHBA (whatever we settle on):
- you don't need to format it prior to your checkName, since backwards
compatibility takes care of eating "hostX" nicely
- you also don't need to report any error when virSCSIHostGetNumber fails,
since one gets formatted already and it didn't bring much value
One would hope it doesn't fail...
Your suggestions make checkName much cleaner:
if (virSCSIHostGetNumber(name, &host_num) &&
virVHBAIsVportCapable(NULL, host_num))
return true;
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("the wwnn/wwpn for '%s' are assigned to an HBA"),
name);
return false;
and only add :
if (!(checkName(name)))
goto cleanup
to the createVport
The @name to @scsi_host_name can return to checkParent. I retest and
repost shortly.
Tks -
John
-> then, right after that call you actually format the "scsi_" name since
you
really need it to traverse the list of devices and find the parent in
checkParent.
Erik
>
> John
9:45 a.m.
One would hope it doesn't fail...
Even if it does, virSCSIHostGetNumber does report an error already.
Your suggestions make checkName much cleaner:
if (virSCSIHostGetNumber(name, &host_num) &&
virVHBAIsVportCapable(NULL, host_num))
return true;
if virSCSIHostGetNumber fails, it will report an error on its own but you'll
also format the one below which wouldn't be true, because at that point you
still don't know whether wwnn/wwpn refer to either an HBA or a vHBA
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("the wwnn/wwpn for '%s' are assigned to an HBA"),
name);
return false;
and only add :
if (!(checkName(name)))
goto cleanup
to the createVport
The @name to @scsi_host_name can return to checkParent. I retest and
I'm fine with ^this change.
Erik
2707
days inactive
2708
days old
5 comments
2 participants
participants (2)
-
Erik Skultety -
John Ferlan