[libvirt] [PATCHv2 14/15] virsh: improve storage unit parsing

[Peter Krempa]

On 03/06/2012 01:34 AM, Eric Blake wrote:
> Now can now do:
>
> virsh vol-resize $vol 10M
> virsh blockresize $dom $vol 10M
>
> to get both interfaces to resize to 10MiB.  The remaining wart
> is that vol-resize defaults to bytes, but blockresize defaults
> to KiB, but we can't break existing scripts; oh well, it's no
> worse than the same wart of the underlying virDomainBlockResize.
>
> * tools/virsh.c (vshCommandOptScaledInt): New function.
> (cmdVolResize): Don't pass negative size.
> (cmdVolSize): Use new helper routine.
> (cmdBlockResize): Likewise; also support bytes.
> * tools/virsh.pod (NOTES): Document suffixes.
> (blockresize, vol-create-as, vol-resize): Point to notes.
> ---

>   static bool
> @@ -11754,7 +11743,7 @@ static const vshCmdInfo info_vol_resize[] = {
>   static const vshCmdOptDef opts_vol_resize[] = {
>       {"vol", VSH_OT_DATA, VSH_OFLAG_REQ, N_("vol name, key or path")},
>       {"capacity", VSH_OT_DATA, VSH_OFLAG_REQ,
> -     N_("new capacity for the vol with optional k,M,G,T suffix")},
> +     N_("new capacity for the vol, as scaled integer (default bytes)")},
>       {"pool", VSH_OT_STRING, 0, N_("pool name or uuid")},
>       {"allocate", VSH_OT_BOOL, 0,
>        N_("allocate the new capacity, rather than leaving it sparse")},
> @@ -11792,16 +11781,12 @@ cmdVolResize(vshControl *ctl, const vshCmd *cmd)
>       if (vshCommandOptString(cmd, "capacity",&capacityStr)<= 0)
>           goto cleanup;
>       if (delta&&  *capacityStr == '-') {
> -        if (cmdVolSize(capacityStr + 1,&capacity)<  0) {
> -            vshError(ctl, _("Malformed size %s"), capacityStr);
> -            goto cleanup;
> -        }
> -        capacity = -capacity;
> -    } else {
> -        if (cmdVolSize(capacityStr,&capacity)<  0) {
> -            vshError(ctl, _("Malformed size %s"), capacityStr);
> -            goto cleanup;
> -        }
> +        capacityStr++;
This shift in the string discards the minus sign in error/success 
messages, but their meaning remains correct.
> +        flags |= VIR_STORAGE_VOL_RESIZE_SHRINK;

This change in code ...

> +    }
> +    if (cmdVolSize(capacityStr,&capacity)<  0) {
> +        vshError(ctl, _("Malformed size %s"), capacityStr);
> +        goto cleanup;
>       }
>
>       if (virStorageVolResize(vol, capacity, flags) == 0) {
> @@ -2146,7 +2168,10 @@ is in. I<vol-name-or-key-or-path>  is the name or key or path of the volume
>   to resize.  The new capacity might be sparse unless I<--allocate>  is
>   specified.  Normally, I<capacity>  is the new size, but if I<--delta>
>   is present, then it is added to the existing size.  Attempts to shrink
> -the volume will fail unless I<--shrink>  is present.
 > +the volume will fail unless I<--shrink>  is present.<capacity>  is

... contradicts with this statment in the virsh man page. With your 
change, if the delta is negative the shrink flag for the api call is set 
automaticaly.

This was probably added as a safety measure and I'd prefer if we would 
require the --shrink flag to mark that the user is sure of what he's 
doing, although it should be obvious enough from the minus sign. (I 
honestly would prefer a docs change but I'm afraid someone could get mad 
at us if he "accidentaly" corrupts his images.)

> +scaled integer (see B<NOTES>  above), which defaults to bytes if there
> +is no suffix.  This command is only safe for storage volumes not in
> +use by an active guest; see also B<blockresize>  for live resizing.
>
>   =back
>

ACK, if you add the check for --shrink.

Peter