GError codes are only unique per domain, so logging the code without
also indicating the domain is not helpful. And anyway, if the error
messages are not distinctive enough to tell the whole story then we
should fix the error messages.
Based-on-patch-by: Dan Winship <danw@gnome.org>
In dispatcher, we install a log-handler which maps G_LOG_LEVEL_MESSAGE
to syslog priority LOG_NOTICE, which in turn causes journal to highlight
the message. We don't want that so instead use g_info() and g_debug()
which maps to lower syslog levels.
There is only one problem, in debug-mode, we don't use syslog but the
default logging handler from glib. In this case, we have to set
G_MESSAGES_DEBUG otherwise g_info()/g_debug() is suppressed.
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
- "gsystem-local-alloc.h" and <gio/gio.h> are already included via
"nm-default.h". No need to include them separately.
- include "nm-macros-internal.h" via "nm-default.h" and drop all
explict includes.
- in the modified files, ensure that we always include "config.h"
and "nm-default.h" first. As second, include the header file
for the current source file (if applicable). Then follow external
includes and finally internal nm includes.
- include nm headers inside source code files with quotes
- internal header files don't need to include default headers.
They can savely assume that "nm-default.h" is already included
and with it glib, nm-glib.h, nm-macros-internal.h, etc.
The request->scripts are allocated in handle_action() and request_free() is
only called after that, so it never sees it NULL. Coverity knows:
CID 59385 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking request->scripts suggests that it may be null,
but it has already been dereferenced on all paths leading to the check.
Previously, we would log messages like:
#3 'pre-up' [tun0]: new request (1 scripts)
Journald interprets the "#" prefix as a severity level and
highlights them.
Commit ac6522bbf1 moved logging of the environment
after the check for @error_message to avoid a crash. However, in case of no
scripts (request->scripts->len == 0) we still want to log the environment,
so move it back to where it was, but avoid the crash.
complete_request() possibly frees the request, making the @script and @request
a dangling pointer. We must not use those pointers, except a plain pointer-
comparison.
The request could have been completed by the call to complete_request() just above.
In that case we should just return instead of looking for nowait scripts to run.
Core was generated by `/usr/libexec/nm-dispatcher'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007f6c916ad0fb in complete_script (script=<error reading variable: value has been optimized out>, script=<error reading variable: value has been optimized out>) at nm-dispatcher.c:349
349 if ( script->request->num_scripts_nowait == 0
(gdb) bt
#0 0x00007f6c916ad0fb in complete_script (script=<error reading variable: value has been optimized out>, script=<error reading variable: value has been optimized out>) at nm-dispatcher.c:349
#1 0x00007f6c8f02d484 in g_child_watch_dispatch (source=<optimized out>, callback=<optimized out>, user_data=<optimized out>) at gmain.c:5148
#2 0x00007f6c8f03079a in g_main_context_dispatch (context=0x7f6c9358d9a0) at gmain.c:3109
#3 0x00007f6c8f03079a in g_main_context_dispatch (context=context@entry=0x7f6c9358d9a0) at gmain.c:3708
#4 0x00007f6c8f030ae8 in g_main_context_iterate (context=0x7f6c9358d9a0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3779
#5 0x00007f6c8f030dba in g_main_loop_run (loop=0x7f6c9358da80) at gmain.c:3973
#6 0x00007f6c916abcd4 in main (argc=1, argv=0x7ffee4326768) at nm-dispatcher.c:935
(gdb)
Fixes: e97a334e37https://bugzilla.redhat.com/show_bug.cgi?id=1297826
request_free() can be called on an error.
Oct 27 20:27:46 localhost.localdomain nm-dispatcher[2328]: #1 'pre-up': completed: invalid request: Missing or invalid required value path!
Oct 27 20:27:46 localhost.localdomain nm-dispatcher[2328]: **
Oct 27 20:27:46 localhost.localdomain nm-dispatcher[2328]: ERROR:nm-dispatcher.c:203:request_free: assertion failed (request->num_scripts_done == request->scripts->len): (0 == 1)
Fixes: 1999723241https://bugzilla.redhat.com/show_bug.cgi?id=1275813
Refactor logging so that all logging lines for a request/script
have the same prefix, indicating the event to which they belong.
Previously, we only scheduled one script at a time so it was always
clear for which request a script belongs. Now we schedule scripts
in parallel, so we must know the event (request) for which we log
a message.
Previously, dispatcher would start all "no-wait" script right away,
but it would also start the first "wait" script while the "no-wait"
script are still running (unless, there are other "wait" scripts that
delay the execution).
Now, do not start processing the "wait" scripts, as long as there are
any "no-wait" scripts from the same request running.
When a script is a symbolic link to the 'no-wait.d' subdirectory, the
dispatcher now schedules it immediately and in parallel with other
no-wait scripts.
https://bugzilla.gnome.org/show_bug.cgi?id=746703
The first argument for dispatcher actions is the interface name, except
for the "hostname" action, where no interface is available.
For "hostname" actions continue to pass "none", as there is no interface
available. But for device actions, it may happen that the interface name
is missing too. In this case, don't pass "none" but instead an empty
name.
Rather than randomly including one or more of <glib.h>,
<glib-object.h>, and <gio/gio.h> everywhere (and forgetting to include
"nm-glib-compat.h" most of the time), rename nm-glib-compat.h to
nm-glib.h, include <gio/gio.h> from there, and then change all .c
files in NM to include "nm-glib.h" rather than including the glib
headers directly.
(Public headers files still have to include the real glib headers,
since nm-glib.h isn't installed...)
Also, remove glib includes from header files that are already
including a base object header file (which must itself already include
the glib headers).
The initsystem should stop nm-dispatcher before stopping D-Bus
service. However, on some systems that is not implemented, so
nm-dispatcher should not exit with a failure message when the
system bus disappears.
Instead just assume that D-Bus service was stopped during shutdown
and exit gracefully.
Based-on-patch-by: Jacob <jacobgodserv@gmail.com>
https://bugzilla.gnome.org/show_bug.cgi?id=751017
There's no point in calling setpgid() on short-lived processes, so
remove the setpgid() calls when spawning dispatcher scripts, iptables,
iscsiadmin, and netconf.
config.h should be included from every .c file, and it should be
included before any other include. Fix that.
(As a side effect of how I did this, this also changes us to
consistently use "config.h" rather than <config.h>. To the extent that
it matters [which is not much], quotes are more correct anyway, since
we're talking about a file in our own build tree, not a system
include.)
Especially now that we have the 'pre-up.d/' and 'pre-down.d/' directories,
silently skip over any sub directories inside the dispatcher directory.
Fixes warning:
nm-dispatcher: Cannot execute '/etc/NetworkManager/dispatcher.d/pre-up.d': not a regular file.
Signed-off-by: Thomas Haller <thaller@redhat.com>
To ensure that NetworkManager does not block needlessly for events
which have no scripts, require scripts that respond to blocking
events to opt into the action.
If there are no dispatcher scripts, don't bother dispatching any
events. This saves some time configuring networking if the event
would have no effect anyway.
