A MAC address is a relatively common "type". The GObject property is of type string,
but the D-Bus type is a bytestring ("ay"). We will need a special NMSettInfoPropertType.
Note that like most implementations, the from-dbus implementation still is based
on GObject setters. This will change in the future.
Also note that the previous compare function was
_nm_setting_property_compare_fcn_default(). That is, it used to convert
the property to GVariant and compare those. The conversion to GVariant
in that case normalizes the string (e.g. it is case insensitive). Also,
only properties could be compared which were also convertible to D-Bus
(which is probably fine, because there is no guarantee the profiles that
don't verify can be compared).
The code now uses the direct comparison of the strings. That mostly
preserves the case-insensitivity of the previous comparison, because
the property setters for mac addresses all use
_nm_utils_hwaddr_canonical_or_invalid() to normalize the strings.
This is subtle, but still correct. Note that this will improve later,
by ensuring that the property setters for mac addresses automatically
perform the right normalization.
The aim is that properties have a "type", that is, that similar
properties share a common behavior and appearance.
Most properties of type "mac-address" normalize the string in the
GObject property setter. Three don't. Let them also do that.
This is also relevant, because the compare function for mac-addresses
(_nm_setting_property_compare_fcn_default()) converts the properties
first to a "ay" GVariant. Which means the comparison is case
insensitive. Normalizing the values in the setter avoids that
inconsistency.
This mode was added to network-legacy in [1]. NetworkManager anyway always
does DHCP in parallel, so this is basically an alias for "dhcp".
Note that network-legacy's "single-dhcp" will stop waiting for DHCP
once the first device gets an address. NetworkManager currently cannot
do that. While it runs DHCP in parallel, all devices need to settle
and there is no concept where completing one device makes the overall
"startup complete" process finish early. That could however be added.
Anyway, while not being exactly the same, it's still more useful to do
something similar instead of not working at all.
See-also: https://github.com/dracutdevs/dracut/pull/853
See-also: https://github.com/dracutdevs/dracut/pull/961
See-also: https://github.com/dracutdevs/dracut/pull/1048
[1] 4026cd3b01
Due to something that really should be fixed, NetworkManager merges the routes
that it wants to configure, with the routes that are configured externally.
This includes a subtract and merge dance, which is wrong.
Anyway. If we are in nm_platform_ip_route_sync(), then we never want to
actively configure a route, that we only have in the list because it is
(or was) present on the interface.
Otherwise we have a problem. Note that we make a plan which
routes/addresses to add/remove before starting. So, if we start with an
IPv4 address configured in kernel, then there is also a corresponding
local route. We would track that local route as external.
During sync, we first remove the IP address, and kernel automatically
also removes the local route. However, as we already made the plan to
keep that route, NetworkManager would wrongly configure it again.
This should fix that bug. It is anyway wrong to even try to explicitly
configure a route, that is purely in the list as being external.
https://bugzilla.redhat.com/show_bug.cgi?id=1979192#c11
- add "required:false" to dependency() and find_library(),
otherwise autodetection will fail.
- rename variable "enable_readline" to "with_readline" for
consistency with autotools.
- with -Dlibreadline=auto, once we detect a library, update
"with_readline" variable to reflect the detected choice.
This will also be printed in the summary output.
This is also important for the nmcli check
`assert(with_readline != 'none', 'readline ...`.
- `m4/ax_lib_readline.m4` was already aware of "$with_readline".
Move the entire handling of the parameter inside the AX_LIB_READLINE
macro.
This lets our fork of ax_lib_readline.m4 further deviate from upstream
version, but it's already so different that this is no new problem.
- raise an error if the user requested --with-readline=libreadline|libedit
but the library was not found.
- only allow yes|no for --with-nmcli argument. But still default to
"yes", which will always require one libreadline library to be
detected. In particular, don't automatically disable nmcli if
libreadline is not available, because building without nmcli
should be an explicit choice. That is like before.
- update the "$with_readline" variable for the "auto" case to reflect
what was detected.
On architectures where "char" is signed, the check "ch < ' '" is also
TRUE for characters greater than 127 (that is, UTF-8 characters).
Let's preserve valid UTF-8 characters and don't clear them.
Also note that already before we filtered out invalid UTF-8 sequences,
so if we encounter here a character > 127, it is part of a valid UTF-8
sequence.
Previously, once in_parent was TRUE it was never reset, thus the
remainder of the string was cleared. That was most likely not intended.
If the intent really was to clear all the remainder, then the code could
have simply truncated the string at the first '('.
This is like using nm_ascii_is_ctrl_or_del() instead of
nm_ascii_is_ctrl() in the previous version of the patch.
We thus now always will switch to ANSIC escaping if we see
a ASCII DEL character. That is probable desirable, but either
way should not make a big difference (because we can parse
the DEL character both in regular quotation and in ANSIC quotation).
The patch is however larger, to also take the opportunity to only check
for nm_ascii_is_regular() in the "fast path". The behavior is the same
as changing nm_ascii_is_ctrl() to nm_ascii_is_ctrl_or_del().
On architectures where "char" is signed, the check "ch < ' '" is
also TRUE for non-ASCII characters greater than 127. This is an
easy mistake to make. Fix it by using nm_ascii_is_control() which
gets this right.
It's a bug, but possibly not too bad because unnecesarily escaping
a UTF-8 characters is not a severe problem, because the user anyway must
be prepared to unescape the string.
These functions have overlap with g_ascii_is*() functions.
However g_ascii_is*() (and the is* functions from <ctype.h>) are
always confusing to me, in the sense that it's not clearly stated
which characters qualify for a certain category. And review is not
easy either, because they are implemented via a table lookup.
E.g. were you aware that 127 is considered g_ascii_iscntrl()? Probably
you were, but it's not clear to see that anywhere.
The main point of our own functions is to have is easier to see how
characters get categorized, by using comparison instead of table lookup.
Also, several existing code did in fact not use the g_ascii_is*()
macros, possibly because of the (perceived) difficulty to understand
their exact meaning. As a consequence, several checks got wrong.
For example, (ch < ' ') is not a valid check for testing whether
the character is a ASCII control character, for two reasons:
- if char is a signed type (as likely it is), then this also evaluates
to TRUE for all non-ASCII, UTF-8 characters that are greater than
127.
- it does not consider DEL character (127) a control character.
When looking at a property, it should always be clear how it is handled.
Also the "default" action should be an explicit hook.
Add _nm_setting_property_from_dbus_fcn_gprop() and set that as
from_dbus_fcn() callback to handle the "default" case which us
build around g_object_set_property().
While this adds lines of code, I think it makes the code easier to
understand. Basically, to convert a GVariant to a property, now all
properties call their from_dbus_fcn() handler, there is no special casing.
And the gprop-hook is only called for properties that are using
_nm_setting_property_from_dbus_fcn_gprop(). So, you can reason about
these two functions at separate layers.
NM_SETTING_NAME is also a GObject property, but it's
not supposed to be serialized to/from D-Bus. It also
is irrelevant for comparison.
Hence, it's operations are all NOPs. Make an explicit property type for
that case instead of checking the GParamSpec flags.
The "to_dbus_data" existed for namespacing the properties inside it.
However, such a struct adds overhead due to the alignment that it
enforces. We can share the memory needed for the bitfield by having
them beside each other.
The goal is to get rid of gprop_to_dbus_fcn() uses.
Note that there is a change in behavior. The "dns" GPtrArray in
NMSettingIPConfig is never NULL (the default of the boxed property),
thus the previous code always serialized the property, even the
empty list.
Now, empty dns properties are omitted from D-Bus.
Also, there is another change in behavior: nm_utils_ip4_dns_to_variant()
will now skip over strings that are not valid IPv4 addresses.
Previously, it would have added 0.0.0.0 (or some undefined address).
The goal is to get rid of gprop_to_dbus_fcn() uses.
Note that there is a change in behavior. The "dns" GPtrArray in
NMSettingIPConfig is never NULL (the default of the boxed property),
thus the previous code always serialized the property, even the
empty list.
Now, empty dns properties are omitted from D-Bus.
All settings have a "name" property. Their compare_fcn() is not interesting
and was already previously ignored. But we should not special handle it via
_nm_setting_property_compare_fcn_default().
