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

Re: [libvirt] [PATCH] storage: add findPoolSources to iscsi_direct pool backend



On 08/02/2018 10:33 AM, clem lse epita fr wrote:
> From: Clementine Hayat <clem lse epita fr>
> 
> Change the SetContext function to be able to take the session type in
> argument.
> Took the function findPoolSources of iscsi backend and wired it to my
> function since the formatting is essentially the same.
> 
> Signed-off-by: Clementine Hayat <clem lse epita fr>
> ---
>  src/storage/storage_backend_iscsi_direct.c | 179 ++++++++++++++++++++-
>  1 file changed, 171 insertions(+), 8 deletions(-)
> 
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index ab192730fb..fc30f2dfac 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -131,7 +131,8 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi,
>  
>  static int
>  virISCSIDirectSetContext(struct iscsi_context *iscsi,
> -                         const char *target_name)
> +                         const char *target_name,
> +                         enum iscsi_session_type session)
>  {
>      if (iscsi_init_transport(iscsi, TCP_TRANSPORT) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -139,13 +140,15 @@ virISCSIDirectSetContext(struct iscsi_context *iscsi,
>                         iscsi_get_error(iscsi));
>          return -1;
>      }
> -    if (iscsi_set_targetname(iscsi, target_name) < 0) {
> -        virReportError(VIR_ERR_INTERNAL_ERROR,
> -                       _("Failed to set target name: %s"),
> -                       iscsi_get_error(iscsi));
> -        return -1;
> +    if (session == ISCSI_SESSION_NORMAL) {
> +        if (iscsi_set_targetname(iscsi, target_name) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to set target name: %s"),
> +                           iscsi_get_error(iscsi));
> +            return -1;
> +        }
>      }
> -    if (iscsi_set_session_type(iscsi, ISCSI_SESSION_NORMAL) < 0) {
> +    if (iscsi_set_session_type(iscsi, session) < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
>                         _("Failed to set session type: %s"),
>                         iscsi_get_error(iscsi));
> @@ -400,6 +403,92 @@ virISCSIDirectDisconnect(struct iscsi_context *iscsi)
>      return 0;
>  }
>  
> +static void
> +virFreeTargets(char **targets, size_t ntargets, char *target)
> +{
> +    size_t i;
> +
> +    VIR_FREE(target);

> +    for (i = 0; i < ntargets; i++)
> +        VIR_FREE(targets[i]);
> +    VIR_FREE(targets);

This is virStringListFreeCount().

> +}
> +
> +static int
> +virISCSIDirectUpdateTargets(struct iscsi_context *iscsi,
> +                            size_t *ntargets,
> +                            char ***targets)
> +{
> +    int ret = -1;
> +    struct iscsi_discovery_address *addr;
> +    struct iscsi_discovery_address *tmp_addr;
> +    size_t tmp_ntargets = 0;
> +    char **tmp_targets = NULL;
> +
> +    if (!(addr = iscsi_discovery_sync(iscsi))) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to discover session: %s"),
> +                       iscsi_get_error(iscsi));
> +        return ret;
> +    }
> +    *ntargets = 0;

1: ^^

> +    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
> +        char *target = NULL;
> +        if (VIR_STRDUP(target, tmp_addr->target_name) < 0) {
> +            virFreeTargets(tmp_targets, tmp_ntargets, NULL);

Instead of calling this function in every failure path, how about
dissolving it under clean up label?

> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to allocate memory for: %s"),
> +                           tmp_addr->target_name);

No need to set error message. VIR_STRDUP() reports OOM.

> +            goto cleanup;
> +        }
> +
> +        if (VIR_APPEND_ELEMENT(tmp_targets, tmp_ntargets, target) < 0) {
> +            virFreeTargets(tmp_targets, tmp_ntargets, target);
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to append to the list element: %s"),
> +                           tmp_addr->target_name);


No need

