[libvirt] [PATCH] storage: add findPoolSources to iscsi_direct pool backend
Erik Skultety
eskultet at redhat.com
Thu Aug 2 12:28:56 UTC 2018
On Thu, Aug 02, 2018 at 10:33:27AM +0200, clem at lse.epita.fr wrote:
> From: Clementine Hayat <clem at 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 at 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
More information about the libvir-list
mailing list