Bug 1135441 - Error messages are not accurate when running virsh command blockjob by passing async, pivot and bandwidth
Summary: Error messages are not accurate when running virsh command blockjob by passin...
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: libvirt
Version: 7.1
Hardware: x86_64
OS: Linux
unspecified
low
Target Milestone: rc
: ---
Assignee: Erik Skultety
QA Contact: Virtualization Bugs
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-08-29 10:31 UTC by Yang Yang
Modified: 2014-10-24 10:57 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-10-24 10:57:10 UTC
Target Upstream Version:


Attachments (Terms of Use)

Description Yang Yang 2014-08-29 10:31:51 UTC
Description of problem:
Error messages are not accurate when running virsh command blockjob by passing async, pivot and bandwidth

Version-Release number of selected component (if applicable):
libvirt-1.2.7-2.el7.x86_64
qemu-kvm-rhev-2.1.0-3.el7.x86_64

How reproducible:
100%

Steps to Reproduce:
1. Create 4 disk-only snapshots
# virsh snapshot-list test1
 Name                 Creation Time             State
------------------------------------------------------------
 s1                   2014-08-29 17:06:33 +0800 disk-snapshot
 s2                   2014-08-29 17:06:33 +0800 disk-snapshot
 s3                   2014-08-29 17:06:34 +0800 disk-snapshot
 s4                   2014-08-29 17:06:35 +0800 disk-snapshot

2. Do blockcommit
# virsh blockcommit test1 hda --wait --verbose --active
Block Commit: [100 %]
Now in synchronized phase

3. Run blockjob
# virsh blockjob test1 hda  --async --bandwidth 10
error: conflict between --abort, --info, and --bandwidth modes

# virsh blockjob test1 hda  --pivot --bandwidth 10
error: conflict between --abort, --info, and --bandwidth modes


Actual results:
As step 3 and 4 show

Expected results:
The error messages in step 3 and 4 should be more accurate and friendly, like
error: conflict between --async and bandwidth modes
error: conflict between --pivot and bandwidth modes

Additional info:

Comment 2 Eric Blake 2014-08-29 13:59:55 UTC
(In reply to yangyang from comment #0)
> Description of problem:
> Error messages are not accurate when running virsh command blockjob by
> passing async, pivot and bandwidth
> 
> Version-Release number of selected component (if applicable):
> libvirt-1.2.7-2.el7.x86_64
> qemu-kvm-rhev-2.1.0-3.el7.x86_64
> 

> 3. Run blockjob
> # virsh blockjob test1 hda  --async --bandwidth 10
> error: conflict between --abort, --info, and --bandwidth modes

Per the man page:

           Manage active block operations.  There are three modes: --info,
           bandwidth, and --abort; --info is default except that --async or
           --pivot implies --abort.

so you are mixing '--async' (which implies --pivot mode) and --bandwidth (which implies --bandwidth mode).  The error message is correct, even if a bit cryptic.


> The error messages in step 3 and 4 should be more accurate and friendly, like
> error: conflict between --async and bandwidth modes
> error: conflict between --pivot and bandwidth modes

Ugg. I'd rather not litter the code with an exponential explosion of special cases, one for each possible bad combination to print a specific message.  It's not very maintainable.  I'd rather close this as not a bug, unless there is a strong argument to the contrary.  This is NOT affecting correctness, only quality of virsh, and not the sort of thing we need clogging up development time when there are still real correctness bugs to be solved.  But I'll leave this open for a while longer to wait for any other opinions.

Comment 3 Eric Blake 2014-08-29 20:55:57 UTC
On second thought, I'm adding an upstream patch that adds even more mode-specific implications (such as --raw implying --info).  As such, I'm going to change the message to:

error: conflict between abort, info, and bandwidth modes

which should hopefully make it a bit more obvious that the conflict is related to modes, and not necessarily which option implied the mode.

Comment 4 Erik Skultety 2014-10-24 10:57:10 UTC
(In reply to Eric Blake from comment #2)
> Ugg. I'd rather not litter the code with an exponential explosion of special
> cases, one for each possible bad combination to print a specific message. 
> It's not very maintainable.  I'd rather close this as not a bug, unless
> there is a strong argument to the contrary.  This is NOT affecting
> correctness, only quality of virsh, and not the sort of thing we need
> clogging up development time when there are still real correctness bugs to
> be solved.  But I'll leave this open for a while longer to wait for any
> other opinions.

We do have plenty of spots in the code, where this could happen as well which of course should be changed as well to stay consistent in case we really decide to modify the error message, otherwise more and more bug reports of the same nature would come.
As Eric mentioned, it's not very maintainable, especially if you imagine adding a future feature which would need a separate cmd option followed by a special conflict case too.

I'm posting Eric's patch (comment #3) and closing this as not a bug:

commit 2019b7cacadb528820bd3687ef5233aed1449052
Author: Eric Blake <eblake@redhat.com>
Date:   Thu Aug 28 17:23:49 2014 -0600

    blockjob: add new --raw flag to virsh blockjob
    
    The current output of 'blockjob [--info]' is a single line
    designed for human consumption; it's not very nice for machine
    parsing.  Furthermore, I have plans to modify the line in
    response to the new flag for controlling bandwidth units.
    Solve that by adding a --raw parameter, which outputs
    information closer to the C struct.
    
    $ virsh blockjob testvm1 vda --raw
     type=Block Copy
     bandwidth=1
     cur=197120
     end=197120
    
    The information is indented, because I'd like for a later patch
    to add a mode that iterates over all the vm's disks with status
    for each; in that mode, each block name would be listed unindented
    before information (if any) about that block.
    
    Now that we have a raw mode, we can guarantee that it won't change
    format over time.  Any app that cares about parsing the output can
    try --raw, and if it fails, know that it was talking to an older
    virsh and fall back to parsing the human-readable format which had
    not changed until now; meanwhile, when not using --raw, we have
    freed future virsh to change the output to whatever makes sense.
    
    My first change to human mode: this command now guarantees a line
    is printed on successful use of the API, even when the API did
    not find a current block job (consistent with the rest of virsh).
    
    Bonus: https://bugzilla.redhat.com/show_bug.cgi?id=1135441
    complained that this message was confusing:
    
    $ virsh blockjob test1 hda  --async --bandwidth 10
    error: conflict between --abort, --info, and --bandwidth modes
    
    even though the man page already documents that --async implies
    abort mode, all because '--abort' wasn't present in the command
    line.  Since I'm adding another case where options are tied
    to or imply a mode, I changed that error to:
    
    error: conflict between abort, info, and bandwidth modes
    
    * tools/virsh-domain.c (cmdBlockJob): Add --raw parameter; tweak
    error wording.
    * tools/virsh.pod (blockjob): Document it.
    
    Signed-off-by: Eric Blake <eblake@redhat.com>

v1.2.8-38-g2019b7c


Note You need to log in before you can comment on or make changes to this bug.