-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
API: implement APIs from the new specification #298
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you very much for this big refactoring!
I really like your re-structuring of the public header files. It seems to be API backwards compatible, too. (But we should probably still run a test re-build of NetworkManager+netplan integration patch, to make sure it doesn't break our current libnetplan consumer.)
Although I'd like to try keeping our dependency tree small, your introduction of CMocka as a new testing framework, is OK, as it is covering a new usecase: To catch memory leaks, as already used in #297 Also, it's only used during build/test time and when coding locally (i.e. no new runtime dependency).
You modified the meson.build
system, while not keeping the Makefile
based build up to date... I wonder if we should just drop the Makefile (or make it call meson
instead), now that major Distros like Debian and Ubuntu are using the meson based build? If not, we'd want to keep both systems updated in parallel.
Wrt to applying the libnetplan API spec, I found a few things missing, as already lined out in the description above:
- netplan_netdef_match_interfaces() => exists, but is not exported publicly.
- netplan_netdef_get_output_filename() => missing. we'll probably need a new internal util for this, to calculate the path at runtime. It should return something like this path
conf_path = g_strjoin(NULL, "run/NetworkManager/system-connections/netplan-", def->id, "-", escaped_ssid, ".nmconnection", NULL);
(also for other backends like sd-networkd). - _netplan_netdef_get_vf_count/vlan_filter/embedded_switch_mode/delay_rebind. missing or partially implemented, but not using the
_
"private" prefix, also not using the new string handling method (e.g. for ..._embedded_switch_mode).
Other observations from the API spec (spec should be updated accordingly):
- netplan_get_id_from_nm_filename => should be using the new string handling method, e.g. by copying it into "netplan_get_id_from_nm_filepath" (keeping the _filename function for backwards compat)
- netplan_get_filename_by_id => should be NETPLAN_DEPRECATED, as it isn't used anywhere and implemented in
abi_compat.c
.
There are several inline comments for you to have a look at, too.
README.md
Outdated
@@ -21,7 +21,7 @@ Netplan's [documentation objectives](https://docs.google.com/document/d/1n47hwLm | |||
|
|||
Steps to build netplan using the [Meson](https://mesonbuild.com) build system inside the `build/` directory: | |||
|
|||
* meson setup build --prefix=/usr [-Db_coverage=true] | |||
* meson setup build --prefix=/usr [-Db_coverage=true] [-Dunit_testing=true] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* meson setup build --prefix=/usr [-Db_coverage=true] [-Dunit_testing=true] | |
* meson setup build --prefix=/usr -Dunit_testing=true [-Db_coverage=true] |
I think we want to recommend running all the tests here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. In this case this option could be 'true' by default then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: Yes and no... I was thinking about keeping it disabled by default, but recommending to enabled unit_testing
inside the README, which is geared towards developers and people building Netplan themselves.
Not a big deal either way, as this only affects people building Netplan, who should be able to manage the build time options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for addressing my comments! We're getting really close to getting this merged now, but I left a 2nd round of (mostly small) inline comments.
README.md
Outdated
@@ -21,7 +21,7 @@ Netplan's [documentation objectives](https://docs.google.com/document/d/1n47hwLm | |||
|
|||
Steps to build netplan using the [Meson](https://mesonbuild.com) build system inside the `build/` directory: | |||
|
|||
* meson setup build --prefix=/usr [-Db_coverage=true] | |||
* meson setup build --prefix=/usr [-Db_coverage=true] [-Dunit_testing=true] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: Yes and no... I was thinking about keeping it disabled by default, but recommending to enabled unit_testing
inside the README, which is geared towards developers and people building Netplan themselves.
Not a big deal either way, as this only affects people building Netplan, who should be able to manage the build time options.
* Implement some of the new APIs described in [1] and add some functions that were intended for internal use to the public header files. * Implement/change unit tests related to the changes. * Introduce some unit tests written in C using the cmocka framework. The idea is to make it easier to catch runtime problems like memory errors. One can just build the tests with -fsanitize=address or run them with valgrind to check for problems. The new files will not be compiled by default, the new option -Dunit_testing=true must be used during the meson setup step. * Integrate the new tests with the meson build configuration. * Add a .gcovr file to exclude the new tests from the coverage test. It a future commit we might just move the new tests directory to another place. * Refactor the public header files Definitions used by other public header files were moved to a new types.h file. The new file was included where it's needed and it shouldn't break external consumers. * Add new netplan_error functions with tests. * Enable cmocka unittests during coverage test. [1] - https://discourse.ubuntu.com/t/spec-api-and-bindings-definition-for-libnetplan/29106
- Enable the C tests by default - Set src as an include directory so the includes in the C test files can be simplified - Use meson.project_source_root() instead of meson.global_source_root()
- Fix code style - Restore original signature of old API functions
- Update types.h header
Remove the attribute declaration from function implementations as they are already defined in the header files.
Use netplan_state_get_netdef() instead
- Add a helper to create the netplan_state used by most of the tests and reduce code duplication. - Add tests for the new APIs
netplan_get_id_from_nm_filepath() does the same as netplan_get_id_from_nm_filename() but uses the new string handling method defined in the spec. Reimplement netplan_get_id_from_nm_filename() using the new netplan_get_id_from_nm_filepath(). Implement unit test for the new function.
- Install cmocka for autopkgtest (while we don't have it as a package dependency) - Disable ctests during the build-abi test stage
Don't pass the exact size of the destination string to copy functions.
- Simplify memory handling in netplan_get_id_from_nm_filename
- Use g_error_free instead of netplan_error_free. If the new generate binary is somehow updated without the new libnetplan it would break.
- Move netplan_netdef_get_embedded_switch_mode() to abi_compat.c
- Mark netplan_get_id_from_nm_filename as deprecated and move it to abi_compat.c. - Change netplan_parser_load_keyfile to use the new API (as the old one now has the "deprecated" attribute, the compiler started to catch it).
- Move compiler parameters required for the test files to their meson.build file. - Ignore deprecation warnings when building tests.
This check is already performed by netplan_copy_string()
- "src" is used only by the test build
- Move tests of deprecated APIs to their own file - test_netplan_error: simplify test_netplan_error_code - test_utils: refactor load_fixture_to_netplan_state to use glib helpers.
- Rename netplan_netdef_get_delay_virtual_functions_rebind to the new API name and make the old name be an alias for the new one so one day we can just drop the alias without major changes.
Drop _netplan_netdef_get_vf_count in favor of _netplan_state_get_vf_count_for_def
So we will remember to refactor the code to use it in the future.
Also, some new tests were moved to test_netplan_deprecated by mistake. Move them back to the original file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for resolving all the remarks! This is looking good to me, after comparing it to the spec again.
Let me rebase this on top of origin/main
and add a small comment about the netplan_error_code
return type, to clarify this situation in the header file (public API). And get it merged after it passes all the tests.
[1] - https://discourse.ubuntu.com/t/spec-api-and-bindings-definition-for-libnetplan/29106
Description
Some APIs still need to be implemented:
netplan_netdef_get_output_filename()
netplan_netdef_match_interface()
_netplan_netdef_get_vf_count/vlan_filter/embedded_switch_mode/delay_rebind
The new cmocka unit testing infrastructure could be in another PR. Although I decided to include it here because it was easy to integrate, the new tests are automatically taken into account during the coverage test (and I'm already taking advantage of it) and it helps catching memory errors. My goal is to have a complete test suite for the C code with it.
The Netplan + NetworkManager integration patch builds fine with these changes.
Checklist
make check
successfully.make check-coverage
).