see

danw/logfixes-bgo744031
danw/libnm-core-shared-bgo744031

Hmm, could we still keep 'func' for debugging statements?  I don't think we care about file + line at all, but I still find the function useful when looking at debug logs.  At least since it woudl be limited to debug messages it wouldn't bloat the binary too much maybe?

>> core: drop extra info from trace/debug/error logging

I agree with changing the log format and no longer printing "file:line func()" for syslog.
    
-         fullmsg = g_strconcat ("<info>  ", msg, NULL);
+         tag = "<info>";
-         fullmsg = g_strconcat ("<warn>  ", msg, NULL);
+         tag = "<warn>";

could we preserve the extra whitspace? then all the "<LEVELS>" have the same width and the logging is aligned.


However, I would not remove the arguments from _nm_log(). Note that we probably have these strings already in the binary, so removing it from the signature would not reduce the binary size (much) -- did you measure it? Does it reduce the number of strings effectively?
Later, we want to support journald logging, there we should pass on those lines as separate fields.
See http://cgit.freedesktop.org/systemd/systemd/tree/src/shared/log.c?id=a88abde72169ddc2df77df3fa5bed30725022253#n462

-- or alternatively, split the commit in two parts. One changing the logging format, one removing the arguments. That way, we can revert one part later.


>> system-dhcp: fix logging
see also: th/systemd-dhcp-integration, http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=51a95f12090136070448567f7b1220d56e0e37dc

>> libnm-core: redo how nm-core-internal.h works
    
 libnm_1_2_0 {
 global:
+    _nm_core_internal_functions_get;


how about adding this function (and possibly further future ones) to a separate entry? It is explicitly private and not ABI stable.
So, we would reserve the right, to break ABI for these symbols.

 _libnm_priv {
 global:
      _nm_core_internal_functions_get;
 }





do we actually need the getter? How about directly exposing 

-NMCoreInternalFunctions *_nm_core_internal_functions_get (void);
+extern const NMCoreInternalVTable *const _nm_core_vtable;
        ^^^^^??


>> libnm, core: build libnm-core-private.so and use it

looks good. I pushed a fixup. 
ignoring "-Wmissing-prototypes" can be skipped if we generate the prototypes too. clang didn't fail on this, but still.
also, no need to use g_assert. Just core dump directly.


didn't do much testing though. I scheduled a jenkins build (#892)

(In reply to Thomas Haller from comment #4)
> how about adding this function (and possibly further future ones) to a
> separate entry? It is explicitly private and not ABI stable.

The reason I didn't do that is because the new section would show up as provided by the RPM. Eg:

  libnm.so.0()(64bit)
  libnm.so.0(libnm_1_0_0)(64bit)
  libnm.so.0(libnm_1_2_0)(64bit)
  libnm.so.0(_libnm_priv)(64bit)

*Explicitly* declaring that we supported something non-ABI-stable seemed weird, so I opted to just sneak it into an existing section.

(Actually... I didn't test this... maybe there's some way [underscore-prefixing or otherwise] to keep a section out of the provides. Though ISTR from the "libnm_local" idea that there isn't.)

> also, no need to use g_assert. Just core dump directly.

"core dump directly" meaning what? "*NULL=1"?

The code should not be reached, so g_assert_not_reached() seems correct.


Also, FTR, the branch is not quite working; trying to activate a VPN ends up hitting one of the assert-not-reached wrapper functions rather than the underlying function, for some reason. (Symbols not getting resolved as expected when loading the VPN plugin I guess?)

(In reply to Dan Winship from comment #5)
> (In reply to Thomas Haller from comment #4)
> > how about adding this function (and possibly further future ones) to a
> > separate entry? It is explicitly private and not ABI stable.
> 
> The reason I didn't do that is because the new section would show up as
> provided by the RPM. Eg:
> 
>   libnm.so.0()(64bit)
>   libnm.so.0(libnm_1_0_0)(64bit)
>   libnm.so.0(libnm_1_2_0)(64bit)
>   libnm.so.0(_libnm_priv)(64bit)
> 
> *Explicitly* declaring that we supported something non-ABI-stable seemed
> weird, so I opted to just sneak it into an existing section.
> 
> (Actually... I didn't test this... maybe there's some way
> [underscore-prefixing or otherwise] to keep a section out of the provides.
> Though ISTR from the "libnm_local" idea that there isn't.)

but wouldn't that allow you to see, that something is wrong -- if you reference any symbol from _libnm_priv (except libnm.so and NetworkManager).


> > also, no need to use g_assert. Just core dump directly.
> 
> "core dump directly" meaning what? "*NULL=1"?
>
> The code should not be reached, so g_assert_not_reached() seems correct.

raise (SIGABRT);

Ah, forgot to mention, I pushed a fixup! The fixup reduces the size of the wrapper binary, while g_assert_not_reached() gives some more details, SIGABRT seems good enough.

removed obsolete branch danw/logfixes-bgo744031

bugzilla.gnome.org is being shut down in favor of a GitLab instance. 
We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time.

If you still use NetworkManager and if you still see this bug / want this feature in a recent and supported version of NetworkManager, then please feel free to report it at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/

Thank you for creating this report and we are sorry it could not be implemented (workforce and time is unfortunately limited).