On 08/13/2018 12:26 AM, clem@lse.epita.fr wrote:

From: Clementine Hayat <clem@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@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

On 08/13/2018 12:26 AM, clem@lse.epita.fr wrote:

From: Clementine Hayat <clem@lse.epita.fr>

Signed-off-by: Clementine Hayat <clem@lse.epita.fr> --- src/storage/storage_backend_iscsi_direct.c | 121 ++++++++++++++++++++- 1 file changed, 120 insertions(+), 1 deletion(-)

diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 7bdd39582b..5a7d70f24d 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -41,6 +41,8 @@

#define ISCSI_DEFAULT_TARGET_PORT 3260 #define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000 +#define BLOCK_PER_PACKET 128 +#define VOL_NAME_PREFIX "unit:0:0:"

VIR_LOG_INIT("storage.storage_backend_iscsi_direct");

@@ -225,7 +227,7 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool, { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);

- if (virAsprintf(&vol->name, "unit:0:0:%u", lun) < 0) + if (virAsprintf(&vol->name, "%s%u", VOL_NAME_PREFIX, lun) < 0) return -1; if (virAsprintf(&vol->key, "ip-%s-iscsi-%s-lun-%u", portal, def->source.devices[0].path, lun) < 0) @@ -601,12 +603,129 @@ virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool) return ret; }

+static int +virStorageBackendISCSIDirectGetLun(virStorageVolDefPtr vol, + int *lun) +{ + const char *name = vol->name; + int ret = -1; + + if (!STRPREFIX(name, VOL_NAME_PREFIX)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid volume name %s"), name); + goto cleanup; + } + + name += strlen(VOL_NAME_PREFIX); + if (virStrToLong_i(name, NULL, 10, lun) < 0) + goto cleanup; + + ret = 0; + cleanup: + return ret; +} + +static int +virStorageBackendISCSIDirectVolWipeZero(virStorageVolDefPtr vol, + struct iscsi_context *iscsi) +{ + uint32_t lba = 0; + uint32_t block_size; + uint32_t nb_block; + struct scsi_task *task = NULL; + int lun = 0; + int ret = -1; + unsigned char *data; + + if (virStorageBackendISCSIDirectGetLun(vol, &lun) < 0) + return ret; + if (virISCSIDirectTestUnitReady(iscsi, lun) < 0) + return ret; + if (virISCSIDirectGetVolumeCapacity(iscsi, lun, &block_size, &nb_block)) + return ret; + if (VIR_ALLOC_N(data, block_size * BLOCK_PER_PACKET)) + return ret; + + while (lba < nb_block) { + if (nb_block - lba > block_size * BLOCK_PER_PACKET) { + + if (!(task = iscsi_write10_sync(iscsi, lun, lba, data, + block_size * BLOCK_PER_PACKET, + block_size, 0, 0, 0, 0, 0))) + goto cleanup; + scsi_free_scsi_task(task); + lba += BLOCK_PER_PACKET; + } else { + if (!(task = iscsi_write10_sync(iscsi, lun, lba, data, block_size, + block_size, 0, 0, 0, 0, 0))) + goto cleanup; + scsi_free_scsi_task(task); + lba++; + } + } + + ret = 0; + cleanup: + VIR_FREE(data); + return ret; +} + +static int +virStorageBackenISCSIDirectWipeVol(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + unsigned int algorithm, + unsigned int flags) +{ + struct iscsi_context *iscsi = NULL; + char *portal = NULL;

After my changes in 2/3 this variable is not needed.

+ int ret = -1; + + virCheckFlags(0, -1); + + if (!(iscsi = virStorageBackendISCSIDirectSetConnection(pool, &portal))) + goto cleanup; + + switch ((virStorageVolWipeAlgorithm) algorithm) { + case VIR_STORAGE_VOL_WIPE_ALG_ZERO: + if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to wipe volume %s"), + vol->name); + goto disconnect; + } + break; + case VIR_STORAGE_VOL_WIPE_ALG_TRIM: + case VIR_STORAGE_VOL_WIPE_ALG_NNSA: + case VIR_STORAGE_VOL_WIPE_ALG_DOD: + case VIR_STORAGE_VOL_WIPE_ALG_BSI: + case VIR_STORAGE_VOL_WIPE_ALG_GUTMANN: + case VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER: + case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7: + case VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33: + case VIR_STORAGE_VOL_WIPE_ALG_RANDOM: + case VIR_STORAGE_VOL_WIPE_ALG_LAST: + virReportError(VIR_ERR_INVALID_ARG, _("unsupported algorithm %d"), + algorithm); + goto disconnect;

It's a pity we have to connect even though we know whether requested algorithm is supported or not. However, I'm unable to come up with generic enough code that would work for more than this single case (e.g. when we introduce ALG_RANDOM).

+ } + + ret = 0; + disconnect: + virISCSIDirectDisconnect(iscsi); + iscsi_destroy_context(iscsi); + cleanup: + VIR_FREE(portal); + return ret; +} + + virStorageBackend virStorageBackendISCSIDirect = { .type = VIR_STORAGE_POOL_ISCSI_DIRECT,

.checkPool = virStorageBackendISCSIDirectCheckPool, .findPoolSources = virStorageBackendISCSIDirectFindPoolSources, .refreshPool = virStorageBackendISCSIDirectRefreshPool, + .wipeVol = virStorageBackenISCSIDirectWipeVol, };

int