Re: [libvirt] [PATCH 4/7] Turn virLogSource into a struct instead of an enum
On Fri, Mar 07, 2014 at 10:28:42AM -0700, Eric Blake wrote:
On 03/03/2014 12:18 PM, Daniel P. Berrange wrote:
As part of the goal to get away from doing string matching on
filenames when deciding whether to emit a log message, turn
the virLogSource enum into a struct which contains a log
name. There will eventually be one virLogSource instance
statically declared per source file. To minimise churn in this
commit though, a single global instance is used.
Thanks for separating the meat from the global switchover.
+++ b/src/node_device/node_device_udev.c
@@ -374,7 +374,7 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
format = virBufferContentAndReset(buf);
-virLogVMessage(VIR_LOG_FROM_LIBRARY,
+virLogVMessage(virLogSelf,
So we're basically tossing out the enum of where an error came from, in
favor of a more complex struct that could theoretically once again
include the source information in a later patch.
Yep, exactly.
@@ -104,11 +105,11 @@ void virAuditSend(const char *filename,
if (auditlog str) {
if (success)
-virLogMessage(VIR_LOG_FROM_AUDIT, VIR_LOG_INFO,
+virLogMessage(source, VIR_LOG_INFO, /* XXX */
What's the XXX for, something that gets fixed later?
Currently the audit code forwards on the file/source/function which
raised the audit record, but used VIR_LOG_FROM_AUDIT. So with libvirt
logging you can filter based on the filename. eg if you can see all
log messages from QEMU of which audit records are some submit.
Conversely though if you queried with systemd journal you can (now)
filter based on the log source, which means if you've turned on logging
for QEMU, you could further refine it to just audit records.
With this change we're loosing the ability to refine it via systemd
and I'm unsure of the best way to re-enable that, if at all. It was
kind of an accident that this was even possible, but at the same
time it is a nice accident.
+++ b/src/util/virerror.c
@@ -715,7 +715,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
* hate thus disable that too.
*/
if (virLogGetNbOutputs() 0)
-virLogMessage(virErrorLogPriorityFilter ? VIR_LOG_FROM_FILE :
VIR_LOG_FROM_ERROR,
+virLogMessage(virLogSelf,
Prior to patch 2/7, virLogVMessage behaved differently for
VIR_LOG_FROM_ERROR than for anything else. 2/7 hoisted a condition here
to virRaiseErrorFull, but changed the distinguishing factor to whether
virErrorLogPriorityFilter is set. Now this patch gets rid of the
distinction altogether (since virLogSelf is global), although it could
theoretically be reinstated once you have per-file sources.
Exactly what was that code used for again, and why are we safe using it?
I'd feel a bit more comfortable acking 2/7 and this patch if you can
explain a bit more in the commit message why we are dropping the
distinction based on error source.
In current GIT, the virlog.c file has code to skip logging any messages
with VIR_LOG_FROM_ERROR when no log outputs are prsent, since we don't
want to pollute stderr with this.
The virErrorLogPriorityFilter callback allows libvirtd to reduce the
priority of virErrorPtr triggered logs from 'error' to 'debug'. In
such a case, we do still want to include them in the debug log output.
Because of the hack in virlog.c that checks source == VIR_LOG_FROM_ERROR,
we must change the source to VIR_LOG_FROM_FILE in virerror.c to ensure
they are definitely logged.
Now that patch 2 has pushed the check for log source out of virlog.c
and into the virerror.c caller, we no longer need to munge the log
source at all. So this change should actually have been part of
patch 2, rather than this patch.
@@ -1202,8 +1197,7 @@ virLogOutputToJournald(virLogSource source,
journalAddString(state, MESSAGE, rawstr);
journalAddInt(state, PRIORITY,
virLogPrioritySyslog(priority));
-journalAddString(state, LIBVIRT_SOURCE,
- virLogSourceTypeToString(source));
+journalAddString(state, LIBVIRT_SOURCE, source-name);
Ah - this means that the journal will log per-file information (after
the next patch converts global virLogSelf into per-file), instead of
just the 5 broad categories that virLogSource used to provide.
Yes, though this sort of duplicates the filename logging I think it
is slightly better since we have often renamed files, but we are not
likely to rename log sources much. So people querying the journal
will have slightly better long term stability guarantees by using
the log source to match on.
Regards,
Daniel
--
|: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
Re: [libvirt] [PATCH 4/7] Turn virLogSource into a struct instead of an enum
On 03/03/2014 12:18 PM, Daniel P. Berrange wrote:
As part of the goal to get away from doing string matching on
filenames when deciding whether to emit a log message, turn
the virLogSource enum into a struct which contains a log
name. There will eventually be one virLogSource instance
statically declared per source file. To minimise churn in this
commit though, a single global instance is used.
Thanks for separating the meat from the global switchover.
Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
src/libvirt_private.syms | 1 +
src/node_device/node_device_udev.c | 2 +-
src/qemu/qemu_capabilities.c | 2 +-
src/util/viraudit.c| 7 ---
src/util/viraudit.h| 10 ++
src/util/virerror.c| 2 +-
src/util/virlog.c | 30 --
src/util/virlog.h | 33 -
src/util/virprobe.h| 4 ++--
tests/testutils.c | 2 +-
10 files changed, 45 insertions(+), 48 deletions(-)
+++ b/src/node_device/node_device_udev.c
@@ -374,7 +374,7 @@ udevLogFunction(struct udev *udev ATTRIBUTE_UNUSED,
format = virBufferContentAndReset(buf);
-virLogVMessage(VIR_LOG_FROM_LIBRARY,
+virLogVMessage(virLogSelf,
So we're basically tossing out the enum of where an error came from, in
favor of a more complex struct that could theoretically once again
include the source information in a later patch.
@@ -104,11 +105,11 @@ void virAuditSend(const char *filename,
if (auditlog str) {
if (success)
-virLogMessage(VIR_LOG_FROM_AUDIT, VIR_LOG_INFO,
+virLogMessage(source, VIR_LOG_INFO, /* XXX */
What's the XXX for, something that gets fixed later?
+++ b/src/util/virerror.c
@@ -715,7 +715,7 @@ virRaiseErrorFull(const char *filename ATTRIBUTE_UNUSED,
* hate thus disable that too.
*/
if (virLogGetNbOutputs() 0)
-virLogMessage(virErrorLogPriorityFilter ? VIR_LOG_FROM_FILE :
VIR_LOG_FROM_ERROR,
+virLogMessage(virLogSelf,
Prior to patch 2/7, virLogVMessage behaved differently for
VIR_LOG_FROM_ERROR than for anything else. 2/7 hoisted a condition here
to virRaiseErrorFull, but changed the distinguishing factor to whether
virErrorLogPriorityFilter is set. Now this patch gets rid of the
distinction altogether (since virLogSelf is global), although it could
theoretically be reinstated once you have per-file sources.
Exactly what was that code used for again, and why are we safe using it?
I'd feel a bit more comfortable acking 2/7 and this patch if you can
explain a bit more in the commit message why we are dropping the
distinction based on error source.
@@ -1202,8 +1197,7 @@ virLogOutputToJournald(virLogSource source,
journalAddString(state, MESSAGE, rawstr);
journalAddInt(state, PRIORITY,
virLogPrioritySyslog(priority));
-journalAddString(state, LIBVIRT_SOURCE,
- virLogSourceTypeToString(source));
+journalAddString(state, LIBVIRT_SOURCE, source-name);
Ah - this means that the journal will log per-file information (after
the next patch converts global virLogSelf into per-file), instead of
just the 5 broad categories that virLogSource used to provide.
-typedef enum {
-VIR_LOG_FROM_FILE,/* General debugging */
-VIR_LOG_FROM_ERROR, /* Errors reported */
-VIR_LOG_FROM_AUDIT, /* Audit operations */
-VIR_LOG_FROM_TRACE, /* DTrace probe pointers */
-VIR_LOG_FROM_LIBRARY, /* 3rd party libraries */
+typedef struct _virLogSource virLogSource;
+typedef virLogSource *virLogSourcePtr;
+
+struct _virLogSource {
+const char *name;
+};
So I think this is a fair trade, but still wonder if the commit message
can make it clearer.
@@ -68,7 +67,7 @@ typedef enum {
*
* Do nothing but eat parameters.
*/
-static inline void virLogEatParams(virLogSource unused, ...)
+static inline void virLogEatParams(virLog unused, ...)
Jan pointed out the compilation error here.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/7] Turn virLogSource into a struct instead of an enum
On 03/03/2014 08:18 PM, Daniel P. Berrange wrote:
As part of the goal to get away from doing string matching on
filenames when deciding whether to emit a log message, turn
the virLogSource enum into a struct which contains a log
name. There will eventually be one virLogSource instance
statically declared per source file. To minimise churn in this
commit though, a single global instance is used.
Signed-off-by: Daniel P. Berrange berra...@redhat.com
---
src/libvirt_private.syms | 1 +
src/node_device/node_device_udev.c | 2 +-
src/qemu/qemu_capabilities.c | 2 +-
src/util/viraudit.c| 7 ---
src/util/viraudit.h| 10 ++
src/util/virerror.c| 2 +-
src/util/virlog.c | 30 --
src/util/virlog.h | 33 -
src/util/virprobe.h| 4 ++--
tests/testutils.c | 2 +-
10 files changed, 45 insertions(+), 48 deletions(-)
diff --git a/src/util/virlog.h b/src/util/virlog.h
index 6ba2daa..cb27725 100644
--- a/src/util/virlog.h
+++ b/src/util/virlog.h
@@ -68,7 +67,7 @@ typedef enum {
*
* Do nothing but eat parameters.
*/
-static inline void virLogEatParams(virLogSource unused, ...)
+static inline void virLogEatParams(virLog unused, ...)
s/virLog /virLogSourcePtr /
{
/* Silence gcc */
unused = unused;
Jan
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
