If we call g_cancellable_connect() on a GCancellable that is already
cancelled, then the callback is invoked synchronously. We need to
handle that.
However, we can slightly simplify the code. There is no change in
behavior, but we can always let the cancelled callback return the
result.
"iface_data->cancellable" is an internal cancellable for the parallel
HTTP requests. Once we encounter a failure, those requests are all
obsolete and must be cancelled.
There are two GCancellable at work: one is provided by the user
during nmcs_provider_get_config(), and one is used internally for the
individual HTTP GET requests.
In _get_config_fetch_done_cb(), if the error reason is "cancelled",
then it means that our internal iface_data->cancellable was cancelled.
Probably because an error happend (like a timeout or the user cancelled
the external GCancellable).
In that case, we must not report that the task completed with a
cancellation, because we need to preserve the error that was the
original cause.
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
nm_utils_hexstr2bin_full() is our general hexstr to binary parsing
method. It uses (either mandatory or optional) delimiters. Before,
if delimiters are in use, it would accept individual hexdigits.
E.g. "a:b" would be accepted as "0a:0b:.
Add an argument that prevents accepting such single digits.
Seems with LTO the compiler can sometimes think that thes variables are
uninitialized. Usually those code paths are only after an assertion was
hit (g_return*()), but we still need to workaround the warning.
Make the error handling similar to the other provider implementations.
- only actually return once all callbacks completed.
- cache the first error and report it.
- drop AzureData.success field. It is redundant to have AzureData.error set.
Also it was actually unused.
- ensure that we keep the first error passed during
_get_config_maybe_task_return(). Once we set an error, that error gets
returned. There is a twist here, that we prefer cancellation errors
over other error reasons.
- drop GCPData.success field. It is redundant to have GCPData.error set.
Also, it's meaningless to indicate failure, if we don't have an error
at hand.
- ensure that we keep the first error passed during
_get_config_maybe_task_return(). Once we set an error, that error gets
returned. There is a twist here, that we prefer cancellation errors
over other error reasons.
- in _get_config_fip_cb(), ensure to call _get_config_maybe_task_return()
even if we are not yet ready. That is useful to record a potential
error.
If the list of addresses, routes and rules is empty, we still want to mangle
the applied connection, to also have an empty list.
nm-cloud-setup has certain expectations. For example, that the static addresses,
routes and rules of the active connection is entirely under the control of the
tool. For example, so it usually replaces the lists entirely. It also should do
that, if the new list is empty.
Maybe, one day there could be more complex merging strategies, where the user could
also add static addresses, routes, or rules to the profile, and nm-cloud-setup
would preserve them. However, that is not implemented, nor is it clear how exactly
that would work.
nm_http_client_poll_get_finish() can only either succeed (returning TRUE
and setting no GError), or failing (returning FALSE and setting GError).
Checking for both is redundant and unnecessary.
Since commit 3bd30f6064 ('nmcs: add error message when a HTTP request times
out'), the case where polling returns %FALSE without an error is no
longer possible. This is preferable, because it follows a consistent
API where a function clearly fails or succeeds.
So, checking for the error code and the returned boolean is redundant and
unnecessary.
nm_utils_parse_next_line() operates on the response buffer obtained
from NMHttpClient. We own this buffer, and we also can rely on the fact
that the buffer has a trailing NUL byte after the data.
There is no need to copy the string to a GString, just use it directly.
nm_utils_parse_next_line() operates on the response buffer obtained
from NMHttpClient. We own this buffer, and we also can rely on the fact
that the buffer has a trailing NUL byte after the data.
There is no need to clone the string to a GString, just use it directly.
The behavior is documented at various places, so this assert is less
to actually assert it, but as making this condition obvious to the
reader of the code.
NMHttpClient guarantees that the returned response is %NUL terminated after the
returned length of the buffer. That guarantee is important and should be documented.
It's not a severe issue, because the GetConfigMetadataData struct is
larger than GetConfigMetadataMac.
Fixes: 69f048bf0c ('cloud-setup: add tool for automatic IP configuration in cloud')
This add a provider implementation for GCP that when detected fetches
the ip addresses of configured internal load balancers.
Once this information is fetched from the metadata server it instructs
NetworkManager to add local routes for each found forwarded-ip.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
libcurl's documentation for CURLMOPT_TIMERFUNCTION requires the
application to install a non-repeating timer.
https://curl.haxx.se/libcurl/c/CURLMOPT_TIMERFUNCTION.html
So let's remove the GSource once expired.
Fixes: 69f048bf0c ('cloud-setup: add tool for automatic IP configuration in cloud')
Since just a single pointer is used to store the socket's GSource
if more than 1 consecutive request was done through the same
HTTP provider the 2nd request would clear the GSource associated to
the second request causing the 1st HTTP request to never complete
and end up in a expired timeout.
Use a hashtable instead so we can correctly track all requests.
https://bugzilla.redhat.com/show_bug.cgi?id=1821787
Fixes: 69f048bf0c ('cloud-setup: add tool for automatic IP configuration in cloud')
Most callers would pass FALSE to nm_utils_error_is_cancelled(). That's
not very useful. Split the two functions and have nm_utils_error_is_cancelled()
and nm_utils_error_is_cancelled_is_disposing().
curl_multi_setopt() accepts CURLMOPT_* options, not CURLOPT_*
ones. Found by GCC 10:
clients/cloud-setup/nm-http-client.c:700:38: error: implicit conversion from ‘enum <anonymous>’ to ‘CURLMoption’ [-Werror=enum-conversion]
700 | curl_multi_setopt (priv->mhandle, CURLOPT_VERBOSE, 1);
Fixes: 69f048bf0c ('cloud-setup: add tool for automatic IP configuration in cloud')
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.
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
