After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 752136 - [review] support also systemd journald for nm-logging [th/logging-sd-journal-bgo752136]
[review] support also systemd journald for nm-logging [th/logging-sd-journal-...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-07-08 18:38 UTC by Thomas Haller
Modified: 2015-07-14 16:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Description Thomas Haller 2015-07-08 18:38:48 UTC 
Please review
Comment 1 Thomas Haller 2015-07-08 18:39:37 UTC 
branch: th/logging-sd-journal-752136
Comment 2 Thomas Haller 2015-07-09 15:22:38 UTC 
(In reply to Thomas Haller from comment #1)
> branch: th/logging-sd-journal-752136

typo: th/logging-sd-journal-bgo752136
Comment 3 Beniamino Galvani 2015-07-10 12:57:49 UTC 
> build: detect systemd-journald support

+       if test "$have_systemd_logind" != "yes"; then
+               if test "$with_systemd_journal" = "yes"; then

s/have_systemd_logind/have_systemd_journal/

> logging: add compile time default for logging.backend configuration

I wonder if this is useful now that distros can place default
configuration options in /usr/lib. But maybe yes...
Comment 4 Thomas Haller 2015-07-10 13:06:47 UTC 
(In reply to Beniamino Galvani from comment #3)
> > build: detect systemd-journald support
> 
> +       if test "$have_systemd_logind" != "yes"; then
> +               if test "$with_systemd_journal" = "yes"; then
> 
> s/have_systemd_logind/have_systemd_journal/

fixed.


> > logging: add compile time default for logging.backend configuration
> 
> I wonder if this is useful now that distros can place default
> configuration options in /usr/lib. But maybe yes...

IMHO yes. Optimally, you don't need any configuration at all you achieve the default behavior on a distribution. Configuration should only be necessary to get non-defaults.


Repushed
Comment 5 Dan Williams 2015-07-12 01:07:06 UTC 
Overall looks fine to me, just one question about "logging: add "journal-syslog-style" logging backend to log the old format".  What's the use-case for this?  Couldn't we just say that if you choose to use systemd logging, you get the structured messages?
Comment 6 Thomas Haller 2015-07-12 11:44:02 UTC 
(In reply to Dan Williams from comment #5)
> Overall looks fine to me, just one question about "logging: add
> "journal-syslog-style" logging backend to log the old format".  What's the
> use-case for this?  Couldn't we just say that if you choose to use systemd
> logging, you get the structured messages?

Our current "syslog" format has information like
  "[timestamp] file:line [func()]"
While I dislike that format because it causes the logging lines no being aligned, it might be better for us when a user reports a logfile.

Maybe on Fedora we should keep using "journal-syslog-style" by default. That way, when a user does plain `journalctl` he still gets the well known, more detailed, information.
Comment 7 Beniamino Galvani 2015-07-14 13:25:36 UTC 
> build: detect systemd-journald support

+if test "$have_systemd_journal" = "yes"; then
+       AC_SUBST(SYSTEMD_JOURNAL_CFLAGS)
+       AC_SUBST(SYSTEMD_JOURNAL_LIBS)

I'm not sure, but I think these can be omitted as already called by PKG_CHECK_MODULES. I would do this:

  http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?h=bg/audit-bgo749364&id=fdb75a7c754cbab107c0c2b80932810b5eba9c55

> logging: add native systemd-journald support to nm-logging

+ /* construct a list of all domains (not only the enabled onces).

s/onces/ones/

The rest looks good to me.