Skip to content
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

parse: set the backend on nm-devices to NM by default #349

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

daniloegea
Copy link
Collaborator

@daniloegea daniloegea commented Apr 24, 2023

By definition, the renderer on nm-devices must be NetworkManager. Not defining it will default to networkd. Apart from the fact it doesn't make sense, it is causing a memory leak because the function g_datalist_clear() will no be called in reset_backend_settings() if the backend is not NM.

Description

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.

By definition, the renderer on nm-devices must be NetworkManager. Not
defining it will default to networkd. Apart from the fact it doesn't
make sense, it is causing a memory leak because the function
g_datalist_clear() will no be called in reset_backend_settings() if the
backend is not NM.
@daniloegea
Copy link
Collaborator Author

The memory leak above is fixed by #348

Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I was pondering if it makes sense to downgrade this to a g_debug(...) message, maybe only if renderer is NETPLAN_BACKEND_NONE.

But it's better to have this as an explicit warning. People are not supposed to be using nm-devices manually at all. It's fine for them to see a warning should they choose a wrong (or non at all) renderer.

src/parse.c Outdated Show resolved Hide resolved
@slyon slyon merged commit 550c7e5 into canonical:main Apr 24, 2023
daniloegea added a commit that referenced this pull request May 17, 2023
* parse: set the backend on nm-devices to NM by default

By definition, the renderer on nm-devices must be NetworkManager. Not
defining it will default to networkd. Apart from the fact it doesn't
make sense, it is causing a memory leak because the function
g_datalist_clear() will no be called in reset_backend_settings() if the
backend is not NM.

* parse.c: formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants