These setters not only invoke a synchronous D-Bus call (ignoring the
return value). They also modify the content of the cache client-side,
bypassing the information that we receive via notifications from the
server.
Also, they don't emit property changed signals, but in any case they
are broken beyond repair.
Fully mark them as deprecated. Note that they were already marked as
_NM_DEPRECATED_SYNC_METHOD. However, that does not actually mark
the API as deprecated, because fully deprecating all synchronous
methods is premature at this point.
Our NMObject implementations should behave in a similar manner.
For example, string properties should be coerced the a consistent
manner.
Add functions _nml_coerce_property_*() for that. Of course, they
are trivial. Their value is not that they encapsulate some complex
implementation, but that they convey a general concept of how we
want to handle certain properties in NMClient's object cache.
Default values should preferably be zero and/or a value that indicates
that the property is unknown/unset.
In practice, this property is not unset because it's present
on the D-Bus API.
Yes, by default (server side) devices do autoconnect.
But that does not mean an NMObject, that has its GObject property
not set via D-Bus shall default to TRUE.
Default values preferably should be FALSE, because that is what we get
by default (memset(0)).
These types are all subclasses of NMObject. These instances are commonly
created by NMClient itself. It makes no sense that a user would
instantiate the type. Much less does it make sense to subclass them.
Hide the object and class structures from public API.
This is an API and ABI break, but of something that is very likely
unused.
This is mainly done to embed the private structure in the object itself.
This has benefits for performance and debugability. But most
importantly, we can obtain a static offset where to access the private data.
That means, we can use the information to access the data pointer
generically, as we will need later.
This is not done for the internal types NMManager, NMRemoteSettings,
and NMDnsManager. These types will be dropped later.
The majority of sources in "libnm/" are implementations of NMObject.
"nm-libnm-utils.h" will contain common definitions for handling such
objects. This means, most of the source files under libnm will require
this include. Include it by default.
This is not merely cosmetic. I will need the obj_properties
array to lookup GParamSpec by their PROP_* enum value. The
alternative would be lookup by name, which is more expensive.
Note that D-Bus is fundamentally asynchronous. Doing blocking calls
on top of D-Bus is odd, especially for libnm's NMClient. That is because
NMClient essentially is a client-side cache of the objects from the D-Bus
interface. This cache should be filled exclusively by (asynchronous) D-Bus
events (PropertiesChanged). So, making a blocking D-Bus call means to wait
for a response and return it, while queuing all messages that are received
in the meantime.
Basically there are three ways how a synchronous API on NMClient could behave:
1) the call just calls g_dbus_connection_call_sync(). This means
that libnm sends a D-Bus request via GDBusConnection, and blockingly
waits for the response. All D-Bus messages that get received in the
meantime are queued in the GMainContext that belongs to NMClient.
That means, none of these D-Bus events are processed until we
iterate the GMainContext after the call returns. The effect is,
that NMClient (and all cached objects in there) are unaffected by
the D-Bus request.
Most of the synchronous API calls in libnm are of this kind.
The problem is that the strict ordering of D-Bus events gets
violated.
For some API this is not an immediate problem. Take for example
nm_device_wifi_request_scan(). The call merely blockingly tells
NetworkManager to start scanning, but since NetworkManager's D-Bus
API does not directly expose any state that tells whether we are
currently scanning, this out of order processing of the D-Bus
request is a small issue.
The problem is more obvious for nm_client_networking_set_enabled().
After calling it, NM_CLIENT_NETWORKING_ENABLED is still unaffected
and unchanged, because the PropertiesChanged signal from D-Bus
is not yet processed.
This means, while you make such a blocking call, NMClient's state
does not change. But usually you perform the synchronous call
to change some state. In this form, the blocking call is not useful,
because NMClient only changes the state after iterating the GMainContext,
and not after the blocking call returns.
2) like 1), but after making the blocking g_dbus_connection_call_sync(),
update the NMClient cache artificially. This is what
nm_manager_check_connectivity() does, to "fix" bgo#784629.
This also has the problem of out-of-order events, but it kinda
solves the problem of not changing the state during the blocking
call. But it does so by hacking the state of the cache. I think
this is really wrong because the state should only be updated from
the ordered stream of D-Bus messages (PropertiesChanged signal and
similar). When libnm decides to modify the state, there may be already
D-Bus messages queued that affect this very state.
3) instead of calling g_dbus_connection_call_sync(), use the
asynchronous g_dbus_connection_call(). If we would use a sepaate
GMainContext for all D-Bus related calls, we could ensure that
while we block for the response, we iterate that internal main context.
This might be nice, because all events are processed in order and
after the blocking call returns, the NMClient state is up to date.
The are problems however: current blocking API does not do this,
so it's a significant change in behavior. Also, it might be
unexpected to the user that during the blocking call the entire
content of NMClient's cache might change and all pointers to the
cache might be invalidated. Also, of course NMClient would invoke
signals for all the changes that happen.
Another problem is that this would be more effort to implement
and it involves a small performance overhead for all D-Bus related
calls (because we have to serialize all events in an internal
GMainContext first and then invoke them on the caller's context).
Also, if the users wants this behavior, they could implement it themself
by running libnm in their own GMainContext. Note that libnm might
have bugs to make that really working, but that should be fixed
instead of adding such synchrnous API behavior.
Read also [1], for why blocking calls are wrong.
[1] https://smcv.pseudorandom.co.uk/2008/11/nonblocking/
So, all possible behaviors for synchronous API have severe behavioural
issues. Mark all this API as deprecated. Also, this serves the purpose of
identifying blocking D-Bus calls in libnm.
Note that "deprecated" here does not really mean that the API is going
to be removed. We don't break API. The user may:
- continue to use this API. It's deprecated, awkward and discouraged,
but if it works, by all means use it.
- use asynchronous API. That's the only sensible way to use D-Bus.
If libnm lacks a certain asynchronous counterpart, it should be
added.
- use GDBusConnection directly. There really isn't anything wrong
with D-Bus or GDBusConnection. This deprecated API is just a wrapper
around g_dbus_connection_call_sync(). You may call it directly
without feeling dirty.
---
The only other remainging API is the synchronous GInitable call for
NMClient. That is an entirely separate beast and not particularly
wrong (from an API point of view).
Note that synchronous API in NMSecretAgentOld, NMVpnPluginOld and
NMVpnServicePlugin as not deprecated here. These types are not part
of the D-Bus cache and while they have similar issues, it's less severe
because they have less state.
NM_DEVICE_MANAGED was intended to work like NM_DEVICE_AUTOCONNECT:
namely it would call the D-Bus property setter synchronously.
But such behavior is horrendous, we certainly don't want blocking calls
during a property getter.
Luckily this one instance was unused and never worked as the property
was marked as G_PARAM_READABLE. Just drop the setter.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
We built (among others) two libraries from the sources in "shared/nm-utils":
"libnm-utils-base.la" and "libnm-utils-udev.la".
It's confusing. Instead use directories so there is a direct
correspondence between these internal libraries and the source files.
(cherry picked from commit 2973d68253)
At a few places we checked whether neighbor->attrs was non-NULL.
That is not necessary, unless we'd like to catch some dangling/invalid
pointers. The attrs hash is always set otherwise.
Instead of just dropping the check, add a NM_IS_LLDP_NEIGHBOR() macro
(inline function).
For one, just reassigning copy->attrs leaks the previous
hash table. Fix that.
Also, NMLldpNeighbor instances are not immutable. I think that
is an uglyness, and it would be preferable that they can be sealed.
A sealed object could safely share/ref the internal hash-table. However,
as it is, we cannot just have two NMLldpNeighbor instances share the
same hash-table. Do a full copy.
Add the const qualifier to the attribute name in LLDP API functions so
that const strings and string literals are accepted. This change is
backwards compatible for existing users of the API.
nm_device_get_device_type would report the device type as it was send on
DBus, while fetching the property would mean that only a known device
types is reported.
Make both results consistent by coercing in nm_device_get_device_type
rather than when setting the property.
(cherry picked from commit a6a185ba00)
The device type was set to the GType rather than a new value in the
NMDeviceType enum.
Add the corresponding enum entry, fix the device type and set the
routing priority to the same value as generic devices.
(cherry picked from commit 8d9365a973)
The @connection argument can be NULL; add the (allow-none) annotation
otherwise calling the API with a NULL argument through GObject
introspection fails with:
Argument 1 does not allow None as a value
Fixes: 278fd4fb0f
Use the same form everywhere: "TRANSLATORS" instead of "Translators".
The manual also seems to prefer the upper-case form [1].
$ sed 's/\<Translators\>: /TRANSLATORS: /g' $(git grep -l Translators) -i
[1] https://www.gnu.org/software/gettext/manual/gettext.html
If an operation is cancelled through the GCancellable, then the idiom is
that the operation is always cancelled, even if it has finished
successfully. To ensure this is the case, add calls to
g_simple_async_result_set_check_cancellable everywhere.
Without this, e.g. gnome-control-center will crash when switching away
from the power panel quickly, as the NMClient creation finishes
asynchronously and g-c-c assume that G_IO_ERROR_CANCELLED is returned to
ensure it doesn't access the now invalid user_data parameter.
https://bugzilla.gnome.org/show_bug.cgi?id=794088
It's very likely that the product said something that was filtered out
by the fixup, such as "PCI Ethernet" or "Wi-Fi Adapter". Use a generic
type name in place of it.
The product names are generally of rather poor quality. The product name
is no place to enumerate product capabilities, the bus it's attached on
and similar nonsense.
The hwdb generally contains the strings of rather poor quality,
especially when it comes to sensibly presenting them to the user and
they need various cleanups.
While the following patches add fixups, this one splits out vendor
fixups, because it turns out that a different set of fixups is needed
than for products.
We also do this for libnm and libnm-core, where it causes visible changes
in behavior. But if somebody would rely on the hashing implementation
for hash tables, it would be seriously flawed.
The new device type represents a PPP interface, and will implement the
activation of new-style PPPoE connections, i.e. the ones that don't
claim the parent device.
