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

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

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.

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.


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


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