API: update netplan_delete_connection() to avoid spawning another process by slyon · Pull Request #322 · canonical/netplan

slyon · 2023-02-01T12:06:41Z

Description

This function has been calling into the python 'netplan set' command for historical reasons. We can now stay internal to libnetplan and execute the corresponding libnetplan functions directly.

Basically, we're now implementing inside this function in util.c what has previously been executed via netplan/cli/commands/set.py

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).
New/changed keys in YAML format are documented.
(Optional) Adds example YAML for new feature.
(Optional) Closes an open bug in Launchpad.

sil2100

Looks good from my POV, but then again I don't really know the exact logic for netplan set anymore so I'm not leaving any Approval/Disapproval. Is the intent to move away from calling netplan set from inside the compiled netplan for various cases? Since I see dbus.c also uses netplan set.

I took a look on how netplan set is written and the workflow here at least makes sense.

sil2100 · 2023-02-01T12:47:41Z

+    rewind(patch);
+    if (   !netplan_parser_load_nullable_fields(output_parser, patch_fd, &error)
+        || !netplan_parser_load_yaml_hierarchy(output_parser, rootdir, &error)) {
        // LCOV_EXCL_START


Just out of curiosity: why are we excluding those multiple error cases from coverage? Are they being tested in integration testing?

Those are things which are really hard to test. They are not being tested in integration testing either. It's basically file system errors or errors in the emitter and/or parser of libyaml, which we cannot easily influence.

I guess I could mock some of the netplan_ functions, when adding new unit tests as part of our new C based cmoka tests in tests/ctest/.. Maybe I should do this to test more of the error reporting cases.

slyon · 2023-02-01T13:19:31Z

Is the intent to move away from calling netplan set from inside the compiled netplan for various cases? Since I see dbus.c also uses netplan set.

Yes, indeed. We want to avoid spawning new processes from inside compiled (lib-)netplan. In the past all of netplan set including it's YAML parser was implemented in Python, so we just called into that python code from the libnetplan API. Now the parser is actually implemented in netplan itself, so a call of this library function would call into the python code, which would in turn call back into libnetplan. We want to avoid this libnetplan -> python -> libnetplan back and forth.

dbus.c is a mess and should be refactored eventually consuming all of the new API. But it's also a stand-alone binary, so not that much of a problem as calling back and forth into the library.

schopin-pro

I'm not approving because of the small tmpfile issue, but overall I really like the patch.

In an ideal world we'd have a netplan_state_delete_netdef function that wouldn't require us to craft a YAML payload, but that's a bit further away ;)

This enables us to create anonymous files in memory (memfd_* functions). _GNU_SOURCE includes _DEFAULT_SOURCE, which includes __USE_MISC (used for the GLOB_BRACE flag in glob.h), so we should avoid setting it manually/explicitly.

daniloegea

It looks good. Only requesting a minor change to avoid a leak when stdin is closed.

This function has been calling into the python 'netplan set' command for historical reasons. We can now stay internal to libnetplan and execute the corresponding libnetplan functions directly.

The find_yaml_glob() function is Netplan internal and only used from netplan-dbus, which has a hard dependency on libnetplan of the same version. ABI LOG: Run abidiff abi-compat/jammy_0.105.xml _build/src/libnetplan.so.0.0 --headers-dir2 include/ --header-file2 src/abi.h --suppressions abi-compat/suppressions.abignore --no-added-syms Functions changes summary: 0 Removed, 1 Changed (46 filtered out), 0 Added (18 filtered out) functions Variables changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added variable 1 function with some indirect sub-type change: [C] 'function int find_yaml_glob(const char*, glob_t*)' at util.c:116:1 has some indirect sub-type changes: parameter 2 of type 'glob_t*' has sub-type changes: in pointed to type 'typedef glob_t' at glob.h:105:1: underlying type 'struct glob_t' at glob.h:82:1 changed: type size hasn't changed 3 data member changes: type of 'void* (void*)* gl_readdir' changed: in pointed to type 'function type void* (void*)': return type changed: in pointed to type 'void' at dirent.h:22:1: entity changed from 'void' to 'struct dirent' at dirent.h:22:1 type size changed from 0 to 2240 (in bits) type of 'int (const char* restrict, void* restrict)* gl_lstat' changed: in pointed to type 'function type int (const char* restrict, void* restrict)': parameter 2 of type 'void* restrict' changed: 'void* restrict' changed to 'stat* restrict' 'int (const char* restrict, void* restrict)* gl_stat' has *some* difference - please report as a bug

daniloegea

Looks good. Just the change to _GNU_SOURCE could use another pair of eyes.

Regarding the ABI breakage, I tested Network Manager built with an old version of libnetplan and it still works after this change (without rebuilding it I mean)

schopin-pro

LGTM.

Regarding the ABI changes, since we have a tight dependency I'm OK with it. However, to be clear: this kind of ABI break wouldn't result in the binary refusing to start, and instead would lead to fun things like stack corruption ;).

