Certain parts of the code are entirely generated or must follow
a certain format that can be enforced by a tool. These invariants
must never fail:
- ci-fairy generate-template (check-ci-script)
- black python formatting
- clang-format C formatting
- msgfmt -vs
On the other hand, we also have a checkpatch script that checks
the current patch for common errors. These are heuristics and
only depend on the current patch (contrary to the previous type
that depend on the entire source tree).
Refactor the gitlab-ci tests:
- split "checkpatch" into "check-patch" and "check-tree".
- merge the "check-ci-script" test into "check-tree".
Now that the individual steps are no longer in .gitlab.yml but we
run a full shell script, clean it up to be better readable.
Also, we need to fail the script when any command fails.
"debian/REQUIRED_PACKAGES" is used by gitlab-ci to prepare the image. We require
"udev" package, if only to install "/usr/share/pkgconfig/udev.pc" to get the
udev directory.
Otherwise build fails with:
Run-time dependency udev found: NO (tried pkgconfig)
meson.build:371:2: ERROR: Dependency "udev" not found, tried pkgconfig
Before, ovsdb_call_method() has a long list of arguments
to account for all possible commands. That does not scale.
Instead, introduce a separate OvsdbMethodPayload type and
only add a macro to allow passing the right parameters.
The text should match the OvsdbCommand enum. If the enum
value is named OVSDB_ADD_INTERFACE, then we should print
"add-interface". Or alternatively, if you think spelling
out interface is too long, then the enum should be renamed.
I don't care, but name should correspond.
ovsdb sends monitor updates, with "new" and "old" values that indicate
whether this is an addition, and update, or a removal.
Since we also cache the entries, we might not agree with what ovsdb
says. E.g. if ovsdb says this is an update, but we didn't have the
interface in our cache, we should rather pretend that the interface
was added. Even if this possibly indicates some inconsistency between
what OVS says and what we have cached, we should make the best of it.
Rework the code. On update, we compare the result with our cache
and care less about the "new" / "old" values.
We will call the function directly as well. Lets aim to
get the types right.
Also the compiler would warn if the cast to (GDestroyNotify)
would be to a fundamtally different function signature.
GHashTable is optimized for data that has no separate value
pointer. We can use the OpenvswitchBridge structs as key themselves,
by having the id as first field of the structure and only use
g_hash_table_add().
Of course, in practice "mtu" is much smaller than 2^31, and
also is sizeof(int) >= sizeof(uint32_t) (on our systems). Hence,
this was correct. Still, it feels ugly to pass a unsigned integer
where not the entire range is covered.
- rename "id" to something more distinct: "call_id".
- consistently use guint64 type. We don't want nor need
to handle negative values. For CALL_ID_UNSPEC we can use
G_MAXUINT64.
- don't use "i" format string for the call id. That expects
an "int", so it's not clear how this was working correctly
previously. Also, "int" has a smaller range than our 64bits.
Use instead "json_int_t" and cast properly in the variadic
arguments of json_pack().
Also, never update the value to %NULL. If the current
message does not contain a UUID, keep the previous one.
Fixes: 830a5a14cb ('device: add support for OpenVSwitch devices')
"nm-device-logging.h" defines logging macros for a NMDevice instance.
It also expects a "self" variable in the call environment, and that
variable had to be in the type of NMDevice or the NMDevice subclass.
Extend the macro foo, so that @self can be either a NMDevice* pointer
or a NMDevice$SUBTYPE.
Of course, that would have always been possible, if we would simply cast
to "(NMDevice *)" where we need it. The trick is that the macro only
works if @self is one of the two expected types, and not some arbitrary
unrelated type.
And example script for getting and setting OVS external-ids.
Since currently there is no nmcli support for these properties yet,
the script becomes more interesting.
This "example" is rather long, and it showcases less the usage of
libnm (which is rather trivial, with respect to configuring
NMSettingOvsExternalIDs). Instead, it aims to provide a useful
command line tool for debugging. Hence, it's mostly concerned with
an elaborate command line syntax and useful print output.
libnm supports verbose debug logging by setting "LIBNM_CLIENT_DEBUG"
environment variable. That mechanism uses g_printerr() (or g_print()).
When testing an application it's useful to combine printf debugging
with this debug logging. However, python's print() statement is
additionally buffered and not in sync with the logging functions that
libnm uses.
As far as I see, g_print() and g_printerr() is not accessible via
introspections/pygobject, probably because these are variadic functions.
Add nm_utils_print() to libnm. This ensures to use the same logging
mechanism as libnm.
We have:
- nm_utils_hashtable_cmp(): this does a full cmp of two hash
tables, with the intent to provide a stable sort order.
It thus takes a GCompareDataFunc() argument.
- nm_utils_hashtable_cmp_equal(): this is like nm_utils_hashtable_cmp(),
except that the caller won't get a compare value, only a boolean
value that indicates equality.
This was previously called nm_utils_hashtable_equal().
- nm_utils_hashtable_equal(): this takes a GEqualFunc function
for comparing the values for equality. It takes thus
a different kind of predicate, but otherwise is similar to
nm_utils_hashtable_cmp_equal().
This was previously called nm_utils_hash_table_equal().
Unify the naming of these functions.
These packages are also used during CI tests (in the checkpatch
stage). They are also used for formatting C/python code, and thus
useful for developing.
Install them as part of REQUIRED_PACKAGES script.
On one image we do extra work, like generating documentation (gitlab-pages).
The same image is currently also used for the "checkpatch" step. That step
checks code formatting using clang-format. The formatting depends on the
clang version, and we currently choose Fedora 33 as the desired version
for formatting.
It means, the "checkpatch" step requires Fedora 33. We could choose a
different image for generating pages and run check patch. However, that
might not be best. Just also generate the pages using Fedora 33.
All the steps of "checkpatch" test (except the last) check
the current tree for consistency. Those checks must always
pass.
Only the last step calls the "checkpatch-feature-branch.sh".
That script checks for common patterns, like avoiding g_assert()
(in favor of other assertion types). That last check only checks
the current patch, and there are many cases where the test is
known to fail (because these are just heuristics). As such, the
step that may fail should be called as last.
