On 08/02/2018 10:33 AM, c...@lse.epita.fr wrote: > From: Clementine Hayat <c...@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 <c...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list