[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 4/4] storage: Disallow usage of the HBA for a fc_host backing




On 07/24/2017 11:00 AM, Erik Skultety wrote:
> On Thu, Jul 20, 2017 at 03:48:49PM -0400, John Ferlan wrote:
>> 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'/>
>>
>> This also alters the caller such that the @parent_name param
>> into checkParent can be NULL so as to confirm that at least
>> the provided wwnn/wwpn found a vHBA instead of an HBA.
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  docs/formatstorage.html.in         | 27 +++++++++++++----------
>>  src/storage/storage_backend_scsi.c | 45 ++++++++++++++++++++++++++++----------
>>  2 files changed, 49 insertions(+), 23 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..c802738 100644
>> --- a/src/storage/storage_backend_scsi.c
>> +++ b/src/storage/storage_backend_scsi.c
>> @@ -231,22 +231,47 @@ checkParent(virConnectPtr conn,
>>      if (!conn)
>>          return true;
>>
>> -    if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
>> -        virReportError(VIR_ERR_XML_ERROR,
>> -                       _("parent '%s' is not properly formatted"),
>> -                       parent_name);
>> +    /* If there's a parent_name, then make sure it's valid */
>> +    if (parent_name) {
>> +        if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
>> +            virReportError(VIR_ERR_XML_ERROR,
>> +                           _("parent '%s' is not properly formatted"),
>> +                           parent_name);
>> +            goto cleanup;
>> +        }
>> +
>> +        if (!virVHBAPathExists(NULL, host_num)) {
>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                           _("parent '%s' is not an fc_host for the wwnn/wwpn"),
>> +                           parent_name);
>> +            goto cleanup;
>> +        }
>> +    }
> 
> ^Here you're handling the device's parent - it's existence
> 
>> +
>> +    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> +        goto cleanup;
>> +
>> +    if (virSCSIHostGetNumber(scsi_host_name, &host_num) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("host name '%s' is not properly formatted"),
>> +                       name);
>>          goto cleanup;
>>      }
>>
>> -    if (!virVHBAPathExists(NULL, host_num)) {
>> +    /* 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,
>> -                       _("parent '%s' is not an fc_host for the wwnn/wwpn"),
>> -                       parent_name);
>> +                       _("the wwnn/wwpn for '%s' are assigned to an HBA"),
>> +                       scsi_host_name);
>>          goto cleanup;
>>      }
> 
> ^Here you're handling the device itself.
> 
>>
>> -    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
>> +    /* No parent name, then no need to get/compare against vhba_parent */
>> +    if (!parent_name) {
>> +        retval = true;
>>          goto cleanup;
>> +    }
>>
> 
> And ^here you're again handling the device's parent - it's !existence and
> perform some further parent device validation in the already existing hunk
> below. Would it make more sense to first verify the device whether it's a vHBA,
> if not then throw an error and then, if @parent_name wasn't supplied, return
> successfully otherwise do both the parent existence check and validation, both
> in a large "else" block. The checks make sense and work as described, I just
> find it more understandable - the logic would be IMHO more continuous, as
> opposed "scattered" (is that even the right term for this? But I think you get
> the picture...)

True - I was also trying to keep diffs to a minimum while also trying to
preserve both sets of error checking while considering patch 3 ordering.

Patch 3 adds the parent_name check above "scsi_host/vhba_parent" because
I didn't want to stick the logic between the virAsprintf and
GetParentName call and it felt awkward to needlessly get vhba_parent
without first checking if @parent_name was going to fail.

But you're right I can do better on this and stop making checkParent be
the slave to too many masters.  I'll push the first 3 and repost
something a bit different that I think will be more palatable.

Thanks -

John

FWIW: I did originally have the logic swapped like shown below, but the
patch 4 diffs got really ugly...

> 
> if (scsi_host_name is vport capable)
>     # it's an HBA
>     error
>     return failure
> 
> if (!parent_name)
>     # no parent to validate
>     return success
> else
>     if (parent doesn't exist)
>         error - not an fc_host
>         return failure
> 
>     if (provided_parent != actual_parent)
>         error - invalid parent
>         return failure
> 
> return success
> 
> Erik.
> 
>>      if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
>>          goto cleanup;
>> @@ -288,9 +313,7 @@ createVport(virConnectPtr conn,
>>       * this pool and we don't have to create the vHBA
>>       */
>>      if ((name = virVHBAGetHostByWWN(NULL, fchost->wwnn, fchost->wwpn))) {
>> -        /* 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))
>> +        if (checkParent(conn, name, fchost->parent))
>>              ret = 0;
>>
>>          goto cleanup;
>> --
>> 2.9.4
>>
>> --
>> libvir-list mailing list
>> libvir-list redhat com
>> https://www.redhat.com/mailman/listinfo/libvir-list


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]