When the errno was accidentally set to EAGAIN or EWOULDBLOCK,
we would only read one single message and return that there is
nothing to read.
This means, if there were more then one messages ready to read,
we would only read the first one and return to the main-loop
(which then again calls back to platform as more data is ready
to be read).
The explainations no longer hold for the most part. Documentation
that is not directly the code is bound the expire.
The code is the best documentation!! :)
With g_return() we try to catch wrong invocations of a function
or passing invalid arguments.
Having ~forgot~ to overwrite a virtual function is such a serious
and fundamental issue, that checking for it at runtime is wasteful
and unnessesary.
If we ever hit such a situation, just crash.
Link related functions should have a "nm_platform_link" prefix. Rename.
Naming is a subjective matter and one might argue that omitting
the "link" part from the name is shorter and even preferred.
However, I think functions related to links should have a common
prefix as the underlyings are strongly related.
Let the link-add functions return the internal pointer to the platform
link object. Similar to link-get, which doesn't copy the link either.
Also adjust the sole users of the add-functions (create-and-realize)
to take the pointer.
Eventually we still copy the returned data, because accessing platform can
invalidate the returned pointer. Thus we don't actually safe any copying
by this (at least every use of the function currently leads to the data
being copied).
Still change it, because I think the API of NMPlatform should look like that.
NMPlatformLink is a plain struct (not a GObject, for which we usually
don't use const). We certainly don't want the functions to modify the
passed-in data.
There are the link-types NONE and UNKNOWN. NONE is a linktype that is never
returned by platform, but UNKNOWN is very much a valid (albeit unspecified)
type.
Effectively, create_and_realized() should create a link of a known type,
thus it should never return an UNKNOWN link type at this point. Still
change it because it feels more correct.
When activating a device, we must progress the device state to
disconnected state.
This matters when activating a device without carrier. In this
case we would have skipped DISCONNECTED state. Skipping the
device state then leads to other issues like a slave device
never noticing that the master got ready.
Device subclasses (for example NMDeviceWifi) can use the ifindex in
their constructor(), but the value now is set later in
parent class constructed(). This causes the following:
nm_platform_wifi_get_capabilities: assertion 'ifindex > 0' failed
Fix this by initializing ifindex earlier in NMDevice's
constructor(). While at it, remove the
nm_assert (pllink->type != NM_LINK_TYPE_NONE);
assertion, since pllink can be NULL there.
Fixes: 6db04dc206
This is to make it possible for the device factories to indicate the desired
link type and make it possible to avoid matching the unrealized device to a
platform device of different link type.
- cleanup _NMLOG()
- implement state_to_string() based on NM_UTILS_STRING_LOOKUP_TABLE(),
which prints unknown values as numeric
- add logging when setting device and state
- cleanup logging in check-master-ready to consistently
print relevant information
- update logging in set_master() to match simpler logging
format like set_device() and set_state().
We often lookup the private data and retrieve it via NM_DEVICE_GET_PRIVATE(),
which in turn calls G_TYPE_INSTANCE_GET_PRIVATE().
Instead cache the pointer to the private data.
There are up- and downsides:
- requries additional sizeof(gpointer) bytes for each NMDevice.
+ retrieving the private pointer will be slightly faster.
+ easier debugging in gdb as it is currently often a pain to
retrieve the private data.
But most importantly, the allows to change our common pattern
to first cache the private data in a variable @priv. That is
often cumbersome to write, especially for short functions.
This change gives us a choice to use self->priv directly.
Such a change should not be aimed for every class. Instead it makes
mostly sense for NMDevice, where it pays off better due to the
class' size and ubiquitous use.
https://mail.gnome.org/archives/networkmanager-list/2015-December/msg00017.html
Showcase for the new macros NM_UTILS_STRING_LOOKUP_TABLE() and
NM_UTILS_STRING_LOOKUP_TABLE_DEFINE_STATIC().
It changes behavior in case of looking up an invalid/unknown
state reason. Previously it would just have returned "unknown"
-- which was indistinguishable from a regular "unknown" value.
Now it returns the numeric id as a string. The string is allocated
with alloca(), which is desired but one should be aware of the pitfalls:
- prevents the caller from being inlined
- bad idea to do in a loop.
Before the device is setup, we call nm_platform_tun_get_properties() without
a valid ifindex. That triggered an assertion [1].
Thereby, change nm_platform_tun_get_properties() to effectively clear
the tun properties when we are unable to fetch them. Also, never modify
the tun-mode of NMDeviceTun.
[1]
#0 0x00007f0a4173e81b in g_logv (breakpoint=1) at gmessages.c:324
#1 0x00007f0a4173e81b in g_logv (log_domain=0x561e9264ccf6 "NetworkManager", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7ffd71a0d4d0) at gmessages.c:1081
#2 0x00007f0a4173e98f in g_log (log_domain=<optimized out>, log_level=<optimized out>, format=<optimized out>) at gmessages.c:1119
#3 0x0000561e9241ecec in nm_platform_tun_get_properties (self=0x561e9354ba70 [NMLinuxPlatform], ifindex=0, props=0x7ffd71a0d650) at platform/nm-platform.c:2081
#4 0x0000561e923aad9c in reload_tun_properties (self=0x561e937fb080 [NMDeviceTun]) at devices/nm-device-tun.c:68
#5 0x0000561e923aa795 in realize (device=0x561e937fb080 [NMDeviceTun], plink=0x7ffd71a0d818, error=0x7ffd71a0d798) at devices/nm-device-tun.c:225
#6 0x0000561e923bdc06 in nm_device_realize (self=0x561e937fb080 [NMDeviceTun], plink=0x7ffd71a0d818, out_compatible=0x7ffd71a0d77c, error=0x7ffd71a0d798) at devices/nm-device.c:1713
#7 0x0000561e924ad995 in platform_link_added (self=0x561e9356e230 [NMManager], ifindex=33, plink=0x7ffd71a0d818) at nm-manager.c:1947
#8 0x0000561e924ad717 in _platform_link_cb_idle (data=0x561e937eb940) at nm-manager.c:2029
#9 0x00007f0a41737e3a in g_main_context_dispatch (context=0x561e93547530) at gmain.c:3154
#10 0x00007f0a41737e3a in g_main_context_dispatch (context=context@entry=0x561e93547530) at gmain.c:3769
#11 0x00007f0a417381d0 in g_main_context_iterate (context=0x561e93547530, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3840
#12 0x00007f0a417384f2 in g_main_loop_run (loop=0x561e935475f0) at gmain.c:4034
#13 0x0000561e923ba3f3 in main (argc=1, argv=0x7ffd71a0dc68) at main.c:488
Fixes: 4dbaac4ba2
I found the handling of the master-device very confusing because it was
unclear who sets priv->master, and when it should be set.
Now:
- Setting priv->master (in a slave) always goes together with adding
the master to priv->slaves (in the master). Previously, this was
done at separate places, so it was not clear if master and slave
always agree on their relationship -- in fact, they did not.
- There are now three basic functions which do the enslaving/releasing:
(1) nm_device_master_add_slave()
(2) nm_device_master_enslave_slave()
(3) nm_device_master_release_one_slave()
Step 3/release basically undoes the 1/add and 2/enslave steps.
- completing the enslaving/releasing is now done by
(1) nm_device_slave_notify_enslave()
(2) nm_device_slave_notify_release()
These functions also emit signals like NM_DEVICE_MASTER.
- Derived classes no longer emit NM_DEVICE_SLAVES notification. Instead
the notification is emited together with NM_DEVICE_MASTER, whenever a
slaves changes state. Also, NM_DEVICE_SLAVES list now only exposes
slaves that are actually @is_enslaved.
