Re: [libvirt] [PATCH 1/3] virsh: fix memtune to also accept 0 as valid value

2015-03-05 Thread Peter Krempa 
On Wed, Mar 04, 2015 at 17:17:05 +0100, Pavel Hrdina wrote:

Commit message is too sparse.

 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539

Also I doubt that this on itself resolves this bug.

 
 Signed-off-by: Pavel Hrdina phrd...@redhat.com
 ---
  tools/virsh-domain.c | 62 
 +---
  1 file changed, 20 insertions(+), 42 deletions(-)
 
 diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
 index 55c269c..773f9f1 100644
 --- a/tools/virsh-domain.c
 +++ b/tools/virsh-domain.c
 @@ -8276,7 +8276,7 @@ vshMemtuneGetSize(const vshCmd *cmd, const char *name, 
 long long *value)
  if (virScaleInteger(tmp, end, 1024, LLONG_MAX)  0)
  return -1;
  *value = VIR_DIV_UP(tmp, 1024);
 -return 0;
 +return 1;

Now that the return values from this function actually make sense it
would be worth adding a comment what they mean.

  }
  
  static bool
 @@ -8294,6 +8294,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
  bool current = vshCommandOptBool(cmd, current);
  bool config = vshCommandOptBool(cmd, config);
  bool live = vshCommandOptBool(cmd, live);
 +int rc = 0;

rc doesn't need to be initialized as the macro below sets and tests it.

  
  VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
  VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
 @@ -8306,50 +8307,26 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
  if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
  return false;
  
 -if (vshMemtuneGetSize(cmd, hard-limit, hard_limit)  0 ||
 -vshMemtuneGetSize(cmd, soft-limit, soft_limit)  0 ||
 -vshMemtuneGetSize(cmd, swap-hard-limit, swap_hard_limit)  0 ||
 -vshMemtuneGetSize(cmd, min-guarantee, min_guarantee)  0) {
 -vshError(ctl, %s,
 - _(Unable to parse integer parameter));
 -goto cleanup;
 -}
 +#define PARSE_MEMTUNE_PARAM(NAME, VALUE, FIELD) \
 +if ((rc = vshMemtuneGetSize(cmd, NAME, VALUE))  0) {  \
 +vshError(ctl, %s, _(Unable to parse integer parameter 'NAME')); \
 +goto cleanup;   \
 +}   \
 +if (rc == 1) {  \
 +if (virTypedParamsAddULLong(params, nparams, maxparams,  \
 +FIELD, VALUE)  0)  \
 +goto save_error;\
 +}   \
  
 -if (hard_limit) {
 -if (hard_limit == -1)
 -hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
 -if (virTypedParamsAddULLong(params, nparams, maxparams,
 -VIR_DOMAIN_MEMORY_HARD_LIMIT,
 -hard_limit)  0)
 -goto save_error;
 -}
  
 -if (soft_limit) {
 -if (soft_limit == -1)
 -soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
 -if (virTypedParamsAddULLong(params, nparams, maxparams,
 -VIR_DOMAIN_MEMORY_SOFT_LIMIT,
 -soft_limit)  0)
 -goto save_error;
 -}
 -
 -if (swap_hard_limit) {
 -if (swap_hard_limit == -1)
 -swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
 -if (virTypedParamsAddULLong(params, nparams, maxparams,
 -VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT,
 -swap_hard_limit)  0)
 -goto save_error;
 -}
 +PARSE_MEMTUNE_PARAM(hard-limit, hard_limit, 
 VIR_DOMAIN_MEMORY_HARD_LIMIT);
 +PARSE_MEMTUNE_PARAM(soft-limit, soft_limit, 
 VIR_DOMAIN_MEMORY_SOFT_LIMIT);
 +PARSE_MEMTUNE_PARAM(swap-hard-limit, swap_hard_limit,
 +VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT);
 +PARSE_MEMTUNE_PARAM(min-guarantee, min_guarantee,
 +VIR_DOMAIN_MEMORY_MIN_GUARANTEE);
  
 -if (min_guarantee) {
 -if (min_guarantee == -1)
 -min_guarantee = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
 -if (virTypedParamsAddULLong(params, nparams, maxparams,
 -VIR_DOMAIN_MEMORY_MIN_GUARANTEE,
 -min_guarantee)  0)
 -goto save_error;
 -}
 +#undef PARSE_MEMTUNE_PARAM

