[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 Thu, Aug 02, 2018 at 10:33:27AM +0200, 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>
> ---

There should be a note that this is a follow-up to the series sent separately.

>  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);
> +}

You don't really need ^this method, see below.

> +
> +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;
> +    for (tmp_addr = addr; tmp_addr; tmp_addr = tmp_addr->next) {
> +        char *target = NULL;

So, Pavel or Michal might have further suggestions regarding the design, I'm
only going to comment on a few things related to our coding style that caught
my eye. Anyhow, if ^this hunk is to stay, then target could be declared as
VIR_AUTOFREE, thus not having to free it explicitly.

> +        if (VIR_STRDUP(target, tmp_addr->target_name) < 0) {
> +            virFreeTargets(tmp_targets, tmp_ntargets, NULL);

Since tmp_targets is char **, you can use the already existing
virStringListFreeCount method. Also, if you use VIR_STEAL_PTR as I'm suggesting
below, freeing can be moved to the cleanup section.

> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("Failed to allocate memory for: %s"),
> +                           tmp_addr->target_name);
> +            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);
> +            goto cleanup;
> +        }
> +
> +    }
> +
> +    if (tmp_ntargets) {

Do we have to make this conditional? I mean, if there were any targets, you'd
rewrite the caller-provided pointer anyway, so rewriting it to NULL on success
because there were no targets is IMHO fine. Besides, you already reset
*ntargets to 0 above, so I think the conditional should be dropped.

> +        *targets = tmp_targets;

^This should become VIR_STEAL_PTR(*targets, tmp_targets); instead.

> +        *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;
> +    }
> +
> +    if (!(portal = virStorageBackendISCSIDirectPortal(source)))
> +        goto cleanup;
> +
> +    if (virISCSIDirectScanTargets(source->initiator.iqn, portal, &ntargets, &targets) < 0)
> +        goto cleanup;
> +
> +    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)

Plain VIR_ALLOC will do just fine ^here.

> +            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);

Hmm, I'm wondering if it weren't beneficial to have a
virStoragePoolSourceListFree wrapper for ^this.

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

virStringListFreeCount ^here as well...

> +    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,
>  };
>
> --
> 2.18.0
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


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