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

2012-03-07 Thread Peter Krempa 

On 03/06/2012 01:34 AM, Eric Blake wrote:

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.
(cmdMemtuneGetSize): New helper.
(cmdMemtune): Use it.
* tools/virsh.pod (setmem, setmaxmem, memtune): Document this.
---

  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
+cmdMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value)


As this is a helper function rename it please to vshMemtuneGetSize to 
avoid confusion with command functions.



+{
+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;


ACK with the name change.

Peter

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

2012-03-07 Thread Eric Blake 
On 03/07/2012 06:14 AM, Peter Krempa wrote:
 On 03/06/2012 01:34 AM, Eric Blake wrote:
 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.


 +static int
 +cmdMemtuneGetSize(const vshCmd *cmd, const char *name, long long *value)
 
 As this is a helper function rename it please to vshMemtuneGetSize to
 avoid confusion with command functions.

Sure.

 
 ACK with the name change.

Thanks for the review.  Nits fixed, and series pushed.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCHv2 15/15] virsh: improve memory unit parsing

2012-03-05 Thread Eric Blake 
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.
(cmdMemtuneGetSize): New helper.
(cmdMemtune): Use it.
* tools/virsh.pod (setmem, setmaxmem, memtune): Document this.
---

v2: new

 tools/virsh.c   |  110 +--
 tools/virsh.pod |   48 
 2 files changed, 90 insertions(+), 68 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c
index d5cc46b..b93706f 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -5668,7 +5668,7 @@ cleanup:
 }

 /*
- * setmemory command
+ * setmem command
  */
 static const vshCmdInfo info_setmem[] = {
 {help, N_(change memory allocation)},
@@ -5678,7 +5678,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)},
@@ -5689,8 +5691,9 @@ static bool
 cmdSetmem(vshControl *ctl, const vshCmd *cmd)
 {
 virDomainPtr dom;
-virDomainInfo info;
-unsigned long 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);
@@ -5719,36 +5722,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;
 }
 }
@@ -5768,7 +5760,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)},
@@ -5779,7 +5773,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);
@@ -5807,24 +5803,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));
-