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 766816 - [review] improve debugging for VPN plugins by NetworkManager enabling debug logging for plugins [th/vpn-plugin-debug-bgo766816]
[review] improve debugging for VPN plugins by NetworkManager enabling debug l...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: VPN (general)
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2016-05-23 17:07 UTC by Thomas Haller
Modified: 2016-05-24 20:49 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Thomas Haller 2016-05-23 17:07:49 UTC
Please review
Comment 1 Thomas Haller 2016-05-23 17:10:37 UTC
Branch th/vpn-plugin-debug-bgo766816, both for NM and plugin.


NM: https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=th/vpn-plugin-debug-bgo766816


and as example,
nm-openvpn: https://git.gnome.org/browse/network-manager-openvpn/log/?h=th/vpn-plugin-debug-bgo766816
Comment 2 Dan Williams 2016-05-24 14:11:51 UTC
There's nm_logging_enabled_syslog() and nm_logging_syslog_enabled() which are similarly named but do very different things.  No strong feeling, but maybe a better name for one of them?

> logging: add new logging domain LOGD_VPN_PLUGIN

adviced -> advised


otherwise LGTM
Comment 3 Thomas Haller 2016-05-24 14:23:13 UTC
(In reply to Dan Williams from comment #2)
> There's nm_logging_enabled_syslog() and nm_logging_syslog_enabled() which
> are similarly named but do very different things.  No strong feeling, but
> maybe a better name for one of them?

We have nm_logging_enabled(), and nm_logging_enabled_syslog() is similar to that, except that instead of returning TRUE/FALSE, it returns the most-verbose syslog level for the given domain.

And we have nm_logging_syslog_openlog() and now nm_logging_syslog_enabled(). The first configures syslog, the second returns whether syslog is enabled.
Comment 4 Dan Williams 2016-05-24 14:28:30 UTC
OK.
Comment 5 Thomas Haller 2016-05-24 17:05:11 UTC
Before this change, nm-openvpn always logs some messages like
  _LOG_I("openvpn[%ld] started"...);
If you manually spawn the plugin with --debug, it would also enable verbose debug-logging of the openvpn service, plus some additional debugging statements by nm-openvpn itself.


Now, with this change, NetworkManager will always set NM_VPN_LOG_LEVEL for the plugin. And by default, unless the user explicitly enables it via

   nmcli general logging level KEEP domains VPN_PLUGIN:INFO

it will be NM_VPN_LOG_LEVEL=0. It defaults to 0, because on higher levels the plugin might log sensitive information to our logfile.



But I also changed that statements like
  _LOG_I("openvpn[%ld] started"...);
obey NM_VPN_LOG_LEVEL, because it's more consistent.

The notable result of that is, that the plugin is now almost silent by default. I guess that is consistent and makes sense. But something to be aware of.
Comment 6 Dan Williams 2016-05-24 17:13:15 UTC
I didn't hceck the VPN plugins themselves, but does --debug still have the old behavior?
Comment 7 Thomas Haller 2016-05-24 17:33:25 UTC
(In reply to Dan Williams from comment #6)
> I didn't hceck the VPN plugins themselves, but does --debug still have the
> old behavior?

Yes it does. In absence of NM_VPN_LOG_LEVEL, the old behavior is mostly identical.
Comment 8 Thomas Haller 2016-05-24 20:49:03 UTC
Added a few improvements to the patches and merged the branches.


NM master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9cf1dbcaefd6128cf8c77cb4960cd57f972c6da0

nm-openvpn master: https://git.gnome.org/browse/network-manager-openvpn/commit/?id=0a8fa06d5e953f402585051717cfb7f2ce441c4c


Logging for VPN plugins shall be enabled with:

  nmcli general logging level KEEP domains VPN_PLUGIN:TRACE

(very nice)


Other VPN plugins should be adjusted too.