The signal is unused (and should be removed).
Still, the parameter passed to g_signal_emit() is a C string, not a
GVariant. I think as there are no subscribers, glib wouldn't actually
do anything with the arguments. Though, I am not sure whether glib still
tries to initialize a GValue with a GVariant type, leading to a crash.
Fixes: f05b7a78c9 ('supplicant: Track P2P Group information, creation and destruction')
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
This should give the compiler more possibilities to warn about wrong
use of the API.
In practice, my current compiler wouldn't flag any issues. However,
some compilers (or compile options) might.
and _nm_utils_inet6_ntop() instead of nm_utils_inet6_ntop().
nm_utils_inet4_ntop()/nm_utils_inet6_ntop() are public API of libnm.
For one, that means they are only available in code that links with
libnm/libnm-core. But such basic helpers should be available everywhere.
Also, they accept NULL as destination buffers. We keep that behavior
for potential libnm users, but internally we never want to use the
static buffers. This patch needs to take care that there are no callers
of _nm_utils_inet[46]_ntop() that pass NULL buffers.
Also, _nm_utils_inet[46]_ntop() are inline functions and the compiler
can get rid of them.
We should consistently use the same variant of the helper. The only
downside is that the "good" name is already taken. The leading
underscore is rather ugly and inconsistent.
Also, with our internal variants we can use "static array indices in
function parameter declarations" next. Thereby the compiler helps
to ensure that the provided buffers are of the right size.
Note that the name "NMSecretAgentOld" comes from when libnm was forked
from libnm-glib. There was a plan to rework the secret agent API and
replace it by a better one. That didn't happen (yet), instead our one
and only agent implementation is still lacking. Don't add a new API, instead
try to improve the existing one, without breaking existing users. Just
get over the fact that the name "NMSecretAgentOld" is ugly.
Also note how nm-applet uses NMSecretAgentOld. It subtypes a class
AppletAgent. The constructor applet_agent_new() is calling the synchronous
g_initable_init() initialization with auto-register enabled. As it was,
g_initable_init() would call nm_secret_agent_old_register(), and if the
"Register" call failed, initialization failed for good. There are even
unit tests that test this behavior. This is bad behavior. It means, when
you start nm-applet without NetworkManager running, it will fail to create
the AppletAgent instance. It would hence be the responsibility of the applet
to recover from this situation (e.g. by retrying after timeout or watching
the D-Bus name owner). Of course, nm-applet doesn't do that and won't recover
from such a failure.
NMSecretAgentOld must try hard not to fail and recover automatically. The
user of the API is not interested in implementing the registration,
unregistration and retry handling. Instead, it should just work best
effort and transparently to the user of the API.
Differences:
- no longer use gdbus-codegen generate bindings. Use GDBusConnection
directly instead. These generated proxies complicate the code by
introducing an additional, stateful layer.
- properly handle GMainContext and synchronous initialization by using an
internal GMainContext.
With this NMSecretAgentOld can be used in a multi threaded context
with separate GMainContext. This does not mean that the object
itself became thread safe, but that the GMainContext gives the means
to coordinate multi-threaded access.
- there are no more blocking calls except g_initiable_init() which
iterates an internal GMainContext until initialization completes.
- obtaining the Unix user ID with "GetConnectionUnixUser" to authenticate
the server is now done asynchronously and only once per name-owner.
- NMSecretAgentOld will now register/export the Agent D-Bus object
already during initialization and stay registered as long as the
instance is alive. This is because usually registering a D-Bus
object would not fail, unless the D-Bus path is already taken.
Such an error would mean that another agent is registered for the same
GDBusConnection, that likely would be a bug in the caller. Hence,
such an issue is truly non-recoverable and should be reported early to
the user. There is a change in behavior compared to before, where previously
the D-Bus object would only be registered while the instance is enabled.
This makes a difference if the user intended to keep the NMSecretAgentOld
instance around in an unregistered state.
Note that nm_secret_agent_old_destroy() was added to really unregister
the D-Bus object. A destroyed instance can no longer be registered.
- the API no longer fully exposes the current registration state. The
user either enables or disables the agent. Then, in the background
NMSecretAgentOld will register, and serve requests as they come. It
will also always automatically re-register and it can de-facto no
longer fail. That is, there might be a failure to register, or the
NetworkManager peer might not be authenticated (non-root) or there
might be some other error, or NetworkManager might not be running.
But such errors are not exposed to the user. The instance is just not
able to provide the secrets in those cases, but it may recover if the
problem can be resolved.
- In particular, it makes no sense that nm_secret_agent_old_register*()
fails, returns an error, or waits until registration is complete. This
API is now only to enable/disable the agent. It is idempotent and
won't fail (there is a catch, see next point).
In particular, nm_secret_agent_old_unregister*() cannot fail anymore.
- However, with the previous point there is a problem/race. When you create
a NMSecretAgentOld instance and immediately afterwards activate a
profile, then you want to be sure that the registration is complete
first. Otherwise, NetworkManager might fail the activation because
no secret agent registered yet. A partial solution for this is
that g_initiable_init()/g_async_initable_init_async() will block
until registration is complete (or with or without success). That means,
if NetworkManager is running, initializing the NMSecretAgentOld will
wait until registration is complete (or failed). However, that does not
solve the race if NetworkManager was not running when creating the
instance.
To solve that race, the user may call nm_secret_agent_old_register_async()
and wait for the command to finish before starting activating. While
async registration no longer fails (in the sense of leaving the agent
permanently disconnected), it will try to ensure that we are
successfully registered and ready to serve requests. By using this
API correctly, a race can be avoided and the user can know that the
instance is now ready to serve request.
The NMSecretAgentOld is build very much around a GDBusConnection, and GDBusConnection
is build around GMainContext. That means, a NMSecretAgentOld instance is
strongly related to these two. That is because NMSecretAgentOld will register
to signals on D-Bus, using GDBusConnection. Hence, that GDBusConnection instance
and the calling GMainContext becomes central to the NMSecretAgentOld instance.
Also, the GMainContext is the way to synchronize access to the
NMSecretAgentOld. Used properly, this allows using the API in multi
threaded context.
Expose these two in the public API. Since NMSecretAgentOld is part of
libnm and supposed to provide a flexible API, this is just useful to
have.
Also, allow to provide a GDBusConnection as construct-only property. This way,
the instance can be used independent of g_bus_get() and the user has full control.
There is no setter for the GMainContext, because it just takes the
g_main_context_get_thread_default() instance at the time of
construction.
This will also be useful for NMSecretAgentOld.
The mechanics how NMClient handles the GMainContext and the
context-busy-watcher apply really to every GObject that uses
GDBusConnection and registers to signals.
At least, as long as the API provides no shutdown/stop method,
because that means shutdown/stop happens when unreferencing the
instance, at which point pending operations get cancelled (but
they cannot complete right away due to the nature of GTask and
g_dbus_connection_call()). If there is a shutdown/stop API, then all
pending operations could keep the instance alive, and the instance
would sticks around (and keeps the GMainContext busy) until shutdown is
completed. Basically, then the instance could be the context-busy-watcher
itself.
But in existing API which does not require the user to explicitly shutdown,
that is not a feasible (backward compatible) addition. But the context-busy-watcher
object is.
The test only uses one GMainContext (the g_main_context_get_default()
singleton.
Between tests, ensure that we iterate the main context long enough,
so that no more sources from the previous test are queued. Otherwise,
there is an ugly dependency between tests and the order in which
they run.
Use nmtstc_context_object_new() to create the NMSecretAgentOld. This
randomly uses sync or async initialization, and checks whether the
main context gets iterated.
nmtstc_client_new() exists to test creating a GInitiable/GAsyncInitiable
in different GMainContext combinations.
This is not only useful for NMClient but will also be useful for
NMSecretAgentOld. Add nmtstc_context_object_new() to allow for that.
Also, allow passing parameters when creating the object.
The resulting nmtstc_context_object_new() is relatively complex. But
this is only testing code, that aims to construct the respective GObject
instance in various manners (randomly using the sync or async initialization).
It is complex, but delivers at testing various code paths of the
underlying code. The API that it provides however is simple.
Also drop _nmtstc_client_new_extra_context() to create the instance with
a different context. For one, this requires that the internal context is
integrated as long as the context-busy-watcher exists. That was not
handled correctly. Also, creating a NMClient instance with a different
context than the current thread default at construct time has
implications to the test later. The tests don't want this variant, and
don't handle them properly. So drop this.
nmtst_main_context_iterate_until*() iterates until the condition is
satisfied. If that doesn't happen within timeout, it fails an assertion.
Rename the function to make that clearer.
The device instance might already be removed from the cache. At that
point, _nm_object_get_client(self) returns %NULL.
Use the correct NMClient instance.
For printf debugging (when you recompile the source) it can be useful
to have one switch to disable logging of NMClient.
For example, this is useful with
$ LIBNM_CLIENT_DEBUG=trace nmcli agent secret
I think it's technically not correct to rely on the "sentinal" field
being immediately after the previous field, due to alignment. Implement
the macro differently.
