[libvirt] [PATCH 4/7] Turn virLogSource into a struct instead of an enum

[Daniel P. Berrange]

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 :|