Um ...

  
  if (nparams == 0) {
  /* get the number of memory parameters */
 @@ -8390,6 +8367,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
  ret = true;
  
   cleanup:
 +#undef PARSE_MEMTUNE_PARAM

... why are you undefing it twice?

  virTypedParamsFree(params, nparams);
  virDomainFree(dom);
  return ret;

Also the virsh man page would benefit from adding a statement that -1
should be used as unlimited.

Peter

Re: [libvirt] [PATCH 1/3] virsh: fix memtune to also accept 0 as valid value

2015-03-05 Thread Pavel Hrdina 
On Thu, Mar 05, 2015 at 10:08:45AM +0100, Peter Krempa wrote:
 On Wed, Mar 04, 2015 at 17:17:05 +0100, Pavel Hrdina wrote:
 
 Commit message is too sparse.
 
  Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1146539
 
 Also I doubt that this on itself resolves this bug.

Well, not directly so I'll remove it from commit message.

 
  
  Signed-off-by: Pavel Hrdina phrd...@redhat.com
  ---
   tools/virsh-domain.c | 62 
  +---
   1 file changed, 20 insertions(+), 42 deletions(-)
  
  diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
  index 55c269c..773f9f1 100644
  --- a/tools/virsh-domain.c
  +++ b/tools/virsh-domain.c
  @@ -8276,7 +8276,7 @@ vshMemtuneGetSize(const vshCmd *cmd, const char 
  *name, long long *value)
   if (virScaleInteger(tmp, end, 1024, LLONG_MAX)  0)
   return -1;
   *value = VIR_DIV_UP(tmp, 1024);
  -return 0;
  +return 1;
 
 Now that the return values from this function actually make sense it
 would be worth adding a comment what they mean.

Good point, I'll add a comment to this function.

 
   }
   
   static bool
  @@ -8294,6 +8294,7 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
   bool current = vshCommandOptBool(cmd, current);
   bool config = vshCommandOptBool(cmd, config);
   bool live = vshCommandOptBool(cmd, live);
  +int rc = 0;
 
 rc doesn't need to be initialized as the macro below sets and tests it.
 
   
   VSH_EXCLUSIVE_OPTIONS_VAR(current, live);
   VSH_EXCLUSIVE_OPTIONS_VAR(current, config);
  @@ -8306,50 +8307,26 @@ cmdMemtune(vshControl *ctl, const vshCmd *cmd)
   if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
   return false;
   
  -if (vshMemtuneGetSize(cmd, hard-limit, hard_limit)  0 ||
  -vshMemtuneGetSize(cmd, soft-limit, soft_limit)  0 ||
  -vshMemtuneGetSize(cmd, swap-hard-limit, swap_hard_limit)  0 ||
  -vshMemtuneGetSize(cmd, min-guarantee, min_guarantee)  0) {
  -vshError(ctl, %s,
  - _(Unable to parse integer parameter));
  -goto cleanup;
  -}
  +#define PARSE_MEMTUNE_PARAM(NAME, VALUE, FIELD)
   \
  +if ((rc = vshMemtuneGetSize(cmd, NAME, VALUE))  0) { 
   \
  +vshError(ctl, %s, _(Unable to parse integer parameter 
  'NAME')); \
  +goto cleanup;  
   \
  +}  
   \
  +if (rc == 1) { 
   \
  +if (virTypedParamsAddULLong(params, nparams, maxparams, 
   \
  +FIELD, VALUE)  0) 
   \
  +goto save_error;   
   \
  +}  
   \
   
  -if (hard_limit) {
  -if (hard_limit == -1)
  -hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
  -if (virTypedParamsAddULLong(params, nparams, maxparams,
  -VIR_DOMAIN_MEMORY_HARD_LIMIT,
  -hard_limit)  0)
  -goto save_error;
  -}
   
  -if (soft_limit) {
  -if (soft_limit == -1)
  -soft_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
  -if (virTypedParamsAddULLong(params, nparams, maxparams,
  -VIR_DOMAIN_MEMORY_SOFT_LIMIT,
  -soft_limit)  0)
  -goto save_error;
  -}
  -
  -if (swap_hard_limit) {
  -if (swap_hard_limit == -1)
  -swap_hard_limit = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
  -if (virTypedParamsAddULLong(params, nparams, maxparams,
  -VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT,
  -swap_hard_limit)  0)
  -goto save_error;
  -}
  +PARSE_MEMTUNE_PARAM(hard-limit, hard_limit, 
  VIR_DOMAIN_MEMORY_HARD_LIMIT);
  +PARSE_MEMTUNE_PARAM(soft-limit, soft_limit, 
  VIR_DOMAIN_MEMORY_SOFT_LIMIT);
  +PARSE_MEMTUNE_PARAM(swap-hard-limit, swap_hard_limit,
  +VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT);
  +PARSE_MEMTUNE_PARAM(min-guarantee, min_guarantee,
  +VIR_DOMAIN_MEMORY_MIN_GUARANTEE);
   
  -if (min_guarantee) {
  -if (min_guarantee == -1)
  -min_guarantee = VIR_DOMAIN_MEMORY_PARAM_UNLIMITED;
  -if (virTypedParamsAddULLong(params, nparams, maxparams,
  -VIR_DOMAIN_MEMORY_MIN_GUARANTEE,
  -min_guarantee)  0)
  -goto save_error;
  -}
  +#undef PARSE_MEMTUNE_PARAM
 
 Um ...
 
   
   if (nparams == 0) {
   /* get the number of memory parameters */
  @@ -8390,6 +8367,7 @@