Re: [libvirt] [PATCH v3 0/5] virsh: Further improve handling of integer options
On 05/28/2015 05:31 AM, Andrea Bolognani wrote:
As suggested by Michal: now that we have a generic error message for
failures related to the parsing of integer options, it makes sense to
perform the corresponding check in a single spot instead of replicating
it every time vshCommandOpt*() is used.
Andrea Bolognani (5):
virsh: Use standard error message in vshCommandOptTimeoutToMs().
virsh: Improve vshCommandOptTimeoutToMs().
virsh: Make vshCommandOptScaledInt() use vshCommandOpt().
virsh: Pass vshControl to all vshCommandOpt*() calls.
virsh: Move error messages inside vshCommandOpt*() functions.
tests/vcpupin | 4 +-
tools/virsh-domain-monitor.c | 17 +--
tools/virsh-domain.c | 226 ++++++++++++---------------------------
tools/virsh-host.c | 67 +++---------
tools/virsh-interface.c | 6 +-
tools/virsh-network.c | 10 +-
tools/virsh-nodedev.c | 4 +-
tools/virsh-snapshot.c | 2 +-
tools/virsh-volume.c | 26 +----
tools/virsh.c | 248 +++++++++++++++++++++++++------------------
tools/virsh.h | 66 ++++++------
11 files changed, 278 insertions(+), 398 deletions(-)
Patch 1 was fine
Patch 2 had some comments which should be simple to fix
Patch 3 was previously ACK'd by Michal (v2 1/3)
Patch 4 had a couple of NIT's about going beyond 80 columns on a line,
but since it's so pervasive in the rest of the module that can be a
future cleanup ;-)... I do note that 'vshCommandOpt' now has an
unrelated difference - I assume it had a ctl argument at one point
that's since been removed... In any case, ACK'd by Michal module OptBool
which appears to have been addressed (v2 2/3)
Patch 5 still seemed to need some sort of error message in
vshCommandOptString... Some callers ignore return status, so even adding
an error may still be "ignored".
Overall seems OK to me with some minor cleanups.
John