Cleanup the handling of close().
First of all, closing an invalid (non-negative) file descriptor (EBADF) is
always a serious bug. We want to catch that. Hence, we should use nm_close()
(or nm_close_with_error()) which asserts against such bugs. Don't ever use
close() directly, to get that additional assertion.
Also, our nm_close() handles EINTR internally and correctly. Recent
POSIX defines that on EINTR the close should be retried. On Linux,
that is never correct. After close() returns, the file descriptor is
always closed (or invalid). nm_close() gets this right, and pretends
that EINTR is a success (without retrying).
The majority of our file descriptors are sockets, etc. That means,
often an error from close isn't something that we want to handle. Adjust
nm_close() to return no error and preserve the caller's errno. That is
the appropriate reaction to error (ignoring it) in most of our cases.
And error from close may mean that there was an IO error (except EINTR
and EBADF). In a few cases, we may want to handle that. For those
cases we have nm_close_with_error().
TL;DR: use almost always nm_close(). Unless you want to handle the error
code, then use nm_close_with_error(). Never use close() directly.
There is much reading on the internet about handling errors of close and
in particular EINTR. See the following links:
https://lwn.net/Articles/576478/https://askcodes.net/coding/what-to-do-if-a-posix-close-call-fails-https://www.austingroupbugs.net/view.php?id=529https://sourceware.org/bugzilla/show_bug.cgi?id=14627https://news.ycombinator.com/item?id=3363819https://peps.python.org/pep-0475/
Our GObject structs should be internal API. In which case, we should
embed the private data in the struct themselves (`_priv`) and use the
_NM_GET_PRIVATE() macro. The advantage is better debugability because
following G_TYPE_INSTANCE_GET_PRIVATE() in the debugger is very
cumbersome. Another (less relevant) advantage is better performance.
Thus, warn about uses of g_type_class_add_private() and
G_TYPE_INSTANCE_GET_PRIVATE().
Note that if the struct and is in a header file (which is usually only
necessary when subclassing the type), then the private data should be
an opaque pointer `_priv` instead, and we should use the _NM_GET_PRIVATE_PTR()
macro. In that case, the use of g_type_class_add_private() and
G_TYPE_INSTANCE_GET_PRIVATE() is correct and the warning is false. But
this is only a warning, for the unusual case where we have deep object
hierarchies.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1396
"-u" calls `git diff -name-only $UPSTREAM` to get a list of files.
However, if $UPSTREAM contains a file that doesn't exist in the current
checkout, we get a non-existing file name and clang-format will fail.
Avoid that, by filtering only files that exist.
Also, pass "--no-renames" option to git-diff.
Reformatting the entire source tree takes quite long. For fast
development it's useful to only check the files that changes on the
current checkout.
For that there was already the "-F|--fast" option, but that only
compared the files that changed compared to HEAD^.
What actually would be useful is to check the files that changed on the
current branch, compared to some upstream commit. Add "-u|--upstream"
option to specify the upstream commit (usually "main").
As a special twist,
./contrib/scripts/nm-code-format.sh -u
is the same as
./contrib/scripts/nm-code-format.sh -u main
We need to mount sysfs, so that `ip netns exec` works.
Do that automatically when starting the system container, via rc.local.
While at it, use `podman build --squash-all` to speedup the building of
the container image.
It's between "stop" and "clean". It removes the container,
but keeps the container images. This is to fast restart without
rebuilding the container (image).
- instead of g_str_hash()/g_direct_hash(), use our own functions
nm_str_hash()/nm_direct_hash(). Those use siphash24 with a random
seed.
- don't pass g_direct_equal() to GHashTable. When omitting the equal
function, it falls back to direct pointer comparison, which is likely
faster. In any case, it's consistent to not use g_direct_hash()
when using pointer equality.
- instead of g_int_hash()/g_int64_hash()/g_double_hash(), use
our nm_pint_hash()/nm_pint64_hash()/nm_pdouble_hash(). The latter
two don't exist yet.
The reason is that we want to use siphash24.
Yes, our name differs from glib's. Our naming seems to make sense
to me however, because we also have nm_pstr_hash(), nm_pdirect_hash()
and even nm_ppdirect_hash() for following the pointers. Naming is hard.
- instead of g_int_equal()/g_int64_equal()/g_double_equal() use
our nm_pint_equal()/nm_pint64_equal()/nm_pdouble_equal(). The latter
two don't exist yet. The reason is purely naming consistency since
our hash variants follow the other name.
Leave a hint about core-dumps.
Also, now we have `contrib/fedora/rpm/configure-for-system.sh` script,
which can configure the build in a way similar to what we get
when doing an RPM build.
That means, inside "contrib/scripts/nm-in-container.sh" we
can just type `make install`, and it will replace the pre-installed
NetworkManager.
The main advantage is that it becomes convenient to run NetworkManager
as a systemd service. Previously, the suggested was to to install
NetworkManager inside another prefix, and run it in the terminal.
Running NetworkManager as systemd service is also necessary for NM-ci,
which restarts the NetworkManager service, and you couldn't run a test,
if you just started NetworkManager in a terminal.
Previously, you had to build a complete RPM, which takes a lot of time.
Yes, it's a large dependency. But on your outer host you
probably configured NetworkManager with QT enabled (for the
example scripts). We want to compile the same work tree inside
the container. So install qt-devel.
Otherwise, the path "src/c-stdaux/src/docs/conf.py" matches for
formatting. But this file is imported via git-subtree, we don't
want to format it.
Filter out such paths.
It's just convenient to have some tools around, not only
for testing, but also for (some limited) development.
In particular, because we bind-mount .vimrc inside the container, and
if I use vim, black/clang-format is just one key binding away.
You can of course just clone NetworkManager repository and start hacking
as you like. However, there are a few things like git-notest which are
interesting to setup.
Add a script to do this.
The script is supposed to be idempotent and do nothing, unless
necessary. By default it also only prints what it would do.
"find-backports" script parses the commit messages to figure out which
patches to backport. We use "refs/notes/bugs" notes to extend the
meta data after the commit was merged. If you don't setup the
notes, the output is likely incomplete or wrong.
Yes, this is annoying. It requires you to setup the notes as described
in "CONTRIBUTING.md". Also because the "release.sh" script runs "find-backports",
so that means you cannot do releases without setting up the notes
(unless you manually disable running "find-backports"). But you really shouldn't
make a release based on incomplete information.
"Ignore-Backport:" is already in use. For the find-backports script it
has the same meaning as a "cherry picked from" line, that means, we
assume that the referenced patch was backported already and the fix
applied.
This is of course useful to make the script shut up about backports that
we don't want to do. However, it requires us to tag the old branch
with this, so that the script thinks that the patch is already there.
Imaging we have a wrong commit on "next" branch with a Fixes line. We
don't want to backport it, so we would have to tag the "old" branch with
"Ignore-Backport:". That is cumbersome.
Instead, now also support that if a commit contains a "Fixes:" line any
an "Ignore-Fixes:" for the same fixed commit, then this let's the
"Fixes:" line be ignored.
This is more for completeness, to go along "nm-code-format.sh"
script.
Usually it's very simple to run black directly (you may still do that).
However, black by default only reformats files with ".py" extension.
So to get all our python files, you'd need to know and explicitly
select them... or use this script.
Also, `black .` scans the entire source tree, and is rather slow.
This script knows which files to select and is thus faster.
When we have a GError* variable on the stack, we usually want to pass
it on to function that can fail. In that case, the variable MUST be
initialized to NULL. This is an easy mistake to make.
Note that this check still can have lots of false positives, for
example, if you have a struct with an GError field. In that case, you
would need to ensure that the entire struct is initialized. Ignore the
warning then.
Also, the check misses if you declare multiple variables on one line.
But that is already discouraged by our style.
