[libvirt] [PATCH v2 1/5] storage: Check for valid fc_host parent at startup

[Michal Privoznik]

On 10.11.2014 23:45, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1160565
>
> If a 'parent' attribute is provided for the fchost, then at startup
> time check to ensure it is a vport capable scsi_host. If the parent
> is not vport capable, then disallow the startup. The following is the
> expected results:
>
> error: Failed to start pool fc_pool
> error: XML error: parent 'scsi_host2' specified for vHBA is not vport capable
>
> where the XML for the fc_pool is:
>
>      <pool type='scsi'>
>        <name>fc_pool</name>
>        <source>
>          <adapter type='fc_host' parent='scsi_host2' wwnn='5001a4aaf3ca174b' wwpn='5001a4a77192b864'/>
>        </source>
> ...
>
> and 'scsi_host2' is not vport capable.
>
> Providing an incorrect parent and a correct wwnn/wwpn could lead to
> failures at shutdown (deleteVport) where the assumption is the parent
> is for the fchost.
>
> NOTE: If the provided wwnn/wwpn doesn't resolve to an existing scsi_host,
>        then we will be creating one with code (virManageVport) which
>        assumes the parent is vport capable.
>
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>   src/storage/storage_backend_scsi.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 02160bc..549d8db 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -556,6 +556,20 @@ createVport(virStoragePoolSourceAdapter adapter)
>       if (adapter.type != VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST)
>           return 0;
>
> +    /* If a parent was provided, then let's make sure it's vhost capable */
> +    if (adapter.data.fchost.parent) {
> +        if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> +            return -1;
> +
> +        if (!virIsCapableFCHost(NULL, parent_host)) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("parent '%s' specified for vHBA "
> +                             "is not vport capable"),
> +                           adapter.data.fchost.parent);
> +            return -1;
> +        }
> +    }
> +
>       /* This filters either HBA or already created vHBA */
>       if ((name = virGetFCHostNameByWWN(NULL, adapter.data.fchost.wwnn,
>                                         adapter.data.fchost.wwpn))) {
> @@ -565,14 +579,14 @@ createVport(virStoragePoolSourceAdapter adapter)
>
>       if (!adapter.data.fchost.parent &&
>           !(adapter.data.fchost.parent = virFindFCHostCapableVport(NULL))) {
> -         virReportError(VIR_ERR_XML_ERROR, "%s",
> +        virReportError(VIR_ERR_XML_ERROR, "%s",
>                          _("'parent' for vHBA not specified, and "
>                            "cannot find one on this host"));
>            return -1;
> -    }
>
> -    if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> -        return -1;
> +        if (virGetSCSIHostNumber(adapter.data.fchost.parent, &parent_host) < 0)
> +            return -1;
> +    }

This chunk seems odd. After the error is reported, the control jumps out 
from the function, so virGetSCSIHostNumer is not called at all. Did you 
forget to commit something?

>
>       if (virManageVport(parent_host, adapter.data.fchost.wwpn,
>                          adapter.data.fchost.wwnn, VPORT_CREATE) < 0)
>

Michal