[libvirt] [PATCH 1/5] storage: Properly check flags when wiping volumes

[John Ferlan]

On 01/27/2016 05:20 AM, Wido den Hollander wrote:
> Using virCheckFlags() we validate if the provided flags
> are support flags by this function.
> 
> The old code would also still run the command since it didn't
> go to cleanup when a invalid flag was supplied.
> 
> We now go to cleanup and exit if a invalid flag would be provided.
> 
> Signed-off-by: Wido den Hollander <wido at widodh.nl>
> ---
>  src/storage/storage_backend.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 03bc18c..15e9109 100644
> --- a/src/storage/storage_backend.c
> +++ b/src/storage/storage_backend.c
> @@ -2056,7 +2056,16 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
>      struct stat st;
>      virCommandPtr cmd = NULL;
>  
> -    virCheckFlags(0, -1);
> +    virCheckFlags(VIR_STORAGE_VOL_WIPE_ALG_ZERO |
> +                  VIR_STORAGE_VOL_WIPE_ALG_NNSA |
> +                  VIR_STORAGE_VOL_WIPE_ALG_DOD |
> +                  VIR_STORAGE_VOL_WIPE_ALG_BSI |
> +                  VIR_STORAGE_VOL_WIPE_ALG_GUTMANN |
> +                  VIR_STORAGE_VOL_WIPE_ALG_SCHNEIER |
> +                  VIR_STORAGE_VOL_WIPE_ALG_PFITZNER7 |
> +                  VIR_STORAGE_VOL_WIPE_ALG_PFITZNER33 |
> +                  VIR_STORAGE_VOL_WIPE_ALG_RANDOM |
> +                  VIR_STORAGE_VOL_WIPE_ALG_LAST, -1);

Looks like my brain had a misfire - these are bits for 'algorithm' and
not 'flags' <sigh>.   I'll clean up, no need to resend.

>  
>      VIR_DEBUG("Wiping volume with path '%s' and algorithm %u",
>                vol->target.path, algorithm);
> @@ -2078,7 +2087,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>      if (algorithm != VIR_STORAGE_VOL_WIPE_ALG_ZERO) {
>          const char *alg_char ATTRIBUTE_UNUSED = NULL;
> -        switch (algorithm) {
> +        switch ((virStorageVolWipeAlgorithm) algorithm) {

FYI: When you do this...  Then the "default:" changes to
VIR_STORAGE_VOL_WIPE_ALG_LAST and of course you'd need a
VIR_STORAGE_VOL_WIPE_ALG_ZERO case.

Because of that if construct I moved the switch outside the if, in order
to add case ZERO (otherwise it's a dead code path from coverity)

I cleaned it up already and pushed.

Tks -

John
>          case VIR_STORAGE_VOL_WIPE_ALG_NNSA:
>              alg_char = "nnsa";
>              break;
> @@ -2107,6 +2116,7 @@ virStorageBackendVolWipeLocal(virConnectPtr conn ATTRIBUTE_UNUSED,
>              virReportError(VIR_ERR_INVALID_ARG,
>                             _("unsupported algorithm %d"),
>                             algorithm);
> +            goto cleanup;
>          }
>          cmd = virCommandNew(SCRUB);
>          virCommandAddArgList(cmd, "-f", "-p", alg_char,
>