libnetplan: expose coherent generator APIs #239

schopin-pro · 2021-10-11T15:48:22Z

Description

New APIs for the generator APIs of libnetplan, with proper error handling and support for the NetplanState feature.

API and ABI compatibility is retained for now. Depends on #232 and #234, as usual see the commits after the merge.

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).

Note that we duplicated a function, as I'd like to remove it from the API seeing as it's really not netplan-specific, but we need to keep it in the ABI. The new API has the ability to report on errors, which allows us to remove all the calls to exit() within networkd.c. Those have nothing to do in library code. We also followed the current convention of using the return value to signal errors, forcing us to use an out-parameter for the "has_been_written" return value.

The NM generator directly called exit() when something went wrong, which is not ideal in library code! We copied the API from the netword generator, hence the has_been_written new capability.

The API is now the same as for the networkd and NM generators. No more exit()ing in the middle of a library call!

slyon · 2021-10-19T09:58:02Z

src/openvswitch.h

+NETPLAN_INTERNAL gboolean
+netplan_ovs_cleanup(const char* rootdir);
+
+


Add comment about deprecated API, instead of additional whitespace.

slyon · 2021-10-19T10:25:22Z

src/openvswitch.c

        return TRUE;
    }
-    return FALSE;
+    *result = FALSE;


The result variable name is a bit unclear here. Maybe it could be called needs_ssl instead?

slyon · 2021-10-19T10:31:55Z

src/openvswitch.c

                if (def->ovs_settings.protocols && def->ovs_settings.protocols->len > 0) {
                    write_ovs_protocols(&(def->ovs_settings), def->id, cmds);
-                } else if (ovs_settings_global.protocols && ovs_settings_global.protocols->len > 0) {
-                    write_ovs_protocols(&(ovs_settings_global), def->id, cmds);
+                } else if (settings->protocols && settings->protocols->len > 0) {
+                    write_ovs_protocols(settings, def->id, cmds);
                }


we could use short-form if clauses here.

codecov-commenter · 2021-10-19T12:54:27Z

Codecov Report

Merging #239 (2b71300) into main (fd1b3c7) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #239      +/-   ##
==========================================
+ Coverage   99.06%   99.10%   +0.03%     
==========================================
  Files          58       58              
  Lines        9713     9792      +79     
==========================================
+ Hits         9622     9704      +82     
+ Misses         91       88       -3

Impacted Files	Coverage Δ
src/abi_compat.c	`100.00% <100.00%> (ø)`
src/generate.c	`100.00% <100.00%> (ø)`
src/networkd.c	`100.00% <100.00%> (ø)`
src/nm.c	`100.00% <100.00%> (+0.53%)`	⬆️
src/openvswitch.c	`100.00% <100.00%> (ø)`
tests/generator/test_auth.py	`100.00% <100.00%> (ø)`
tests/generator/test_common.py	`100.00% <100.00%> (ø)`
tests/generator/test_routing.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd1b3c7...2b71300. Read the comment docs.

slyon

This is another very good PR, removing the exit(1) calls from inside the library (that lintian has been complaining about for a long time). Thank you Simon!

I added only a few inline nits about formatting things. I will push those changes and get this PR merged.

slyon · 2021-10-19T11:07:29Z

src/nm.h

+NETPLAN_INTERNAL gboolean
+netplan_nm_cleanup(const char* rootdir);
+
+


Add comment about deprecated API, instead of additional whitespace.

slyon · 2021-10-19T11:08:15Z

src/nm.c

