Re: [libvirt] [PATCH v2 3/3] virsh: Move error messages inside vshCommandOpt*() functions.

2015-05-28 Thread Andrea Bolognani 
On Wed, 2015-05-27 at 16:47 +0200, Michal Privoznik wrote:

  diff --git a/tools/virsh.c b/tools/virsh.c
  index 9f44793..db9354c 100644
  --- a/tools/virsh.c
  +++ b/tools/virsh.c
  @@ -1517,23 +1517,27 @@ vshCommandOpt(const vshCmd *cmd,
* 0 in all other cases (@value untouched)
*/
 
 Does it makes sense to note in comments that this function (and others that 
 you're fixing) is reporting an error?

Done. I've only updated the comments for vshCommandOptInt() because all
other functions explicitly refer to that one in their comments.

 While reworking this, you've missed vshCommandOptTimeoutToMs() which in case 
 of something like following '--timeout blah' will report error twice: from 
 both OptInt() and OptTimeoutToMs():
 
 error: Numeric value 'blah' for timeout option is malformed or out of range
 error: invalid timeout
 
 I think this should be squashed in:
 
 @@ -1906,11 +1907,13 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const 
 vshCmd *cmd,
  {
  int rv = vshCommandOptInt(ctl, cmd, timeout, timeout);
  
 -if (rv  0 || (rv  0  *timeout  1)) {
 -vshError(ctl, %s, _(invalid timeout));
 +if (rv  0)
  return -1;
 -}
  if (rv  0) {
 +if (*timeout  1) {
 +vshError(ctl, %s, _(invalid timeout));
 +return -1;
 +}
  /* Ensure that we can multiply by 1000 without overflowing. */
  if (*timeout  INT_MAX / 1000) {
  vshError(ctl, %s, _(timeout is too big));

Agreed, two error messages for a single failure is not something the
user should run into.

I've gone for a slightly different implementation here, which adds two
small commits at the beginning of the series. Please check it out.

 You've missed one ocurrance of vshCommandOptULongLong in cmdAllocpages().

I'd actually missed it the first time around, when I unified all the error
messages. Great catch!

 Interestingly vshCommandOptString() is missing. So far, the only way for the 
 function to fail is if one uses --option  and VSH_OFLAG_EMPTY_OK is not 
 specified. So if we come up with good error message for that case, we are 
 okay to drop all the other error messages.

I will look into it next, see what I can do :)

Cheers.

-- 
Andrea Bolognani
Software Engineer - Virtualization Team
$ python -c print('a'.join(['', 'bologn', '@redh', 't.com']))

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

Re: [libvirt] [PATCH v2 3/3] virsh: Move error messages inside vshCommandOpt*() functions.

2015-05-27 Thread Michal Privoznik 
On 22.05.2015 10:59, Andrea Bolognani wrote:
 ---
  tests/vcpupin|   4 +-
  tools/virsh-domain-monitor.c |   9 +--
  tools/virsh-domain.c | 134 
 +++
  tools/virsh-host.c   |  57 +++---
  tools/virsh-interface.c  |   6 +-
  tools/virsh-network.c|   6 +-
  tools/virsh-volume.c |  24 ++--
  tools/virsh.c| 111 +--
  8 files changed, 104 insertions(+), 247 deletions(-)

Nice cleanup.

 diff --git a/tools/virsh.c b/tools/virsh.c
 index 9f44793..db9354c 100644
 --- a/tools/virsh.c
 +++ b/tools/virsh.c
 @@ -1517,23 +1517,27 @@ vshCommandOpt(const vshCmd *cmd,
   * 0 in all other cases (@value untouched)
   */

Does it makes sense to note in comments that this function (and others that 
you're fixing) is reporting an error?

  int
 -vshCommandOptInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
 +vshCommandOptInt(vshControl *ctl, const vshCmd *cmd,
   const char *name, int *value)
  {
  vshCmdOpt *arg;
  int ret;
  
 -ret = vshCommandOpt(cmd, name, arg, true);
 -if (ret = 0)
 +if ((ret = vshCommandOpt(cmd, name, arg, true)) = 0)
  return ret;
  
 -if (virStrToLong_i(arg-data, NULL, 10, value)  0)
 -return -1;
 -return 1;
 +if ((ret = virStrToLong_i(arg-data, NULL, 10, value))  0)
 +vshError(ctl,
 + _(Numeric value '%s' for %s option is malformed or out 
 of range),
 + arg-data, name);
 +else
 +ret = 1;
 +
 +return ret;
  }
  

While reworking this, you've missed vshCommandOptTimeoutToMs() which in case of 
something like following '--timeout blah' will report error twice: from both 
OptInt() and OptTimeoutToMs():

error: Numeric value 'blah' for timeout option is malformed or out of range
error: invalid timeout

I think this should be squashed in:

@@ -1906,11 +1907,13 @@ vshCommandOptTimeoutToMs(vshControl *ctl, const vshCmd 
*cmd,
 {
 int rv = vshCommandOptInt(ctl, cmd, timeout, timeout);
 
-if (rv  0 || (rv  0  *timeout  1)) {
-vshError(ctl, %s, _(invalid timeout));
+if (rv  0)
 return -1;
-}
 if (rv  0) {
+if (*timeout  1) {
+vshError(ctl, %s, _(invalid timeout));
+return -1;
+}
 /* Ensure that we can multiply by 1000 without overflowing. */
 if (*timeout  INT_MAX / 1000) {
 vshError(ctl, %s, _(timeout is too big));

  static int
 -vshCommandOptULongLongInternal(vshControl *ctl ATTRIBUTE_UNUSED, const 
 vshCmd *cmd,
 +vshCommandOptULongLongInternal(vshControl *ctl, const vshCmd *cmd,
 const char *name, unsigned long long *value,
 bool wrap)
  {
 @@ -1754,15 +1767,18 @@ vshCommandOptULongLongInternal(vshControl *ctl 
 ATTRIBUTE_UNUSED, const vshCmd *c
  if ((ret = vshCommandOpt(cmd, name, arg, true)) = 0)
  return ret;
  
 -if (wrap) {
 -if (virStrToLong_ull(arg-data, NULL, 10, value)  0)
 -return -1;
 -} else {
 -if (virStrToLong_ullp(arg-data, NULL, 10, value)  0)
 -return -1;
 -}
 +if (wrap)
 +ret = virStrToLong_ull(arg-data, NULL, 10, value);
 +else
 +ret = virStrToLong_ullp(arg-data, NULL, 10, value);
 +if (ret  0)
 +vshError(ctl,
 + _(Numeric value '%s' for %s option is malformed or out 
 of range),
 + arg-data, name);
 +else
 +ret = 1;
  
 -return 1;
 +return ret;
  }

You've missed one ocurrance of vshCommandOptULongLong in cmdAllocpages().

  
  /**
 @@ -1812,7 +1828,7 @@ vshCommandOptULongLongWrap(vshControl *ctl, const 
 vshCmd *cmd,
   * See vshCommandOptInt()
   */
  int
 -vshCommandOptScaledInt(vshControl *ctl ATTRIBUTE_UNUSED, const vshCmd *cmd,
 +vshCommandOptScaledInt(vshControl *ctl, const vshCmd *cmd,
 const char *name, unsigned long long *value,
 int scale, unsigned long long max)
  {
 @@ -1825,9 +1841,16 @@ vshCommandOptScaledInt(vshControl *ctl 
 ATTRIBUTE_UNUSED, const vshCmd *cmd,
  
  if (virStrToLong_ull(arg-data, end, 10, value)  0 ||
  virScaleInteger(value, end, scale, max)  0)
 -return -1;
 +{
 +vshError(ctl,
 + _(Numeric value '%s' for %s option is malformed or out 
 of range),
 + arg-data, name);
 +ret = -1;
 +} else {
 +ret = 1;
 +}
  
 -return 1;
 +return ret;
  }
  
  
 

Interestingly vshCommandOptString() is missing. So far, the only way for the 
function to fail is if one uses --option  and VSH_OFLAG_EMPTY_OK is not 
specified. So if we come up with good error message for that case, we are okay 
to drop all the other error messages.

Otherwise looking good.

Michal

--
libvir-list mailing list
libvir-list@redhat.com

[libvirt] [PATCH v2 3/3] virsh: Move error messages inside vshCommandOpt*() functions.

2015-05-22 Thread Andrea Bolognani 
---
 tests/vcpupin|   4 +-
 tools/virsh-domain-monitor.c |   9 +--
 tools/virsh-domain.c | 134 +++
 tools/virsh-host.c   |  57 +++---
 tools/virsh-interface.c  |   6 +-
 tools/virsh-network.c|   6 +-
 tools/virsh-volume.c |  24 ++--
 tools/virsh.c| 111 +--
 8 files changed, 104 insertions(+), 247 deletions(-)

diff --git a/tests/vcpupin b/tests/vcpupin
index ab0d38f..b6b8b31 100755
--- a/tests/vcpupin
+++ b/tests/vcpupin
@@ -34,7 +34,7 @@ fail=0
 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test a 0,1  
out 21
 test $? = 1 || fail=1
 cat \EOF  exp || fail=1
-error: Numeric value for vcpu option is malformed or out of range
+error: Numeric value 'a' for vcpu option is malformed or out of range
 
 EOF
 compare exp out || fail=1
@@ -52,7 +52,7 @@ compare exp out || fail=1
 $abs_top_builddir/tools/virsh --connect test:///default vcpupin test -100 0,1 
 out 21
 test $? = 1 || fail=1
 cat \EOF  exp || fail=1
-error: Numeric value for vcpu option is malformed or out of range
+error: Numeric value '-100' for vcpu option is malformed or out of range
 
 EOF
 compare exp out || fail=1
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index db7ef8b..5b17505 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -340,12 +340,8 @@ cmdDomMemStat(vshControl *ctl, const vshCmd *cmd)
 /* Providing a period will adjust the balloon driver collection period.
  * This is not really an unsigned long, but it
  */
-if ((rv = vshCommandOptInt(ctl, cmd, period, period))  0) {
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- period);
+if ((rv = vshCommandOptInt(ctl, cmd, period, period))  0)
 goto cleanup;
-}
 if (rv  0) {
 if (period  0) {
 vshError(ctl, _(Invalid collection period value '%d'), period);
@@ -1440,9 +1436,6 @@ cmdDomTime(vshControl *ctl, const vshCmd *cmd)
 
 if (rv  0) {
 /* invalid integer format */
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- time);
 goto cleanup;
 } else if (rv  0) {
 /* valid integer to set */
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
index bb8e20a..5094b2a 100644
--- a/tools/virsh-domain.c
+++ b/tools/virsh-domain.c
@@ -1552,9 +1552,6 @@ cmdBlkiotune(vshControl * ctl, const vshCmd * cmd)
 return false;
 
 if ((rv = vshCommandOptInt(ctl, cmd, weight, weight))  0) {
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- weight);
 goto cleanup;
 } else if (rv  0) {
 if (weight = 0) {
@@ -1692,12 +1689,8 @@ blockJobImpl(vshControl *ctl, const vshCmd *cmd,
 if (vshCommandOptStringReq(ctl, cmd, path, path)  0)
 goto cleanup;
 
-if (vshCommandOptULWrap(ctl, cmd, bandwidth, bandwidth)  0) {
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- bandwidth);
+if (vshCommandOptULWrap(ctl, cmd, bandwidth, bandwidth)  0)
 goto cleanup;
-}
 
 switch (mode) {
 case VSH_CMD_BLOCK_JOB_ABORT:
@@ -2216,24 +2209,12 @@ cmdBlockCopy(vshControl *ctl, const vshCmd *cmd)
  * MiB/s, and either reject negative input or treat it as 0 rather
  * than trying to guess which value will work well across both
  * APIs with their different sizes and scales.  */
-if (vshCommandOptULWrap(ctl, cmd, bandwidth, bandwidth)  0) {
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- bandwidth);
+if (vshCommandOptULWrap(ctl, cmd, bandwidth, bandwidth)  0)
 goto cleanup;
-}
-if (vshCommandOptUInt(ctl, cmd, granularity, granularity)  0) {
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- granularity);
+if (vshCommandOptUInt(ctl, cmd, granularity, granularity)  0)
 goto cleanup;
-}
-if (vshCommandOptULongLong(ctl, cmd, buf-size, buf_size)  0) {
-vshError(ctl,
- _(Numeric value for %s option is malformed or out of 
range),
- buf-size);
+if (vshCommandOptULongLong(ctl, cmd, buf-size, buf_size)  0)
 goto cleanup;
-}
 
 if (xml) {
 if (virFileReadAll(xml, VSH_MAX_XML_FILE, xmlstr)  0) {
@@ -2800,12 +2781,8 @@ cmdBlockResize(vshControl *ctl, const vshCmd *cmd)
 if (vshCommandOptStringReq(ctl, cmd, path, (const char **) path)  0)
 return false;
 
-if (vshCommandOptScaledInt(ctl, cmd, size, size, 1024, ULLONG_MAX)  0) 
{
-vshError(ctl,
- _(Numeric value for %s option is malformed or