PrintDataCol contains a reference to the parent structure, for which
it was created. Previously, that was expressed via the "parent_idx"
field, which is an index into the list of all PrintDataCol entries.
That is inconvenient. Resolve the index to the actual pointer.
Note that during _output_selection_append() we still need to use
the index instead of resolving the pointer right away. That is because
_output_selection_append() grows the GArray into which the parent_idx
pointers to. So, obtaining the real pointer at that point would result
in using a dangling pointer later on.
Introduce a new step _output_selection_complete() which converts the
index into the actual pointer.
The print-output knows three modes: NORMAL, PRETTY, TERSE.
Never compare whether a mode is != a particular mode.
Instead, always explicitly check for the modes for which we enable a
particular behavior.
The effect is, that we always do something when the mode is set to a
particular value. We don't ever do something, if the mode is not set to
a particular value. Consistently opt-in is easier to understand.
The print-output knows three modes: NORMAL, PRETTY, TERSE.
Instead of using local variables "pretty" and "terse",
check for the output mode directly.
Note how we have tree modes, so mapping them to two boolean
variables is confusing. Especially at one place where we did:
pretty = (nmc_config->print_output != NMC_PRINT_TERSE);
while at other places:
pretty = (nmc_config->print_output == NMC_PRINT_PRETTY);
The header-cell is the header for a column with possibly many
rows.
We must not set:
header_cell->skip = nmc_config->overview && is_default;
for a particular cell, because it does not take into accound
the neighbouring rows. It is only correct, if there is only
one row.
Invert the logical meaning and rename "strip" to "to_print".
Each cell then can opt-in, whether it needs to be printed.
Only if no cell opts-in, it will be skipped.
Fixes: 9a19bbcb2f
Instead of re-fetching the UUID each time with a D-Bus call,
cache/memoize it.
On my system, this improves
$ make check-local-clients-tests-test-client
from 20.9 to 20.4 seconds (- 2.6%). That is not stellar, but noticible
enough to warrant this simple change.
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.
