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

Re: [libvirt] [PATCH] storage: add wipeVol to iscsi-direct storage backend



On 08/10/2018 01:12 PM, clem lse epita fr wrote:
> From: Clementine Hayat <clem lse epita fr>
> 
> Change set volume capacity to get volume capacity to avoid code
> duplicate.
> 
> Signed-off-by: Clementine Hayat <clem lse epita fr>
> ---
> 
> Set BLOCK_PER_PACKET to 128. Not sure about this value. Should be
> potentially tuned.
> 
>  src/storage/storage_backend_iscsi_direct.c | 152 +++++++++++++++++++--
>  1 file changed, 143 insertions(+), 9 deletions(-)

This fails 'make syntax-check'. You should see an automated e-mail in a while (hopefully patchew is up and running).

> 
> diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c
> index 0764356b62..094425c101 100644
> --- a/src/storage/storage_backend_iscsi_direct.c
> +++ b/src/storage/storage_backend_iscsi_direct.c
> @@ -41,6 +41,7 @@
>  
>  #define ISCSI_DEFAULT_TARGET_PORT 3260
>  #define VIR_ISCSI_TEST_UNIT_TIMEOUT 30 * 1000
> +#define BLOCK_PER_PACKET 128
>  
>  VIR_LOG_INIT("storage.storage_backend_iscsi_direct");
>  
> @@ -237,13 +238,13 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
>  }
>  
>  static int
> -virISCSIDirectSetVolumeCapacity(struct iscsi_context *iscsi,
> -                                virStorageVolDefPtr vol,
> -                                int lun)
> +virISCSIDirectGetVolumeCapacity(struct iscsi_context *iscsi,
> +                                int lun,
> +                                uint32_t *block_size,
> +                                uint32_t *nb_block)
>  {
>      struct scsi_task *task = NULL;
>      struct scsi_inquiry_standard *inq = NULL;
> -    long long size = 0;
>      int ret = -1;
>  
>      if (!(task = iscsi_inquiry_sync(iscsi, lun, 0, 0, 64)) ||
> @@ -282,10 +283,8 @@ virISCSIDirectSetVolumeCapacity(struct iscsi_context *iscsi,
>              goto cleanup;
>          }
>  
> -        size  = rc10->block_size;
> -        size *= rc10->lba;
> -        vol->target.capacity = size;
> -        vol->target.allocation = size;
> +	*block_size  = rc10->block_size;

This is one problem that syntax-check raises. We use spaces, not TABs.

> +        *nb_block = rc10->lba;
>  
>      }
>  
> @@ -303,6 +302,8 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool,
>  {
>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>      virStorageVolDefPtr vol = NULL;
> +    uint32_t block_size;
> +    uint32_t nb_block;
>      int ret = -1;
>  
>      virStoragePoolObjClearVols(pool);
> @@ -314,9 +315,12 @@ virISCSIDirectRefreshVol(virStoragePoolObjPtr pool,
>  
>      vol->type = VIR_STORAGE_VOL_NETWORK;
>  
> -    if (virISCSIDirectSetVolumeCapacity(iscsi, vol, lun) < 0)
> +    if (virISCSIDirectGetVolumeCapacity(iscsi, lun,
> +                                        &block_size, &nb_block) < 0)
>          goto cleanup;
>  
> +    vol->target.capacity = block_size * nb_block;
> +    vol->target.allocation = block_size * nb_block;
>      def->capacity += vol->target.capacity;
>      def->allocation += vol->target.allocation;
>  
> @@ -584,12 +588,142 @@ virStorageBackendISCSIDirectRefreshPool(virStoragePoolObjPtr pool)
>      return ret;
>  }
>  
> +static int
> +virStorageBackendISCSIDirectGetLun(virStorageVolDefPtr vol,
> +                                   int *lun)
> +{
> +    char *name = NULL;
> +    char **name_split = NULL;
> +    int ret = -1;
> +
> +    if (VIR_STRDUP(name, vol->name) < 0)
> +        return -1;
> +    if (!(name_split = virStringSplit(name, ":", 4)))
> +        goto cleanup;
> +    if (!name_split[3])

This is unsafe. What if name is "a:b"? Then there is not going to be name_split[3] and the code crashes.

I think we are just safe with something like this:

diff --git i/src/storage/storage_backend_iscsi_direct.c w/src/storage/storage_backend_iscsi_direct.c
index 094425c101..d473366382 100644
--- i/src/storage/storage_backend_iscsi_direct.c
+++ w/src/storage/storage_backend_iscsi_direct.c
@@ -218,6 +218,9 @@ virISCSIDirectTestUnitReady(struct iscsi_context *iscsi,
     return ret;
 }
 
+
+#define VOL_NAME_PREFIX "unit:0:0:"
+
 static int
 virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
                                   virStorageVolDefPtr vol,
@@ -226,7 +229,7 @@ virISCSIDirectSetVolumeAttributes(virStoragePoolObjPtr pool,
 {
     virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
 
-    if (virAsprintf(&vol->name, "unit:0:0:%u", lun) < 0)
+    if (virAsprintf(&vol->name, VOL_NAME_PREFIX "%u", lun) < 0)
         return -1;
     if (virAsprintf(&vol->key, "ip-%s-iscsi-%s-lun-%u", portal,
                     def->source.devices[0].path, lun) < 0)

And then:

    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;


> +        goto cleanup;
> +    if (virStrToLong_i(name_split[3], NULL, 10, lun) < 0)
> +        goto cleanup;
> +
> +    ret = 0;
> + cleanup:
> +    VIR_FREE(name);
> +    virStringListFree(name_split);
> +    printf("\n");

Oops ;-)

> +    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)
> +{
> +    virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
> +    struct iscsi_context *iscsi = NULL;
> +    char *portal = NULL;
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);
> +


Starting from here ...

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

... to here, the code looks exactly the same as in virStorageBackendISCSIDirectRefreshPool(). I suspect it is going to be copied over to another methods that will be implemented. Therefore I think it should be moved to a separate function.

> +
> +
> +    switch ((virStorageVolWipeAlgorithm) algorithm) {
> +    case VIR_STORAGE_VOL_WIPE_ALG_ZERO:
> +        if (virStorageBackendISCSIDirectVolWipeZero(vol, iscsi) < 0) {
> +            virReportSystemError(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;
> +    }
> +
> +    ret = 0;
> + disconnect:
> +    virISCSIDirectDisconnect(iscsi);
> + cleanup:
> +    VIR_FREE(portal);
> +    iscsi_destroy_context(iscsi);
> +    return ret;
> +}
> +
> +
>  virStorageBackend virStorageBackendISCSIDirect = {
>      .type = VIR_STORAGE_POOL_ISCSI_DIRECT,
>  
>      .checkPool = virStorageBackendISCSIDirectCheckPool,
>      .findPoolSources = virStorageBackendISCSIDirectFindPoolSources,
>      .refreshPool = virStorageBackendISCSIDirectRefreshPool,
> +    .wipeVol = virStorageBackenISCSIDirectWipeVol,
>  };
>  
>  int
> 

Otherwise looking good.

Michal


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