[libvirt] [PATCH 3/3] storage: Clean up volume wiping

[John Ferlan]

On 08/01/2016 09:12 AM, Martin Kletzander wrote:
> Let's cleanly differentiate what wiping a volume does for ploop and
> other volumes so it's more readable what is done for each one instead of
> branching out multiple times in different parts of the same function.
> 
> Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> ---
>  src/storage/storage_backend.c | 78 ++++++++++++++++++++++++-------------------
>  1 file changed, 44 insertions(+), 34 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index b7c7af298f29..27a757edec73 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2401,47 +2401,28 @@ virStorageBackendWipeLocal(const char *path,
>  }
> 
> 
> -/* In here just for a clean patch series, will be removed in future patch */
> -static int virStorageBackendVolWipePloop(virStorageVolDefPtr vol);
> -
> -
> -int
> -virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
> -                              virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> -                              virStorageVolDefPtr vol,
> -                              unsigned int algorithm,
> -                              unsigned int flags)
> +static int
> +virStorageBackendVolWipeLocalFile(const char *path,
> +                                  unsigned int algorithm,
> +                                  unsigned long long allocation)
>  {
>      int ret = -1, fd = -1;
>      const char *alg_char = NULL;
>      struct stat st;
>      virCommandPtr cmd = NULL;
> -    char *path = NULL;
> -    char *target_path = vol->target.path;
> 
> -    virCheckFlags(0, -1);
> -
> -    VIR_DEBUG("Wiping volume with path '%s' and algorithm %u",
> -              vol->target.path, algorithm);
> -
> -    if (vol->target.format == VIR_STORAGE_FILE_PLOOP) {
> -        if (virAsprintf(&path, "%s/root.hds", vol->target.path) < 0)
> -            goto cleanup;
> -        target_path = path;
> -    }
> -
> -    fd = open(target_path, O_RDWR);
> +    fd = open(path, O_RDWR);
>      if (fd == -1) {
>          virReportSystemError(errno,
>                               _("Failed to open storage volume with path '%s'"),
> -                             vol->target.path);
> +                             path);
>          goto cleanup;
>      }
> 
>      if (fstat(fd, &st) == -1) {
>          virReportSystemError(errno,
>                               _("Failed to stat storage volume with path '%s'"),
> -                             vol->target.path);
> +                             path);
>          goto cleanup;
>      }
> 
> @@ -2484,10 +2465,11 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
> 
> +    VIR_DEBUG("Wiping file '%s' with algorithm '%s'", path, alg_char);
> +
>      if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) {
>          cmd = virCommandNew(SCRUB);
> -        virCommandAddArgList(cmd, "-f", "-p", alg_char,
> -                             target_path, NULL);
> +        virCommandAddArgList(cmd, "-f", "-p", alg_char, path, NULL);
> 
>          if (virCommandRun(cmd, NULL) < 0)
>              goto cleanup;
> @@ -2499,26 +2481,23 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
>          } else {
>              ret = virStorageBackendWipeLocal(path,
>                                               fd,
> -                                             vol->target.allocation,
> +                                             allocation,
>                                               st.st_blksize);
>          }
>          if (ret < 0)
>              goto cleanup;
>      }
> 
> -    if (vol->target.format == VIR_STORAGE_FILE_PLOOP)
> -        ret = virStorageBackendVolWipePloop(vol);
> -
>   cleanup:
>      virCommandFree(cmd);
> -    VIR_FREE(path);
>      VIR_FORCE_CLOSE(fd);
>      return ret;
>  }
> 
> 
>  static int
> -virStorageBackendVolWipePloop(virStorageVolDefPtr vol)
> +virStorageBackendVolWipePloop(virStorageVolDefPtr vol,
> +                              unsigned int algorithm)


Ironically... this could take "path", "allocation", and "capacity" as
parameters instead of "vol"....

Your call on adjusting this one as well...  If you do, you should adjust
the odd vertical alignment for the virCommandAddArgFormat for
VIR_DIV_UP(capacity,...)

John
>  {
>      virCommandPtr cmd = NULL;
>      char *target_path = NULL;
> @@ -2540,6 +2519,11 @@ virStorageBackendVolWipePloop(virStorageVolDefPtr vol)
>      if (virAsprintf(&disk_desc, "%s/DiskDescriptor.xml", vol->target.path) < 0)
>          goto cleanup;
> 
> +    if (virStorageBackendVolWipeLocalFile(target_path,
> +                                          algorithm,
> +                                          vol->target.allocation) < 0)
> +        goto cleanup;
> +
>      if (virFileRemove(disk_desc, 0, 0) < 0) {
>          virReportError(errno, _("Failed to delete DiskDescriptor.xml of volume '%s'"),
>                         vol->target.path);
> @@ -2568,6 +2552,32 @@ virStorageBackendVolWipePloop(virStorageVolDefPtr vol)
>  }
> 
> 
> +int
> +virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
> +                              virStoragePoolObjPtr pool ATTRIBUTE_UNUSED,
> +                              virStorageVolDefPtr vol,
> +                              unsigned int algorithm,
> +                              unsigned int flags)
> +{
> +    int ret = -1;
> +
> +    virCheckFlags(0, -1);
> +
> +    VIR_DEBUG("Wiping volume with path '%s' and algorithm %u",
> +              vol->target.path, algorithm);
> +
> +    if (vol->target.format == VIR_STORAGE_FILE_PLOOP) {
> +        ret = virStorageBackendVolWipePloop(vol, algorithm);
> +    } else {
> +        ret = virStorageBackendVolWipeLocalFile(vol->target.path,
> +                                                algorithm,
> +                                                vol->target.allocation);
> +    }
> +
> +    return ret;
> +}
> +
> +
>  #ifdef GLUSTER_CLI
>  int
>  virStorageBackendFindGlusterPoolSources(const char *host,
>