These nmcli calls are inside a Util.iter_nmcli_output_modes()
loop, hence, --terse will passed to them already.
Specifying --terse more than once, causes nmcli to fail.
We didn't actually want to test how nmcli rejects such
duplicate arguments. Adjust the test.
Add more tests for various output modes of nmcli.
This most interestingly includes a terse output for
$ nmcli device status
Which was not tested previously (but a later commit will change
behavior here).
This blows up the number of tests even further.
Previously, the test invoked nmcli 850 times (taking ~15 seconds
ony my machine), afterwards, it is 1334 times (taking ~20 seconds).
No doubt, this seems excessive and questionable.
However, I maintain that the computer doesn't mind running a lot of
redundant tests. The important thing is, that we cover as many cases
as possible. Trying at the same time to minimize the number of tests
by avoiding duplicates and redundant tests, is just a lot of manual
labor. Manual labor that must be repeated whenever new tests are added, to
ensure that this test case is not yet covered. So, I am fine
with the large number of tests. Let the computer do the work.
Instead of letting each nmcli run write an individual .expected file,
combine the output of multiple runs in one file (per test).
Advantages:
- since there is a very large number of tests, having a file for each
tests is cumbersome. For example, since they are all added to
$(EXTRA_DIST) (and since we use non-recursive make), autoconf can
easily hit a length limit when processing all the file names.
- previously, whenever we added tests, all .expected files shifted
and the diff was large. Now, there is a chance that the diff is
smaller and more accurate.
It seems, curl_multi_socket_action() can fail with
connectivity check failed: 4
where "4" means CURLM_INTERNAL_ERROR.
When that happens, it also seems that the file descriptor may still have data
to read, so the glib IO callback _con_curl_socketevent_cb() will be called in
an endless loop. Thereby, keeping the CPU busy with doing nothing (useful).
Workaround by disabling polling on the file descriptor when something
goes wrong.
Note that optimally we would cancel the affected connectivity-check
right away. However, due to the design of libcurl's API, from within
_con_curl_socketevent_cb() we don't know which connectivity-checks
are affected by a failure on this file descriptor. So, all we can do
is avoid polling on the (possibly) broken file descriptor. Note that
we anyway always schedule a timeout of last resort for each check. Even
if something goes very wrong, we will fail the check within 15 seconds.
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=903996
On non-Windows, libcurl's "curl_socket_t" type is just a typedef for
int. We rely on that, because we use it as file descriptor.
Add a compile time check to ensure that.
Since this is "C" there are not namespaces and libraries commonly choose
a particular name prefix for their symbols.
In case of libcurl, that is "curl_".
We should avoid using the same name prefix, and choose something distinct.
Before:
$ nmcli connection up my-wired
Error: Connection activation failed: No suitable device found for this connection.
After:
$ nmcli connection up my-wired
Error: Connection activation failed: No suitable device found for this connection (device eth0 not available because device has no carrier).
This relies on nm_manager_get_best_device_for_connection() giving a
suitable error. That is however a bit complicated, because if no
suitable device is found, it's not immediately clear what is the
exact reason. E.g. if you try to activate a Wi-Fi profile, the
failure reason
"SSID is not visible"
is better than
"Wi-Fi profile cannot activate on ethernet device".
This is controlled by carefully setting the failure codes
NM_UTILS_ERROR_CONNECTION_AVAILABLE_* to indicate an absolute
relevance of the failure. And subsequently, by selecting the failure
with the highest relevance. This might still need some improvements,
for example by reordering checks (so that more relevant failures
are handled first) and tweaking the error relevance.
Before:
$ nmcli connection up my-wired ifname eth0
Error: Connection activation failed: Connection 'my-wired' is not available on the device eth0 at this time.
After:
$ nmcli connection up my-wired ifname eth0
Error: Connection activation failed: Connection 'my-wired' is not available on device eth0 because device has no carrier
Still unused, but will be used to give a better failure reason when
no device is found.
The difficulty here is to select the failure message from the most appropriate
device. This might still need some tweaking by setting the error codes accordingly
and re-ordering checks so that failure cares that are more accurate are handled
first.
Note the special error codes NM_UTILS_ERROR_CONNECTION_AVAILABLE_*.
This will be used to determine, whether the profile is fundamentally
incompatible with the device, or whether just some other properties
mismatch. That information will be importand during a plain `nmcli
connection up`, where NetworkManager searches all devices for a device
to activate. If no device is found (and multiple errors happened),
we want to show the error that is most likely relevant for the user.
Also note, how NMDevice's check_connection_compatible() uses the new
class field "device_class->connection_type_check_compatible" to simplify
checks for compatible profiles.
The error reason is still unused.
It seems to me the NM_DEVICE_CLASS_DECLARE_TYPES() macro confuses more
than helping. Let's explicitly initialize the two fields, albeit with
another helper macro NM_DEVICE_DEFINE_LINK_TYPES() to get the list of
link-types right.
For consistency, also leave nop-lines like
device_class->connection_type_supported = NULL;
device_class->link_types = NM_DEVICE_DEFINE_LINK_TYPES ();
because all NMDevice class init methods should have this same
boiler plate code and to make it explicit that this is intended.
And there are only 3 occurences where this actually comes into play.
NMDeviceOvsPort and NMDeviceOvsInterface don't have an underlying link-type from platform.
Still use NM_DEVICE_CLASS_DECLARE_TYPES() macro, for consistancy reasons.
This requires to extend NM_DEVICE_CLASS_DECLARE_TYPES() macro, to support
a variadic argument list with zero link-types.
The majority of device implementations name their parent-class variable
"device_class". That also makes more sense as it is more consistant.
E.g. "parent" sounds like it's the direct parent, but that is not
the crucial point here. The crucial point at this place, is that we
access the NMDeviceClass typed pointer. Rename.
We also have NMDeviceClass.check_connection_compatible(). It is preferable
to use unique names, especially for the virtual function table. A reasonable
thing to do is grep for the function name to find all places that implement
this function. But if different classes use the same name, grep just
turns up annoying false positives.
Extend the lame-ass dhclient.conf parser to ignore the blocks we can't
do anything useful about: alias{}, pseudo{} and even lease{}.
Note that there's still a lot of cases we can't handle without a
full-fledged dhclient.conf parser -- notably the files that don't use
line breaks to separate the statements.
That is probably okay -- the whole thing is probably mostly useless and
we shall ever bother only about cases that actually cause trouble.
https://github.com/NetworkManager/NetworkManager/pull/153
We only have a certain granularity of how our headers in "shared/nm-utils"
can be used independently.
For example, it's not supported to use "nm-macros-internal.h" without
"gsystem-local-alloc.h". Likewise, you cannot use "nm-glib.h" directly,
you always get it together with "nm-macros-internal.h".
This is, we don't support to use certain headers entirely independently,
because usually you anyway want to use them together.
As such, no longer support "gsystem-local-alloc.h", but merge the
remainder into "nm-macros-internal.h". There is really no reason
to support arbitrary flexibility of including individual bits. You
want cleanup-macros? Include "nm-macros-internal.h".
Merge the headers.
These cleanup macros are unused by NetworkManager code. Note that
since "shared/nm-utils" is used by applet and VPN plugins, theoretically,
they could be used there. I didn't check that, but breaking API of
"shared/nm-utils" is fine (as long as we catch it with a compilation
error).
Historically, we use libgsystem's gsystem-local-alloc header and their
"gs_*" macros. However, they are not really our style and don't have
a nm-prefix (like the rest of our code). We keep the gs_ names, because
they are wildly used and because we wanted to keep gsystem-local-alloc
in sync with upstream (which is no longer the case).
Our own cleanup macros are always called "nm_auto_*". So, at least
for the unused "gs_*" macros, rename them to "nm_auto_*".
Don't drop them, despite they being unused. The reason is, that we should
make use of cleanup functions more eagerly. Dropping them now -- because
they are momentarily unused -- hampers using them in the future. We
often don't use the cleanup macros at places where I think we should,
so by dropping them, we hamper future use.
Drop unused "gs_fd_close" macro, now that we no longer care about keeping
"gsystem-local-alloc.h" header in sync with (unmaintained) upstream.
Also, our own "nm_auto_close" macro should be preferred, because
- it preserves errno
- it uses nm_close(), which asserts against EBADF.
We used to build with -std=gnu89 so commit 1391bdfa61
added a local patch to systemd code to avoid compilation error due to
missing __STDC_VERSION__ define.
In the meantime, since commit ba2b2de3ad
and commit b9bc20f4da, we also use -std=gnu99
and thus __STDC_VERSION__ is defined.
Revert our local modification.
With --enable-more-warnings, we already used -std=gnu99, see
commit ba2b2de3ad.
Compilation may behave differently depending on the selected
C standard that we choose. It seems wrong, with more-warnings,
to build against a C standard, while otherwise leaving it undefind.
Indeed, one might argue, that our build system should not use
such compiler specific options. At least, not without detecting
support for the compiler option during ./configure.
However:
- we already did this for --enable-more-warnings.
- we should not program against a theoretical compiler. In practice,
only gcc and clang works to build NetworkManager. Both these compilers
support this option, so there is no reason to not use it. If we ever
come into the situation to support another compiler, adjusting -std=gnu99
will be the smallest problem. Until that happens (and that's far from
imminent), don't pretend to be portable to non-existing compilers and
use the flag that in practice is available.
See-also: https://gcc.gnu.org/onlinedocs/gcc/Standards.html
When developing, we usually do in-tree-builds, so that case is
already better tested in every-day usage. It makes sense for
travis to test the less-well-tested case: the out-of-tree
build with autotools.
1) the command line gets shorter. I frequently run `make V=1` to see
the command line arguments for the compiler, and there is a lot
of noise.
2) define each of these variables at one place. This makes it easy
to verify that for all compilation units, a particular
define has the same value. Previously that was not obvious or
even not the case (see commit e5d1a71396
and commit d63cf1ef2f).
The point is to avoid redundancy.
3) not all compilation units need all defines. In fact, most modules
would only need a few of these defines. We aimed to pass the necessary
minium of defines to each compilation unit, but that was non-obvious
to get right and often we set a define that wasn't used. See for example
"src_settings_plugins_ibft_cppflags" which needlessly had "-DSYSCONFDIR".
This question is now entirely avoided by just defining all variables in
a header. We don't care to find the minimum, because every component
gets anyway all defines from the header.
4) this also avoids the situation, where a module that previously did
not use a particular define gets modified to require it. Previously,
that would have required to identify the missing define, and add
it to the CFLAGS of the complation unit. Since every compilation
now includes "config-extra.h", all defines are available everywhere.
5) the fact that each define is now available in all compilation units
could be perceived as a downside. But it isn't, because these defines
should have a unique name and one specific value. Defining the same
name with different values, or refer to the same value by different
names is a bug, not a desirable feature. Since these defines should
be unique accross the entire tree, there is no problem in providing
them to every compilation unit.
6) the reason why we generate "config-extra.h" this way, instead of using
AC_DEFINE() in configure.ac, is due to the particular handling of
autoconf for directory variables. See [1].
With meson, it would be trivial to put them into "config.h.meson".
While that is not easy with autoconf, the "config-extra.h" workaround
seems still preferable to me.
[1] https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Installation-Directory-Variables.html
Also, "src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-common.h"
already has a define IFCFG_DIR, but with a different value.
We shouldn't name different things the same.
