lib: use an explicit parser context #233
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #233 +/- ##
==========================================
- Coverage 99.10% 99.09% -0.02%
==========================================
Files 58 58
Lines 9810 9913 +103
==========================================
+ Hits 9722 9823 +101
- Misses 88 90 +2
Continue to review full report at Codecov.
|
schopin-pro
force-pushed
the
netplan-parser-struct
branch
5 times, most recently
from
a43e613
to
5c58924
Compare
5 tasks
schopin-pro
force-pushed
the
netplan-parser-struct
branch
8 times, most recently
from
5b58f58
to
3e75a6b
Compare
slyon
reviewed
Oct 20, 2021
slyon
reviewed
Oct 20, 2021
schopin-pro
force-pushed
the
netplan-parser-struct
branch
from
687dcd4
to
1fb7f1f
Compare
2 tasks
schopin-pro
force-pushed
the
netplan-parser-struct
branch
2 times, most recently
from
d210631
to
adde219
Compare
|
Note that while the Codecov report says I'm missing some coverage, the fact that the unit tests passed means that |
…ucts We already have the functions to actually free the objects, these helpers just wrap those to also clear up the *pointer* and nullifying it. These functions are going to be used in the parser code to clear intermediate state, notably on the error paths. Function names, signature and implementations are heavily inspired by g_datalist_clear(). V2: rename the functions to match the new scheme [prefix_]type_action
Keeping a pointer around after we've transferred ownership of the data to a netdef is a recipy for disaster, as we have no way of knowing if we're still responsible for cleaning up the NetplanAddressOptions object.
`error` is usually a GError** argument, and the new name is just as legible.
This allows us to shift the code left, make it easier to read.
The function is supposed to work on an already empty state, as it parses a whole tree and validates it afterwards. We thus clear it explicitly. This is a latent bug that might become apparent should the validation be a little more stringent, such as redefinition of already existing netdefs.
… dependency Some tests were relying on the state having been cleared in a previous state, which isn't necessarily the case.
While the current approach works for now, it'll break when we separate parser state and global state, until we do the actual validation.
Instead of directly writing into the final network state, use a separate global parser object to store any intermediate data needed during parsing. This allows us to ensure that the client can only see valid configurations, as the parser is opaque to the client, who then must call our functions to import *and validate* the data into the final network state. Since the public API doesn't allow us to pass around some context, we still store a static object to use in those APIs, but all the internal functions shouldn't touch any external data. V2: * Remove stray cur_filename global variable * Fix some formatting * Rename 'done' field to 'parsed_defs' * Reduced the nocov zone
The netplan_finish_parse implementation is moved to our ABI attic, abi_compat.c, and is now a simple wrapper around netplan_state_import_parser_results, which is the new function used to import into a Netplan state the results of a parsing operations. As before, the idea is to maintain a constantly valid Netplan state, so this function does validation before import.
The old functions have been rewritten as wrappers around the new APIs V2: * Add some header comments to the new functions
…new API scheme There are no process_input_file() replacement as it can very well be written by the client code. process_yaml_hierarchy() has been replaced by a function in utils, netplan_parser_load_yaml_hierarchy(), which leaves the error policy up to the client code rather than calling exit(). The old versions have been moved to abi_compat.c and reimplemented to keep their previous semantics.
This function is only there for legacy reasons, and should be replaced by a normalized API.
This is an old API. The ABI compat version is entirely reimplemented using public, up-to-date functions.
This new implementation does away with global state, creating its own local state instead. V2: fix formatting
This makes it possible to use the wrapper class in autonomy, and remove the implicit dependency between the netplan_get_ids_for_devtype tests and the _NetdefIdIterator tests. The latter would crash if run before the former, as the ctypes bindings wouldn't have been initialized.
THis call meant that the network file was generated using the global state as reference instead of the local np_state, making things such as the VLAN feature basically useless.
This allows us to do proper cleanup on error cases. Note that since generate was the only consumer of some of the legacy APIs, those endpoints are not hit by the testsuite anymore and are thus marked as ignored by the coverage calculations.
schopin-pro
force-pushed
the
netplan-parser-struct
branch
from
adde219
to
9e4d3eb
Compare
slyon
approved these changes
Nov 24, 2021
There was a problem hiding this comment.
Thank you Simon for handling all those comments properly. Another PR that is moving us a big step forward, kudos!
I was able to successfully run the autopkgtests locally, incl. 1 run in "ABI compat" mode (new libnetplan, old binaries).
ACK, Codecov is fine, it is only complaining because of those lines (it does not recognize the LCOV_EXCL_* stanzas):
//LCOV_EXCL_START
if (npp->ids_in_file) {
g_hash_table_destroy(npp->ids_in_file);
npp->ids_in_file = NULL;
//LCOV_EXCL_STOP
https://app.codecov.io/gh/canonical/netplan/compare/233/tree/src/parse.c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Instead of directly writing into the final network state, use a separate
global parser object to store any intermediate data needed during
parsing. This allows us to ensure that the client can only see valid
configurations, as the parser is opaque to the client, who then must
call our functions to import and validate the data into the final
network state.
Since the public API doesn't allow us to pass around some context, we
still store a static object to use in those APIs, but all the internal
functions shouldn't touch any external data.
This PR depends on #229 and #227 and is only a prequel to the proper YAML
parser unification work, which will start with the removal of the current parsing
API, replaced by one based on explicitly allocated states, in conjunction with
the #232 work. To make things easier, the latter PR has been added as a dependency
for this one.
It also picked up a dependency on #234 along the way :)
Only the commits after the merge commit are part of the PR proper, the rest are
from the dependencies.
Checklist
make checksuccessfully.make check-coverage).missing_idsystem