[libvirt] [PATCH 2/2] conf: Fix vHBA checkParent logic for pool creation

[Erik Skultety]

On Mon, Jul 03, 2017 at 02:52:06PM -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1458708
>
> When originally designed in commit id '42a021c1', providing the 'parent'
> attribute to the <adapter type='fc_host' wwnn='vHBA_wwnn' wwpn='vHBA_wwpn'/>
> was checked to make sure that the "parent" of the wwnn/wwpn matched that
> of the provided parent "just in case" someone created the node device first,
> then defined the storage pool using that node device afterwards. The result
> is to not perform the vport_create call when the scsi_host represented by
> the wwnn/wwpn already exists since it would fail.
>
> Eventually someone came up with the brilliant idea to provide wwnn/wwpn of
> an HBA instead of a vHBA, e.g. <adapter type='fc_host' wwnn='HBA_wwnn'
> wwpn='HBA_wwpn'/>. This is the same as creating a storage pool backed to
> the HBA that just happens to also be vport (vHBA) capable. The logic to
> bypass the vport_create call was the same as the vHBA code since the wwn's
> already exist. Once that was determined to work, adding in the 'parent'
> attribute caused a problem for the DeleteVport code, which was fixed by
> commit id '2c8e30ee7e'.
>
> The next test tried was providing a valid pair of wwns that would find
> the scsi_host HBA, but providing the wrong name for the 'parent' attribute.
> This caused a different failure because at DeleteVport time if a parent
> is provided it was assumed valid especially since the CreateVport code
> would check that by calling virVHBAPathExists.
>
> So alter the checkParent code to first ensure that the provided parent_name
> is a valid HBA/vHBA, then check if the found scsi_host is an HBA or a vHBA
> and make the appropriate check similar to the check made during DestroyVport.
> This restores some checks that were "lost" during various refactorings such
> as commit id '79ab0935' which altered the return value logic and commit id
> '9fdc8c426' which moved the parent host name validity check, but neglected
> to add a similar check for this odd HBA configuration. As it turns out prior
> to this patch, the checkParent code would fail for an HBA, but that was
> masked by the changed return value checking logic.
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  src/conf/node_device_conf.c | 50 +++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 42 insertions(+), 8 deletions(-)
>
> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
> index e5947e6..d4075b5 100644
> --- a/src/conf/node_device_conf.c
> +++ b/src/conf/node_device_conf.c
> @@ -2268,6 +2268,7 @@ checkParent(virConnectPtr conn,
>              const char *name,
>              const char *parent_name)
>  {
> +    unsigned int host_num;
>      char *scsi_host_name = NULL;
>      char *vhba_parent = NULL;
>      bool retval = false;
> @@ -2278,20 +2279,53 @@ checkParent(virConnectPtr conn,
>      if (!conn)
>          return true;
>
> -    if (virAsprintf(&scsi_host_name, "scsi_%s", name) < 0)
> +    if (virSCSIHostGetNumber(parent_name, &host_num) < 0) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("parent '%s' is not properly formatted"), name);
>          goto cleanup;
> +    }
>
> -    if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> +    if (!virVHBAPathExists(NULL, host_num)) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("parent '%s' is not a vHBA/HBA"), parent_name);
>          goto cleanup;
> +    }
>
> -    if (STRNEQ(parent_name, vhba_parent)) {
> -        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);
> +    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 scsi_host_name is vport capable, then it's an HBA, thus compare
> +     * only against the parent_name; otherwise, as long as the scsi_host_name
> +     * path exists, then the scsi_host_name is a vHBA in which case we need
> +     * to compare against it's parent. */
> +    if (virVHBAIsVportCapable(NULL, host_num)) {
> +        if (STRNEQ(parent_name, scsi_host_name)) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("parent HBA '%s' doesn't match the wwnn/wwpn "
> +                             "scsi_host '%s'"),
> +                           parent_name, scsi_host_name);
> +            goto cleanup;
> +        }
> +    } else {
> +        if (!(vhba_parent = virNodeDeviceGetParentName(conn, scsi_host_name)))
> +            goto cleanup;
> +
> +        if (STRNEQ(parent_name, vhba_parent)) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("parent vHBA '%s' doesn't match the wwnn/wwpn "
> +                             "scsi_host '%s' parent '%s'"),
> +                           parent_name, scsi_host_name, vhba_parent);
> +            goto cleanup;
> +        }
> +    }
> +
> +
>      retval = true;
>
>   cleanup:
> @@ -2333,7 +2367,7 @@ virNodeDeviceCreateVport(virConnectPtr conn,
>      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 (fchost->parent && !checkParent(conn, name, fchost->parent))
>              VIR_FREE(name);

This last hunk actually causes another issue to which I created a BZ prior to
going through your patch (I was just testing 1/2) and instead cooked one on my
own, but since you're addressing it too, I would strip this hunk to a separate
patch, squeezed it in between 1 and 2 and linked
https://bugzilla.redhat.com/show_bug.cgi?id=1472277 to it - I also think that
since this hasn't worked as advertised since commit @08c0ea16fc, this should
go to v3.2.0-maint and onward.

Erik

PS: I still need to do an actual review of this patch though, this was just
something that I spotted right away.