NetDefinition: ownership and cleanup (FR-786) #228

schopin-pro · 2021-09-21T16:26:03Z

Description

This PR is mostly about cleaning up the NetplanNetDefinition objects, and when to do it. There are a couple of precursor commits that either made the transition easier or fixed some bugs that were uncovered when actually trying to clean up things ;).

There are no API or ABI change, although there's one observable change with the dhcp_identifier which now defaults to NULL instead of an owned string "duid". The rationales are in the commit that makes the change.

As usual, each commit log contains some context information related to each change, and the commits are split up with the explicit intent of making it easier to review, one commit at a time.

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.

codecov-commenter · 2021-09-21T16:29:46Z

Codecov Report

Merging #228 (0d3ad14) into main (d4884cf) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 0d3ad14 differs from pull request most recent head f3e8308. Consider uploading reports for the commit f3e8308 to get more accurate results

@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
+ Coverage   99.02%   99.04%   +0.01%     
==========================================
  Files          56       57       +1     
  Lines        9347     9524     +177     
==========================================
+ Hits         9256     9433     +177     
  Misses         91       91

Impacted Files	Coverage Δ
src/netdef.c	`100.00% <100.00%> (ø)`
src/netplan.c	`100.00% <100.00%> (ø)`
src/networkd.c	`100.00% <100.00%> (ø)`
src/parse-nm.c	`100.00% <100.00%> (ø)`
src/parse.c	`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 d4884cf...f3e8308. Read the comment docs.

schopin-pro · 2021-09-22T08:25:49Z

V2: Split off the new reset code into its own file, add some more comments, fix some style issues (type *val vs type* val, I tend to write the former, the codebase uses the latter)

slyon

Simon, thank you very much for another excellent PR!
The cleanup of this big memory leak was long overdue.

I am overall +1 on your changes, but left a few inline comments that I'd like to see addressed before we merge this.

src/parse-nm.c

src/netdef.c

slyon · 2021-09-22T10:22:48Z

src/netdef.c

+    netdef->tunnel.fwmark = 0;
+    netdef->tunnel.port = 0;


Same here. This is fine, but we could be using memset() to keep it in line with the cleanup of the other structs. Then move the netdef->tunnel.mode = NETPLAN_TUNNEL_MODE_UNKNOWN after memset().

I'm still undecided between memset()-ing the substructs vs initializing every component to 0, hence the lack of coherence.

I'll go with memset() for now to get this out the doors, but in the future I think I'd rather name the structs and use dedicated reset_ functions with explicit defaults for each field. It wouldn't add that much more noise anyway 😁

Yeah.. I was leaning towards explicitly initializing every component to 0 at first as well. But OTOH I also liked the forward-compatibility of memset() to initialize everything, including future fields, to 0, similar to how g_new0() creates this data structure in the first place.
I'd be fine with both, I guess :-)

schopin-pro

Thanks for the thorough review! There were quite a few nice spots in there, expect a V3 later today with the mentioned changes.

src/netdef.c

schopin-pro · 2021-09-22T11:13:50Z

src/netdef.c

+    netdef->tunnel.fwmark = 0;
+    netdef->tunnel.port = 0;


I'm still undecided between memset()-ing the substructs vs initializing every component to 0, hence the lack of coherence.

I'll go with memset() for now to get this out the doors, but in the future I think I'd rather name the structs and use dedicated reset_ functions with explicit defaults for each field. It wouldn't add that much more noise anyway 😁

Some of the strings that were stored in the generated backend settings had unclear ownership, leading to either leaks, double-free, or dangling pointers. This patch forces the duplication in order to simplify handling afterwards, and documents some assumptions that aren't shown through the typesystem (I'm looking at you, GData *).

This centralizes the place where we know about "duid", and makes my life easier when writing reset/freeing code for netdef as I don't have to reallocate a string after I've cleared them all.

schopin-pro · 2021-09-22T11:45:27Z

V3: Addressed comments in review, and tweaked slightly the first patch to document passthrough a bit more.

Those functions do a full in-depth cleaning and reset each value to its default. I chose to put them in a separate source file as I feel we're getting away from the notion of parsing and much more into the object management after its creation. In all honesty, if the netplan_netdef_new function didn't reference a static data structure, I would have moved it into the new module as well (and probably will in my later API-breaking patchset, but that's a story for another time).

With this patch, we now decide arbitrarily that the owner of the netdef objects is the netdef_ordered list, and all other pointers are weak references. Knowing this, we can now easily clean up all netdefs. Since _ordered is the owner of the global netdefs, it doesn't make sense to change the cur_netdef reference before the new object is in the global array, which is why we do the netdef initialization on a local-only object, which we hand over only after it has been registered in _ordered.

src/netdef.c

slyon

Thanks for addressing all my concerns! This looks good to me.

I only left one small inline comment about a dangling pointer to settings->nm.passthrough, that I'd like to ask you to fix.

Other than that.. as this is a pretty big change, I'd like to run the full suite of (integration) tests on this PR before I merge it. That will probably be tomorrow.

slyon · 2021-09-23T08:22:50Z

OK, thanks for clarifying the g_datalist_clear() situation.
All unit- and integration-tests are looking good, let's move on with this!

$ time autopkgtest ../netplan.io_0.103-0wip99_amd64.changes -U -- qemu /data/autopkgtest-impish-amd64.img
autopkgtest [09:19:58]: starting date: 2021-09-23
autopkgtest [09:19:58]: version 5.16ubuntu1
[...]
autopkgtest [10:20:50]: @@@@@@@@@@@@@@@@@@@@ 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 51051 (/usr/bin/python3)

real	60m52,951s
user	36m2,680s
sys	2m23,091s

schopin-pro requested a review from slyon September 21, 2021 16:26

schopin-pro force-pushed the netdef-reset-duid branch 3 times, most recently from 0d3ad14 to 0bc84ff Compare September 22, 2021 08:24

slyon requested changes Sep 22, 2021

View reviewed changes

schopin-pro commented Sep 22, 2021

View reviewed changes

schopin-pro added 2 commits September 22, 2021 13:28

lib: use NULL for the dhcp_identifier default case

ee22d36

This centralizes the place where we know about "duid", and makes my life easier when writing reset/freeing code for netdef as I don't have to reallocate a string after I've cleared them all.

schopin-pro force-pushed the netdef-reset-duid branch 2 times, most recently from ba8777b to 5127d59 Compare September 22, 2021 11:44

schopin-pro added 2 commits September 22, 2021 14:22

schopin-pro force-pushed the netdef-reset-duid branch from 5127d59 to f3e8308 Compare September 22, 2021 12:22

slyon reviewed Sep 22, 2021

View reviewed changes

src/netdef.c Show resolved Hide resolved

slyon approved these changes Sep 22, 2021

View reviewed changes

slyon merged commit 73d4024 into canonical:main Sep 23, 2021

schopin-pro mentioned this pull request Sep 24, 2021

lib: introduce the notion of NetplanState #232

Merged

5 tasks

slyon mentioned this pull request Sep 30, 2021

API/ABI: restrict the symbol export to a determined public API #227

Merged

3 tasks

schopin-pro deleted the netdef-reset-duid branch April 13, 2022 09:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetDefinition: ownership and cleanup (FR-786) #228

NetDefinition: ownership and cleanup (FR-786) #228

schopin-pro commented Sep 21, 2021

codecov-commenter commented Sep 21, 2021 •

edited

Loading

schopin-pro commented Sep 22, 2021

slyon left a comment

slyon Sep 22, 2021

schopin-pro Sep 22, 2021

slyon Sep 22, 2021

schopin-pro left a comment

schopin-pro Sep 22, 2021

schopin-pro commented Sep 22, 2021

slyon left a comment

slyon commented Sep 23, 2021 •

edited

Loading

NetDefinition: ownership and cleanup (FR-786) #228

NetDefinition: ownership and cleanup (FR-786) #228

Conversation

schopin-pro commented Sep 21, 2021

Description

Checklist

codecov-commenter commented Sep 21, 2021 • edited Loading

Codecov Report

schopin-pro commented Sep 22, 2021

slyon left a comment

Choose a reason for hiding this comment

slyon Sep 22, 2021

Choose a reason for hiding this comment

schopin-pro Sep 22, 2021

Choose a reason for hiding this comment

slyon Sep 22, 2021

Choose a reason for hiding this comment

schopin-pro left a comment

Choose a reason for hiding this comment

schopin-pro Sep 22, 2021

Choose a reason for hiding this comment

schopin-pro commented Sep 22, 2021

slyon left a comment

Choose a reason for hiding this comment

slyon commented Sep 23, 2021 • edited Loading

codecov-commenter commented Sep 21, 2021 •

edited

Loading

slyon commented Sep 23, 2021 •

edited

Loading