8:11 a.m.
Everytime a public API returns an error, libvirtd pollutes
syslog with that error message. Reduce the error logging
level to INFO so these don't appear by default.
* src/util/virterror.c: Log all errors at INFO
---
src/util/virterror.c | 14 +-------------
1 files changed, 1 insertions(+), 13 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c
index d524d04..ecd9fc9 100644
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -64,18 +64,6 @@ void *virUserData = NULL; /* associated data */
}} \
}
-static virLogPriority virErrorLevelPriority(virErrorLevel level) {
- switch (level) {
- case VIR_ERR_NONE:
- return(VIR_LOG_INFO);
- case VIR_ERR_WARNING:
- return(VIR_LOG_WARN);
- case VIR_ERR_ERROR:
- return(VIR_LOG_ERROR);
- }
- return(VIR_LOG_ERROR);
-}
-
static const char *virErrorDomainName(virErrorDomain domain) {
const char *dom = "unknown";
switch (domain) {
@@ -719,7 +707,7 @@ virRaiseErrorFull(virConnectPtr conn ATTRIBUTE_UNUSED,
* Hook up the error or warning to the logging facility
* XXXX should we include filename as 'category' instead of domain name ?
*/
- virLogMessage(virErrorDomainName(domain), virErrorLevelPriority(level),
+ virLogMessage(virErrorDomainName(domain), VIR_LOG_INFO,
funcname, linenr, 1, "%s", str);
/*
--
1.7.2.3
1:10 p.m.
On 11/22/2010 07:11 AM, Daniel P. Berrange wrote:
Everytime a public API returns an error, libvirtd pollutes
syslog with that error message. Reduce the error logging
level to INFO so these don't appear by default.
* src/util/virterror.c: Log all errors at INFO
---
src/util/virterror.c | 14 +-------------
1 files changed, 1 insertions(+), 13 deletions(-)
diff --git a/src/util/virterror.c b/src/util/virterror.c
index d524d04..ecd9fc9 100644
--- a/src/util/virterror.c
+++ b/src/util/virterror.c
@@ -64,18 +64,6 @@ void *virUserData = NULL; /* associated data */
}} \
}
-static virLogPriority virErrorLevelPriority(virErrorLevel level) {
- switch (level) {
- case VIR_ERR_NONE:
- return(VIR_LOG_INFO);
- case VIR_ERR_WARNING:
- return(VIR_LOG_WARN);
- case VIR_ERR_ERROR:
- return(VIR_LOG_ERROR);
- }
- return(VIR_LOG_ERROR);
This logs at VIR_LOG_ERROR if level is ever out of range (not one of the
3 defined virErrorLevel values). Can we ever get virErrorLevel set from
external input, or would an out-of-range enum value represent a bug in
our code? If the former, then it may still be worth keeping this
function, and only mapping the three known levels to VIR_LOG_INFO while
keeping all other values as VIR_LOG_ERROR. If the latter, then this
patch seems fine to me.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
4:23 a.m.
On Mon, Nov 22, 2010 at 12:10:58PM -0700, Eric Blake wrote:
On 11/22/2010 07:11 AM, Daniel P. Berrange wrote:
> Everytime a public API returns an error, libvirtd pollutes
> syslog with that error message. Reduce the error logging
> level to INFO so these don't appear by default.
>
> * src/util/virterror.c: Log all errors at INFO
> ---
> src/util/virterror.c | 14 +-------------
> 1 files changed, 1 insertions(+), 13 deletions(-)
>
> diff --git a/src/util/virterror.c b/src/util/virterror.c
> index d524d04..ecd9fc9 100644
> --- a/src/util/virterror.c
> +++ b/src/util/virterror.c
> @@ -64,18 +64,6 @@ void *virUserData = NULL; /* associated data */
> }} \
> }
>
> -static virLogPriority virErrorLevelPriority(virErrorLevel level) {
> - switch (level) {
> - case VIR_ERR_NONE:
> - return(VIR_LOG_INFO);
> - case VIR_ERR_WARNING:
> - return(VIR_LOG_WARN);
> - case VIR_ERR_ERROR:
> - return(VIR_LOG_ERROR);
> - }
> - return(VIR_LOG_ERROR);
This logs at VIR_LOG_ERROR if level is ever out of range (not one of the
3 defined virErrorLevel values). Can we ever get virErrorLevel set from
external input, or would an out-of-range enum value represent a bug in
our code? If the former, then it may still be worth keeping this
function, and only mapping the three known levels to VIR_LOG_INFO while
keeping all other values as VIR_LOG_ERROR. If the latter, then this
patch seems fine to me.
The levels only ever come from our code. In addition every single
usage is just VIR_ERR_ERROR, except for 3 places in libvirt.c
As such the error levels have no real useful information and it
is simplest to ignore them
Daniel
9:03 a.m.
On 11/23/2010 03:23 AM, Daniel P. Berrange wrote:
> This logs at VIR_LOG_ERROR if level is ever out of range (not one
of the
> 3 defined virErrorLevel values). Can we ever get virErrorLevel set from
> external input, or would an out-of-range enum value represent a bug in
> our code? If the former, then it may still be worth keeping this
> function, and only mapping the three known levels to VIR_LOG_INFO while
> keeping all other values as VIR_LOG_ERROR. If the latter, then this
> patch seems fine to me.
The levels only ever come from our code. In addition every single
usage is just VIR_ERR_ERROR, except for 3 places in libvirt.c
As such the error levels have no real useful information and it
is simplest to ignore them
Sounds reasonable. Patches that remove more than they add are always
fun to justify.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:29 a.m.
On 11/23/2010 08:03 AM, Eric Blake wrote:
On 11/23/2010 03:23 AM, Daniel P. Berrange wrote:
>> This logs at VIR_LOG_ERROR if level is ever out of range (not one of the
>> 3 defined virErrorLevel values). Can we ever get virErrorLevel set from
>> external input, or would an out-of-range enum value represent a bug in
>> our code? If the former, then it may still be worth keeping this
>> function, and only mapping the three known levels to VIR_LOG_INFO while
>> keeping all other values as VIR_LOG_ERROR. If the latter, then this
>> patch seems fine to me.
>
> The levels only ever come from our code. In addition every single
> usage is just VIR_ERR_ERROR, except for 3 places in libvirt.c
> As such the error levels have no real useful information and it
> is simplest to ignore them
Sounds reasonable. Patches that remove more than they add are always
fun to justify.
ACK.
Hmm; this patch causes 'make check' to fail qemuxml2argvtest and
nwfilterxml2xmltest, because the default logging (warn and error) is no
longer triggered by logging at level info, so virtTestLogContentAndReset
no longer captures any failures when it was expected to catch a warning.
I'm not sure whether the fix is to revert this patch, or to teach the
testsuite to catch all messages (info included), since that might result
in noise from other tests where info messages were previously undetected.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:37 a.m.
On Tue, Nov 23, 2010 at 09:29:52AM -0700, Eric Blake wrote:
On 11/23/2010 08:03 AM, Eric Blake wrote:
> On 11/23/2010 03:23 AM, Daniel P. Berrange wrote:
>>> This logs at VIR_LOG_ERROR if level is ever out of range (not one of the
>>> 3 defined virErrorLevel values). Can we ever get virErrorLevel set from
>>> external input, or would an out-of-range enum value represent a bug in
>>> our code? If the former, then it may still be worth keeping this
>>> function, and only mapping the three known levels to VIR_LOG_INFO while
>>> keeping all other values as VIR_LOG_ERROR. If the latter, then this
>>> patch seems fine to me.
>>
>> The levels only ever come from our code. In addition every single
>> usage is just VIR_ERR_ERROR, except for 3 places in libvirt.c
>> As such the error levels have no real useful information and it
>> is simplest to ignore them
>
> Sounds reasonable. Patches that remove more than they add are always
> fun to justify.
>
> ACK.
Hmm; this patch causes 'make check' to fail qemuxml2argvtest and
nwfilterxml2xmltest, because the default logging (warn and error) is no
longer triggered by logging at level info, so virtTestLogContentAndReset
no longer captures any failures when it was expected to catch a warning.
I'm not sure whether the fix is to revert this patch, or to teach the
testsuite to catch all messages (info included), since that might result
in noise from other tests where info messages were previously undetected.
Any test suite that is looking at the log output to decide success
is broken. If it wants to check for whether an error was raised,
it should check that virError is set, not that a message was
logged. Log messages are purely for debugging and not to be relied
on for any actions.
Regards,
Daniel
5113
days inactive
5114
days old
5 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Eric Blake