Description of problem:
The error output of snapshot-revert should be more friendly
Version-Release number of selected component (if applicable):
Steps to Reproduce:
1.prepare an guest and keep it running
# virsh list --name --state-running
2.create two internal system checkpoint snapshot of the guest
#virsh snapshot-create-as rhel6 s1
Domain snapshot s1 created
#virsh snapshot-create-as rhel6 s2
Domain snapshot s2 created
3. try to revert snapshot with exclusive options
#virsh snapshot-revert rhel6 s1 --running --paused
error: running and paused flags in virDomainRevertToSnapshot are mutually exclusive
The error output can be more friendly.
There is no need to show up virDomainRevertToSnapshot to user.
(In reply to Shanzhi Yu from comment #0)
> #virsh snapshot-revert rhel6 s1 --running --paused
> error: running and paused flags in virDomainRevertToSnapshot are mutually
> Expected results:
> The error output can be more friendly.
> There is no need to show up virDomainRevertToSnapshot to user.
What exactly do you propose? I'm not in the mood to duplicate error checking that normally occurs in virDomainRevertToSnapshot to also occur in virsh - one of the fundamental principles of virsh is that it should ALLOW conflicting flags to be passed into underlying API so that we can prove the underlying API is properly rejecting the conflicting flags.
I'm inclined to close this as WONTFIX.
Alternatively, we could use this bug as a reason to kill all use of __FUNCTION__ in the body of error messages in src/libvirt.c - after all, virError already includes __FUNCTION__ information in a separate member of the struct, so repeating it in the message is redundant and leads to situations where higher level code ends up reporting the lower level name and thus bug reports like this.
I will love to work on this bug. Looking at /src/util/virerror.* I'm thinking of reworking some of the functions to remove references to the __FUNC__ arguments for string message errors. i would like to know which particular functions where this is necessary.
I am working on the above bug to remove all references to __FUNC__ in error messages. While trying to rework virError to remove __FUNC__ args in error message. I searched for src/libvirt* files containing __FUNC__ args and come across libvirt_domain.c only with references to this. While following +Eric's comments to kill all __FUNC__ references in src/libvirt.c. Only virReportErrorHelper has references to __FUNC__ arguments. Dunno if it's ok to rework only these(libvirt-domain.c, libvirt.c) or is there anything else i may be missing?
Created attachment 1009206 [details]
Reworked some error messages
Created attachment 1009645 [details]
__FUNCTION__ discard from error mesages
this should fix libvirt.c libvirt-domain.c libvirt-domain-snapshot.c
I've just pushed the patch upstream:
Author: Noella Ashu <firstname.lastname@example.org>
AuthorDate: Fri Apr 3 11:33:44 2015 +0100
Commit: Michal Privoznik <email@example.com>
CommitDate: Fri Apr 3 14:02:54 2015 +0200
libvirt: virsh: Kill all uses of __FUNCTION__ in error messages
The error output of snapshot-revert should be more friendly.
There is no need to show virDomainRevertToSnapshot to user.
virReportError already includes __FUNCTION__ information in a
separate member of the struct, so repeating it in the message is
redundant and leads to situations where higher level code ends up
reporting the lower level name. We correctly converted the error
output making it more succinct and user-friendly.