virsh: improve memory unit parsing

The last vestige of the inaccurate 'kilobytes' when we meant 1024 is
now gone.  And virsh is now useful for setting memory in units other
than KiB.

* tools/virsh.c (cmdSetmem, cmdSetmaxmem): Use new helper routine,
allow passing bogus arguments on to hypervisor to test driver
sanity checking, and fix leak on parse error.
(vshMemtuneGetSize): New helper.
(cmdMemtune): Use it.
* tools/virsh.pod (setmem, setmaxmem, memtune): Document this.

tools/virsh.c

@@ -5822,7 +5822,7 @@ cleanup:
 }
 
 /*
- * "setmemory" command
+ * "setmem" command
  */
 static const vshCmdInfo info_setmem[] = {
     {"help", N_("change memory allocation")},
@@ -5832,7 +5832,9 @@ static const vshCmdInfo info_setmem[] = {
 
 static const vshCmdOptDef opts_setmem[] = {
     {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
-    {"kilobytes", VSH_OT_INT, VSH_OFLAG_REQ, N_("number of kilobytes of memory")},
+    {"kilobytes", VSH_OT_ALIAS, 0, "size"},
+    {"size", VSH_OT_INT, VSH_OFLAG_REQ,
+     N_("new memory size, as scaled integer (default KiB)")},
     {"config", VSH_OT_BOOL, 0, N_("affect next boot")},
     {"live", VSH_OT_BOOL, 0, N_("affect running domain")},
     {"current", VSH_OT_BOOL, 0, N_("affect current domain")},
@@ -5843,8 +5845,9 @@ static bool
 cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 {
     virDomainPtr dom;
-    virDomainInfo info;
+    unsigned long long bytes = 0;
-    unsigned long kilobytes = 0;
+    unsigned long long max;
+    unsigned long kibibytes = 0;
     bool ret = true;
     int config = vshCommandOptBool(cmd, "config");
     int live = vshCommandOptBool(cmd, "live");
@@ -5873,36 +5876,25 @@ cmdSetmem(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return false;
 
-    if (vshCommandOptUL(cmd, "kilobytes", &kilobytes) < 0) {
+    /* The API expects 'unsigned long' KiB, so depending on whether we
+     * are 32-bit or 64-bit determines the maximum we can use.  */
+    if (sizeof(kibibytes) < sizeof(max))
+        max = 1024ull * ULONG_MAX;
+    else
+        max = ULONG_MAX;
+    if (vshCommandOptScaledInt(cmd, "size", &bytes, 1024, max) < 0) {
         vshError(ctl, "%s", _("memory size has to be a number"));
-        return false;
-    }
-
-    if (kilobytes <= 0) {
         virDomainFree(dom);
-        vshError(ctl, _("Invalid value of %lu for memory size"), kilobytes);
-        return false;
-    }
-
-    if (virDomainGetInfo(dom, &info) != 0) {
-        virDomainFree(dom);
-        vshError(ctl, "%s", _("Unable to verify MaxMemorySize"));
-        return false;
-    }
-
-    if (kilobytes > info.maxMem) {
-        virDomainFree(dom);
-        vshError(ctl, _("Requested memory size %lu kb is larger than maximum of %lu kb"),
-                 kilobytes, info.maxMem);
         return false;
     }
+    kibibytes = VIR_DIV_UP(bytes, 1024);
 
     if (flags == -1) {
-        if (virDomainSetMemory(dom, kilobytes) != 0) {
+        if (virDomainSetMemory(dom, kibibytes) != 0) {
             ret = false;
         }
     } else {
-        if (virDomainSetMemoryFlags(dom, kilobytes, flags) < 0) {
+        if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) {
             ret = false;
         }
     }
@@ -5922,7 +5914,9 @@ static const vshCmdInfo info_setmaxmem[] = {
 
 static const vshCmdOptDef opts_setmaxmem[] = {
     {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
-    {"kilobytes", VSH_OT_INT, VSH_OFLAG_REQ, N_("maximum memory limit in kilobytes")},
+    {"kilobytes", VSH_OT_ALIAS, 0, "size"},
+    {"size", VSH_OT_INT, VSH_OFLAG_REQ,
+     N_("new maximum memory size, as scaled integer (default KiB)")},
     {"config", VSH_OT_BOOL, 0, N_("affect next boot")},
     {"live", VSH_OT_BOOL, 0, N_("affect running domain")},
     {"current", VSH_OT_BOOL, 0, N_("affect current domain")},
@@ -5933,7 +5927,9 @@ static bool
 cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
 {
     virDomainPtr dom;
-    int kilobytes = 0;
+    unsigned long long bytes = 0;
+    unsigned long long max;
+    unsigned long kibibytes = 0;
     bool ret = true;
     int config = vshCommandOptBool(cmd, "config");
     int live = vshCommandOptBool(cmd, "live");
@@ -5961,24 +5957,26 @@ cmdSetmaxmem(vshControl *ctl, const vshCmd *cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return false;
 
-    if (vshCommandOptInt(cmd, "kilobytes", &kilobytes) < 0) {
+    /* The API expects 'unsigned long' KiB, so depending on whether we
+     * are 32-bit or 64-bit determines the maximum we can use.  */
+    if (sizeof(kibibytes) < sizeof(max))
+        max = 1024ull * ULONG_MAX;
+    else
+        max = ULONG_MAX;
+    if (vshCommandOptScaledInt(cmd, "size", &bytes, 1024, max) < 0) {
         vshError(ctl, "%s", _("memory size has to be a number"));
-        return false;
-    }
-
-    if (kilobytes <= 0) {
         virDomainFree(dom);
-        vshError(ctl, _("Invalid value of %d for memory size"), kilobytes);
         return false;
     }
+    kibibytes = VIR_DIV_UP(bytes, 1024);
 
     if (flags == -1) {
-        if (virDomainSetMaxMemory(dom, kilobytes) != 0) {
+        if (virDomainSetMaxMemory(dom, kibibytes) != 0) {
             vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
             ret = false;
         }
     } else {
-        if (virDomainSetMemoryFlags(dom, kilobytes, flags) < 0) {
+        if (virDomainSetMemoryFlags(dom, kibibytes, flags) < 0) {
             vshError(ctl, "%s", _("Unable to change MaxMemorySize"));
             ret = false;
         }
@@ -6151,21 +6149,45 @@ static const vshCmdInfo info_memtune[] = {
 static const vshCmdOptDef opts_memtune[] = {
     {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")},
     {"hard-limit", VSH_OT_INT, VSH_OFLAG_NONE,
-     N_("Max memory in kilobytes")},
+     N_("Max memory, as scaled integer (default KiB)")},
     {"soft-limit", VSH_OT_INT, VSH_OFLAG_NONE,
-     N_("Memory during contention in kilobytes")},
+     N_("Memory during contention, as scaled integer (default KiB)")},
     {"swap-hard-limit", VSH_OT_INT, VSH_OFLAG_NONE,
-     N_("Max memory plus swap in kilobytes")},
+     N_("Max memory plus swap, as scaled integer (default KiB)")},
     {"min-guarantee", VSH_OT_INT, VSH_OFLAG_NONE,
-     N_("Min guaranteed memory in kilobytes")},
+     N_("Min guaranteed memory, as scaled integer (default KiB)")},
     {"config", VSH_OT_BOOL, 0, N_("affect next boot")},
     {"live", VSH_OT_BOOL, 0, N_("affect running domain")},
     {"current", VSH_OT_BOOL, 0, N_("affect current domain")},
     {NULL, 0, 0, NULL}
 };
 
+static int
+vshMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value)
+{
+    int ret;
+    unsigned long long tmp;
+    const char *str;
+    char *end;
+
+    ret = vshCommandOptString(cmd, name, &str);
+    if (ret <= 0)
+        return ret;
+    if (virStrToLong_ll(str, &end, 10, value) < 0)
+        return -1;
+    if (*value < 0) {
+        *value = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
+        return 1;
+    }
+    tmp = *value;
+    if (virScaleInteger(&tmp, end, 1024, LLONG_MAX) < 0)
+        return -1;
+    *value = VIR_DIV_UP(tmp, 1024);
+    return 0;
+}
+
 static bool
-cmdMemtune(vshControl * ctl, const vshCmd * cmd)
+cmdMemtune(vshControl *ctl, const vshCmd *cmd)
 {
     virDomainPtr dom;
     long long hard_limit = 0, soft_limit = 0, swap_hard_limit = 0;
@@ -6198,10 +6220,10 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd)
     if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
         return false;
 
-    if (vshCommandOptLongLong(cmd, "hard-limit", &hard_limit) < 0 ||
+    if (vshMemtuneGetSize(cmd, "hard-limit", &hard_limit) < 0 ||
-        vshCommandOptLongLong(cmd, "soft-limit", &soft_limit) < 0 ||
+        vshMemtuneGetSize(cmd, "soft-limit", &soft_limit) < 0 ||
-        vshCommandOptLongLong(cmd, "swap-hard-limit", &swap_hard_limit) < 0 ||
+        vshMemtuneGetSize(cmd, "swap-hard-limit", &swap_hard_limit) < 0 ||
-        vshCommandOptLongLong(cmd, "min-guarantee", &min_guarantee) < 0) {
+        vshMemtuneGetSize(cmd, "min-guarantee", &min_guarantee) < 0) {
         vshError(ctl, "%s",
                  _("Unable to parse integer parameter"));
         goto cleanup;

tools/virsh.pod

@@ -1170,7 +1170,7 @@ B<Examples>
   # send a tab, held for 1 second
   virsh send-key --holdtime 1000 0xf
 
-=item B<setmem> I<domain-id> B<kilobytes> [[I<--config>] [I<--live>] |
+=item B<setmem> I<domain-id> B<size> [[I<--config>] [I<--live>] |
 [I<--current>]]
 
 Change the memory allocation for a guest domain.
@@ -1181,15 +1181,17 @@ Both I<--live> and I<--config> flags may be given, but I<--current> is
 exclusive. If no flag is specified, behavior is different depending
 on hypervisor.
 
-Some hypervisors require a larger granularity than kilobytes, and requests
+I<size> is a scaled integer (see B<NOTES> above); it defaults to kibibytes
-that are not an even multiple will be rounded up.  For example, vSphere/ESX
+(blocks of 1024 bytes) unless you provide a suffix (and the older option
-rounds the parameter up unless the kB argument is evenly divisible by 1024
+name I<--kilobytes> is available as a deprecated synonym) .  Libvirt rounds
-(that is, the kB argument happens to represent megabytes).
+up to the nearest kibibyte.  Some hypervisors require a larger granularity
+than KiB, and requests that are not an even multiple will be rounded up.
+For example, vSphere/ESX rounds the parameter up to mebibytes (1024 kibibytes).
 
 For Xen, you can only adjust the memory of a running domain if the domain is
 paravirtualized or running the PV balloon driver.
 
-=item B<setmaxmem> I<domain-id> B<kilobytes> [[I<--config>] [I<--live>] |
+=item B<setmaxmem> I<domain-id> B<size> [[I<--config>] [I<--live>] |
 [I<--current>]]
 
 Change the maximum memory allocation limit for a guest domain.
@@ -1202,22 +1204,24 @@ on hypervisor.
 
 This command works for at least the Xen, QEMU/KVM and vSphere/ESX hypervisors.
 
-Some hypervisors require a larger granularity than kilobytes, rounding up
+I<size> is a scaled integer (see B<NOTES> above); it defaults to kibibytes
-requests that are not an even multiple of the desired amount.  vSphere/ESX
+(blocks of 1024 bytes) unless you provide a suffix (and the older option
-is one of these, requiring the parameter to be evenly divisible by 4MB.  For
+name I<--kilobytes> is available as a deprecated synonym) .  Libvirt rounds
-vSphere/ESX, 263168 (257MB) would be rounded up because it's not a multiple
+up to the nearest kibibyte.  Some hypervisors require a larger granularity
-of 4MB, while 266240 (260MB) is valid without rounding.
+than KiB, and requests that are not an even multiple will be rounded up.
+For example, vSphere/ESX rounds the parameter up to mebibytes (1024 kibibytes).
 
-
+=item B<memtune> I<domain-id> [I<--hard-limit> B<size>]
-=item B<memtune> I<domain-id> [I<--hard-limit> B<kilobytes>]
+[I<--soft-limit> B<size>] [I<--swap-hard-limit> B<size>]
-[I<--soft-limit> B<kilobytes>] [I<--swap-hard-limit> B<kilobytes>]
+[I<--min-guarantee> B<size>] [[I<--config>] [I<--live>] | [I<--current>]]
-[I<--min-guarantee> B<kilobytes>] [[I<--config>] [I<--live>] | [I<--current>]]
 
 Allows you to display or set the domain memory parameters. Without
 flags, the current settings are displayed; with a flag, the
 appropriate limit is adjusted if supported by the hypervisor.  LXC and
 QEMU/KVM support I<--hard-limit>, I<--soft-limit>, and I<--swap-hard-limit>.
-I<--min-guarantee> is supported only by ESX hypervisor.
+I<--min-guarantee> is supported only by ESX hypervisor.  Each of these
+limits are scaled integers (see B<NOTES> above), with a default of
+kibibytes (blocks of 1024 bytes) if no suffix is present.
 
 If I<--live> is specified, affect a running guest.
 If I<--config> is specified, affect the next boot of a persistent guest.
@@ -1235,24 +1239,20 @@ one needs guess and try.
 
 =item I<--hard-limit>
 
-The maximum memory the guest can use.  The units for this value are kilobytes
+The maximum memory the guest can use.
-(i.e. blocks of 1024 bytes).
 
 =item I<--soft-limit>
 
-The memory limit to enforce during memory contention.  The units for this
+The memory limit to enforce during memory contention.
-value are kilobytes (i.e. blocks of 1024 bytes).
 
 =item I<--swap-hard-limit>
 
-The maximum memory plus swap the guest can use.  The units for this value are
+The maximum memory plus swap the guest can use.  This has to be more
-kilobytes (i.e. blocks of 1024 bytes).  This has to be more than hard-limit
+than hard-limit value provided.
-value provided.
 
 =item I<--min-guarantee>
 
-The guaranteed minimum memory allocation for the guest.  The units for this
+The guaranteed minimum memory allocation for the guest.
-value are kilobytes (i.e. blocks of 1024 bytes).
 
 =back