Description of problem: The error output of snapshot-revert should be more friendly Version-Release number of selected component (if applicable): libvirt-1.1.1-29.el7.x86_64 How reproducible: 100% Steps to Reproduce: 1.prepare an guest and keep it running # virsh list --name --state-running rhel6 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 Actual results: Expected results: The error output can be more friendly. There is no need to show up virDomainRevertToSnapshot to user. Additional info:
(In reply to Shanzhi Yu from comment #0) > #virsh snapshot-revert rhel6 s1 --running --paused > error: running and paused flags in virDomainRevertToSnapshot are mutually > exclusive > > 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.
Hello everyone, 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. Thanks, Noella
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: commit c4db8c5ee3e7bc4559ccde83587ff5318e115865 Author: Noella Ashu <ashu.noella207> AuthorDate: Fri Apr 3 11:33:44 2015 +0100 Commit: Michal Privoznik <mprivozn> 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. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1086726 v1.2.14-45-gc4db8c5