I'm guessing last time I bumped the XOPEN_SOURCE value it was an ABI break too, huh? Live & learn...

slyon · 2023-02-08T16:58:05Z

Thank you for the feedback @daniloegea and @schopin-pro! Let's get this merged.

slyon requested review from daniloegea, jawn-smith and ogayot February 1, 2023 12:07

sil2100 reviewed Feb 1, 2023

View reviewed changes

ogayot reviewed Feb 1, 2023

View reviewed changes

Comment thread src/util.c Outdated

Comment thread src/util.c Outdated

slyon requested a review from schopin-pro February 1, 2023 16:45

slyon force-pushed the api_delete_connection branch from a1fee9d to 63d3af8 Compare February 2, 2023 15:37

schopin-pro reviewed Feb 2, 2023

View reviewed changes

Comment thread src/util.c Outdated

slyon force-pushed the api_delete_connection branch 4 times, most recently from ec9b848 to eefb892 Compare February 6, 2023 14:57

meson: bump to _GNU_SOURCE, avoid explicit __USE_MISC

ebec828

This enables us to create anonymous files in memory (memfd_* functions). _GNU_SOURCE includes _DEFAULT_SOURCE, which includes __USE_MISC (used for the GLOB_BRACE flag in glob.h), so we should avoid setting it manually/explicitly.

slyon force-pushed the api_delete_connection branch from eefb892 to 0b3c032 Compare February 6, 2023 15:42

daniloegea suggested changes Feb 7, 2023

View reviewed changes

Comment thread src/util.c Outdated

slyon added 2 commits February 7, 2023 14:52

util: update netplan_delete_connection() API

c4c5c90

This function has been calling into the python 'netplan set' command for historical reasons. We can now stay internal to libnetplan and execute the corresponding libnetplan functions directly.

test:ctest: adopt for netplan_delete_connection API

c068854

slyon force-pushed the api_delete_connection branch from 0b3c032 to c068854 Compare February 7, 2023 14:05

slyon requested a review from daniloegea February 7, 2023 14:08

slyon requested a review from schopin-pro February 7, 2023 16:17

daniloegea approved these changes Feb 8, 2023

View reviewed changes

schopin-pro approved these changes Feb 8, 2023

View reviewed changes

slyon merged commit c658bf0 into canonical:main Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: update netplan_delete_connection() to avoid spawning another process#322

API: update netplan_delete_connection() to avoid spawning another process#322
slyon merged 4 commits intocanonical:mainfrom
slyon:api_delete_connection

slyon commented Feb 1, 2023 •

edited

Loading

Uh oh!

sil2100 left a comment

Uh oh!

sil2100 Feb 1, 2023

Uh oh!

slyon Feb 1, 2023

Uh oh!

slyon commented Feb 1, 2023

Uh oh!

Uh oh!

Uh oh!

schopin-pro left a comment

Uh oh!

Uh oh!

daniloegea left a comment

Uh oh!

Uh oh!

daniloegea left a comment

Uh oh!

schopin-pro left a comment

Uh oh!

slyon commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

slyon commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

sil2100 left a comment

Choose a reason for hiding this comment

Uh oh!

sil2100 Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

slyon Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

slyon commented Feb 1, 2023

Uh oh!

Uh oh!

Uh oh!

schopin-pro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daniloegea left a comment

Choose a reason for hiding this comment

Uh oh!

schopin-pro left a comment

Choose a reason for hiding this comment

Uh oh!

slyon commented Feb 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

slyon commented Feb 1, 2023 •

edited

Loading