[libvirt] [PATCH v3 2/3] storage: add SetConnection to iscsi-direct backend

[Michal Prívozník]

On 08/13/2018 12:26 AM, clem at lse.epita.fr wrote:
> From: Clementine Hayat <clem at lse.epita.fr>
> 
> The code to set the connection and connect using libiscsi will always be
> the same. Add function to avoid code duplication.
> 
> Signed-off-by: Clementine Hayat <clem at lse.epita.fr>
> ---
>  src/storage/storage_backend_iscsi_direct.c | 38 +++++++++++++++-------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 62b7e0d8bc..7bdd39582b 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -557,23 +557,37 @@ virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
>      return ret;
>  }
>  
> +struct iscsi_context *
> +virStorageBackendISCSIDirectSetConnection(virStoragePoolObjPtr pool,
> +                                          char **portal)

The function needs to be static. You're not exporting it, only using it
within this file.
Also, right now the only caller of the function needs to know the
portal, but that will not always be the case. So I think we can have the
function accepting NULL for *portal meaning caller doesn't care what the
portal is.

> +{
> +    virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> +    struct iscsi_context *iscsi = NULL;
> +
> +    if (!(*iscsi = virISCSIDirectCreateContext(def->source.initiator.iqn)))
> +        return iscsi;

This doesn't look right. We don't know what iscsi_context look like, we
shouldn't dereference it. Have you tried to compile this patch? ;-)

> +    if (!(*portal = virStorageBackendISCSIDirectPortal(&def->source)))
> +        goto error ;
> +    if (virStorageBackendISCSIDirectSetAuth(*iscsi, &def->source) < 0)
> +        goto error ;
> +    if (virISCSIDirectSetContext(*iscsi, def->source.devices[0].path, ISCSI_SESSION_NORMAL) < 0)
> +        goto error ;
> +    if (virISCSIDirectConnect(*iscsi, *portal) < 0)
> +        goto error ;
> +    return iscsi;
> +
> + error:
> +    iscsi_destroy_context(iscsi);
> +    return iscsi;
> +}
> +
>  static int
>  virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
>  {
> -    virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>      struct iscsi_context *iscsi = NULL;
>      char *portal = NULL;
>      int ret = -1;
> -
> -    if (!(iscsi = virISCSIDirectCreateContext(def->source.initiator.iqn)))
> -        goto cleanup;
> -    if (!(portal = virStorageBackendISCSIDirectPortal(&def->source)))
> -        goto cleanup;
> -    if (virStorageBackendISCSIDirectSetAuth(iscsi, &def->source) < 0)
> -        goto cleanup;
> -    if (virISCSIDirectSetContext(iscsi, def->source.devices[0].path, ISCSI_SESSION_NORMAL) < 0)
> -        goto cleanup;
> -    if (virISCSIDirectConnect(iscsi, portal) < 0)
> +    if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, &portal)))
>          goto cleanup;
>      if (virISCSIDirectReportLuns(pool, iscsi, portal) < 0)
>          goto disconect;
> @@ -581,9 +595,9 @@ virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
>      ret = 0;
>   disconect:
>      virISCSIDirectDisconnect(iscsi);
> +    iscsi_destroy_context(iscsi);
>   cleanup:
>      VIR_FREE(portal);
> -    iscsi_destroy_context(iscsi);
>      return ret;
>  }
>  
> 

Michal