> +            goto cleanup;
> +        }
> +
> +    }
> +
> +    if (tmp_ntargets) {

So I'd just drop this check and replace [1] with an empty line.

> +        *targets = tmp_targets;
> +        *ntargets = tmp_ntargets;
> +    }
> +
> +    ret = 0;
> + cleanup:
> +    iscsi_free_discovery_data(iscsi, addr);
> +    return ret;
> +}
> +
> +static int
> +virISCSIDirectScanTargets(char *initiator_iqn,
> +                          char *portal,
> +                          size_t *ntargets,
> +                          char ***targets)
> +{
> +    struct iscsi_context *iscsi = NULL;
> +    int ret = -1;
> +
> +    if (!(iscsi = virISCSIDirectCreateContext(initiator_iqn)))
> +        goto cleanup;
> +    if (virISCSIDirectSetContext(iscsi, NULL, ISCSI_SESSION_DISCOVERY) < 0)
> +        goto cleanup;
> +    if (virISCSIDirectConnect(iscsi, portal) < 0)
> +        goto cleanup;
> +    if (virISCSIDirectUpdateTargets(iscsi, ntargets, targets) < 0)
> +        goto disconnect;
> +
> +    ret = 0;
> + disconnect:
> +    virISCSIDirectDisconnect(iscsi);
> + cleanup:
> +    iscsi_destroy_context(iscsi);
> +    return ret;
> +}
> +
>  static int
>  virStorageBackendISCSIDirectCheckPool(virStoragePoolObjPtr pool,
>                                        bool *isActive)
> @@ -408,6 +497,79 @@ virStorageBackendISCSIDirectCheckPool(virStoragePoolObjPtr pool,
>      return 0;
>  }
>  
> +static char *
> +virStorageBackendISCSIDirectFindPoolSources(const char *srcSpec,
> +                                            unsigned int flags)
> +{
> +    virStoragePoolSourcePtr source = NULL;
> +    size_t ntargets = 0;
> +    char **targets = NULL;
> +    char *ret = NULL;
> +    size_t i;
> +    virStoragePoolSourceList list = {
> +        .type = VIR_STORAGE_POOL_ISCSI_DIRECT,
> +        .nsources = 0,
> +        .sources = NULL
> +    };
> +    char *portal = NULL;
> +
> +    virCheckFlags(0, NULL);
> +
> +    if (!srcSpec) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("hostname must be specified for iscsi sources"));
> +        return NULL;
> +    }
> +
> +    if (!(source = virStoragePoolDefParseSourceString(srcSpec, list.type)))
> +        return NULL;
> +
> +    if (source->nhost != 1) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("Expected exactly 1 host for the storage pool"));
> +        goto cleanup;
> +    }

2: here ^^

> +
> +    if (!(portal = virStorageBackendISCSIDirectPortal(source)))
> +        goto cleanup;
> +
> +    if (virISCSIDirectScanTargets(source->initiator.iqn, portal, &ntargets, &targets) < 0)
> +        goto cleanup;

so if source->initiator.iqn is NULL (because it wasn't provided by user)
then libiscsi crashes us instantly. You need to check for it somewhere
around [2] (where you already do another checks).

The same problem exists when starting new pool.

> +
> +    if (VIR_ALLOC_N(list.sources, ntargets) < 0)
> +        goto cleanup;
> +
> +    for (i = 0; i < ntargets; i++) {
> +        if (VIR_ALLOC_N(list.sources[i].devices, 1) < 0 ||
> +            VIR_ALLOC_N(list.sources[i].hosts, 1) < 0)
> +            goto cleanup;
> +        list.sources[i].nhost = 1;
> +        list.sources[i].hosts[0] = source->hosts[0];
> +        list.sources[i].initiator = source->initiator;
> +        list.sources[i].ndevice = 1;
> +        list.sources[i].devices[0].path = targets[i];
> +        list.nsources++;
> +    }
> +
> +    if (!(ret = virStoragePoolSourceListFormat(&list)))
> +        goto cleanup;
> +
> + cleanup:
> +    if (list.sources) {
> +        for (i = 0; i < ntargets; i++) {
> +            VIR_FREE(list.sources[i].hosts);
> +            VIR_FREE(list.sources[i].devices);
> +        }
> +        VIR_FREE(list.sources);
> +    }
> +    for (i = 0; i < ntargets; i++)
> +        VIR_FREE(targets[i]);
> +    VIR_FREE(targets);
> +    VIR_FREE(portal);
> +    virStoragePoolSourceFree(source);
> +    return ret;
> +}
> +
>  static int
>  virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
>  {
> @@ -422,7 +584,7 @@ virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
>          goto cleanup;
>      if (virStorageBackendISCSIDirectSetAuth(iscsi, &def->source) < 0)
>          goto cleanup;
> -    if (virISCSIDirectSetContext(iscsi, def->source.devices[0].path) < 0)
> +    if (virISCSIDirectSetContext(iscsi, def->source.devices[0].path, ISCSI_SESSION_NORMAL) < 0)
>          goto cleanup;
>      if (virISCSIDirectConnect(iscsi, portal) < 0)
>          goto cleanup;
> @@ -442,6 +604,7 @@ virStorageBackend virStorageBackendISCSIDirect = {
>      .type = VIR_STORAGE_POOL_ISCSI_DIRECT,
>  
>      .checkPool = virStorageBackendISCSIDirectCheckPool,
> +    .findPoolSources = virStorageBackendISCSIDirectFindPoolSources,
>      .refreshPool = virStorageBackendISCSIDirectRefreshPool,
>  };
>  
> 

Otherwise looking good. Fixing all the issues I've found locally and
ACKing. Will push after the release.

Michal


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