Bug 1086726 - Kill all uses of __FUNCTION__ in error messages
Summary: Kill all uses of __FUNCTION__ in error messages
Keywords:
Status: CLOSED UPSTREAM
Alias: None
Product: Virtualization Tools
Classification: Community
Component: libvirt
Version: unspecified
Hardware: Unspecified
OS: Unspecified
low
low
Target Milestone: ---
Assignee: Eric Blake
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-04-11 11:39 UTC by Shanzhi Yu
Modified: 2015-04-03 12:27 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-04-03 12:27:43 UTC


Attachments (Terms of Use)
Reworked some error messages (3.67 KB, application/mbox)
2015-03-31 18:22 UTC, Noella
no flags Details
__FUNCTION__ discard from error mesages (23.08 KB, patch)
2015-04-01 12:49 UTC, Noella
no flags Details | Diff

Description Shanzhi Yu 2014-04-11 11:39:59 UTC
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:

Comment 1 Eric Blake 2014-04-11 12:13:07 UTC
(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.

Comment 2 Eric Blake 2014-04-11 14:43:06 UTC
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.

Comment 3 Noella 2015-03-31 11:49:00 UTC
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

Comment 4 Noella 2015-03-31 13:02:33 UTC
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?

Comment 5 Noella 2015-03-31 18:22:30 UTC
Created attachment 1009206 [details]
Reworked some error messages

Comment 6 Noella 2015-04-01 12:49:08 UTC
Created attachment 1009645 [details]
__FUNCTION__ discard  from error mesages

this should fix libvirt.c libvirt-domain.c libvirt-domain-snapshot.c

Comment 7 Michal Privoznik 2015-04-03 12:27:43 UTC
I've just pushed the patch upstream:

commit c4db8c5ee3e7bc4559ccde83587ff5318e115865
Author:     Noella Ashu <ashu.noella207@gmail.com>
AuthorDate: Fri Apr 3 11:33:44 2015 +0100
Commit:     Michal Privoznik <mprivozn@redhat.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.
    
    Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1086726

v1.2.14-45-gc4db8c5


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