@@ -787,7 +789,8 @@ write_nm_conf_access_point(NetplanNetDefinition* def, const char* rootdir, const
    /* We can only write search domains and routes if we have an address */
    if (def->ip4_addresses || def->dhcp4) {
        write_search_domains(def, "ipv4", kf);
-        write_routes(def, kf, AF_INET);
+        if (!write_routes(def, kf, AF_INET, error))
+            return FALSE;


Fix test coverage.

slyon · 2021-10-19T12:30:47Z

src/networkd.c

-/**
- * Generate networkd configuration in @rootdir/run/systemd/network/ from the
- * parsed #netdefs.
- * @rootdir: If not %NULL, generate configuration in this root directory
- *           (useful for testing).
- * Returns: TRUE if @def applies to networkd, FALSE otherwise.
- */


We should probably keep that comment in networkd.c (instead of abi_compat.c)

slyon · 2021-10-19T12:43:09Z

src/networkd.c

-/**
- * Create enablement symlink for systemd-networkd.service.
- */


Let's not drop this comment, but move it to generate.c, too.

slyon · 2021-10-19T13:59:51Z

passed integration tests with ABI compatibility check:

autopkgtest [15:59:23]: @@@@@@@@@@@@@@@@@@@@ summary
ovs                  PASS
ethernets            PASS
bridges              PASS
bonds                PASS
routing              PASS
vlans                PASS
wifi                 FLAKY non-zero exit status 1
tunnels              PASS
scenarios            PASS
regressions          PASS
autostart            PASS
cloud-init           PASS
qemu-system-x86_64: terminating on signal 15 from pid 2688951 (/usr/bin/python3)

schopin-pro added 3 commits October 12, 2021 19:45

lib: nm: normalize the API with error handling

bb1ee5e

The NM generator directly called exit() when something went wrong, which is not ideal in library code! We copied the API from the netword generator, hence the has_been_written new capability.

lib: openvswitch: normalize the API with proper error handling

583917e

The API is now the same as for the networkd and NM generators. No more exit()ing in the middle of a library call!

schopin-pro force-pushed the netplan-generator-consistent-api branch from 0be6e22 to 583917e Compare October 12, 2021 17:52

slyon reviewed Oct 19, 2021

View reviewed changes

slyon added 5 commits October 19, 2021 14:47

openvswitch: formatting cleanup

3b65cb3

test_routing: fix coverage

46d3dc9

nm: formatting cleanup

3f170ca

generate: do not drop comments

d7f63fa

networkd: formatting & comments

2b71300

slyon approved these changes Oct 19, 2021

View reviewed changes

slyon merged commit 87c5b04 into canonical:main Oct 19, 2021

schopin-pro deleted the netplan-generator-consistent-api branch April 13, 2022 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libnetplan: expose coherent generator APIs #239

libnetplan: expose coherent generator APIs #239

schopin-pro commented Oct 11, 2021

slyon Oct 19, 2021 •

edited

Loading

slyon Oct 19, 2021

slyon Oct 19, 2021

codecov-commenter commented Oct 19, 2021 •

edited

Loading

slyon left a comment

slyon Oct 19, 2021

slyon Oct 19, 2021

slyon Oct 19, 2021

slyon Oct 19, 2021

slyon commented Oct 19, 2021

		NETPLAN_INTERNAL gboolean
		netplan_ovs_cleanup(const char* rootdir);

		NETPLAN_INTERNAL gboolean
		netplan_nm_cleanup(const char* rootdir);

libnetplan: expose coherent generator APIs #239

libnetplan: expose coherent generator APIs #239

Conversation

schopin-pro commented Oct 11, 2021

Description

Checklist

slyon Oct 19, 2021 • edited Loading

Choose a reason for hiding this comment

slyon Oct 19, 2021

Choose a reason for hiding this comment

slyon Oct 19, 2021

Choose a reason for hiding this comment

codecov-commenter commented Oct 19, 2021 • edited Loading

Codecov Report

slyon left a comment

Choose a reason for hiding this comment

slyon Oct 19, 2021

Choose a reason for hiding this comment

slyon Oct 19, 2021

Choose a reason for hiding this comment

slyon Oct 19, 2021

Choose a reason for hiding this comment

slyon Oct 19, 2021

Choose a reason for hiding this comment

slyon commented Oct 19, 2021

slyon Oct 19, 2021 •

edited

Loading

codecov-commenter commented Oct 19, 2021 •

edited

Loading