[libvirt] [PATCH 1/2] storage: Fix autostart of pool with "fc_host" type adapter

[John Ferlan]

On 01/06/2014 05:19 AM, Osier Yang wrote:
> The "checkPool" is a bit different for pool with "fc_host"
> type source adapter, since the vHBA it's based on might be
> not created yet (it's created by "startPool", which is
> involked after "checkPool" in storageDriverAutostart). So it
> should not fail, otherwise the "autostart" of the pool will
> fail either.
> 
> The problem is easy to reproduce:
>     * Enable "autostart" for the pool
>     * Restart libvirtd service
>     * Check the pool's state
> ---
>  src/storage/storage_backend_scsi.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 

Not sure this is the right thing to do. With this change it doesn't
matter what getAdapterName() returns for fc_host's...

The getAdapterName() already has some code to specifically handle
VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST. The call to
virGetFCHostNameByWWN() is similar to createVport() which is called by
the 'start' path, except that the createVport() path will be happy if
the name already exists.

Since it seems the checkPool is meant to initialize things (from reading
the error message in the calling function), why not create the vPort in
the check function? It's a bit more refactoring that perhaps initially
desired, although having a vport hanging around unused may not be quite
right either.

Another option would be to have the check function perform "enough
initialization" or "checks" to make it more likely that the start path
will succeed.

Looking at the code does cause me to wonder what happens in the
"existing" code if the vport was created when the CheckPool function was
called returning the NameByWWN() in the 'name' field. The subsequent
getHostNumber() call uses the 'name' instead of what the start path does
using the parent value.

It seems the "check" for fc_host and non-fc_host is different enough
that the distinguishment needs to be in the check routine rather than
hidden in the getAdapterName() function.


John

> diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c
> index 6f86ffc..93039c1 100644
> --- a/src/storage/storage_backend_scsi.c
> +++ b/src/storage/storage_backend_scsi.c
> @@ -702,8 +702,18 @@ virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>      *isActive = false;
>  
> -    if (!(name = getAdapterName(pool->def->source.adapter)))
> -        return -1;
> +    if (!(name = getAdapterName(pool->def->source.adapter))) {
> +        /* It's normal for the pool with "fc_host" type source
> +         * adapter fails to get the adapter name, since the vHBA
> +         * the adapter based on might be not created yet.
> +         */
> +        if (pool->def->source.adapter.type ==
> +            VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
> +            return 0;
> +        } else {
> +            return -1;
> +        }
> +    }
>  
>      if (getHostNumber(name, &host) < 0)
>          goto cleanup;
>