-
Notifications
You must be signed in to change notification settings - Fork 195
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
Fix a memory leak, an assert and an error message #350
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.
Thanks for spotting and fixing more papercuts!
I put one tiny suggestion inline (up to you if you want it that way!), and one comment about your fix in nm.c
which I'd rather like to see fixed at its origin in parse-nm.c
.
03a7ff0
to
009c36f
Compare
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.
Your approach wfm! It's good to warn the user when an entry is deleted/ignored.
009c36f
to
87414c7
Compare
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.
Two more inline comments:
- We cannot be sure the
renderer
already got set in theparse.c
stage, we need to check that invalidation.c
stage, maybe using ahas_backend_settings_nm
helper variable. - We might want to drop the
NetplanBackendSettings
union all together.backend_settings.networkd.union
doesn't seem to be used anywhere. Replacing the union by juststruct NetplanNMSettings
should be ABI-safe IMO.
If the keyfile group.key is malformed, the function g_key_file_set_string() will trigger a critical assertion.
87414c7
to
5062506
Compare
I moved the wireguard part to another PR (#352). I converted the NetplanBackendSettings union to a struct as suggested, I think it makes sense. |
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, this is looking good now!
Just two little nitpicks, maybe you can squash them into their relevant commits, so we can merge this PR with a clean history:
- nitpick: nm.c:write_fallback_key_value() "backend_settings.nm.passthrough" in comment should be adopted, too.
- See inline comment
E.g.:
diff --git a/src/nm.c b/src/nm.c
index 112e1ff6..1d90e54b 100644
--- a/src/nm.c
+++ b/src/nm.c
@@ -517,7 +517,7 @@ write_vxlan_parameters(const NetplanNetDefinition* def, GKeyFile* kf)
/**
* Special handling for passthrough mode: read key-value pairs from
- * "backend_settings.nm.passthrough" and inject them into the keyfile as-is.
+ * "backend_settings.passthrough" and inject them into the keyfile as-is.
*/
static void
write_fallback_key_value(GQuark key_id, gpointer value, gpointer user_data)
diff --git a/src/validation.c b/src/validation.c
index e159a7c2..c87d7f67 100644
--- a/src/validation.c
+++ b/src/validation.c
@@ -413,7 +413,7 @@ validate_netdef_grammar(const NetplanParser* npp, NetplanNetDefinition* nd, GErr
backend = npp->global_backend;
if (nd->has_backend_settings_nm && backend != NETPLAN_BACKEND_NM) {
- return yaml_error(npp, NULL, error, "%s: networkmanager.passthrough settings found but renderer is not NetworkManager.", nd->id);
+ return yaml_error(npp, NULL, error, "%s: networkmanager backend settings found but renderer is not NetworkManager.", nd->id);
}
valid = TRUE;
If the definition has NetworkManager custom settings, validate if NetworkManager is used as renderer. A new netdef fields was defined, has_backend_settings_nm, to keep track if custom NetworkManager settings were found during parsing.
The networkd settings found in the union NetplanBackendSettings is not used anywhere. Removing it and converting the union into a struct shouldn't change the memory layout as we are keeping the bigger part of it.
5062506
to
432c6ca
Compare
Description
Checklist
make check
successfully.make check-coverage
).