parser: fix some error handling code #234
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
4 tasks
schopin-pro
force-pushed
the
error-code-flow
branch
from
f547704
to
af3d857
Compare
This was referenced Oct 7, 2021
slyon
approved these changes
Oct 12, 2021
The previous code flow had several issues. First, if the caller provided NULL as the GError, then errors in individual rule objects were ignored and the object simply dropped unless the from/to check failed. Second, the parsed object was added to the netdef before all checks were finished, which meant that in case of an error, the netdef was left in an invalid state. This patch uses a return-value-based error flow instead, and moves the cur_netdef bits to the end of the process when we know the object is valid. Also, we actually do some cleanup on the error path to prevent the buggy rule leaking out into the world. This is only superficial, so there will be some leakage from the subobjects, but this will be handled in a later patchset. V2: remove braces on single-line 'if' bodies
This new version checks the validity of a given AP as early as possible, starting with SSID conflict checks before even parsing the body, and only impacts the netdef at the end of the loop body. This allows us to keep the netdef without provably erroneous data, and also consolidates the error path, where we can easily add some cleanup code, which we started to do. Said cleanup will be the object of a further patchset, though.
This new version checks the validity of a given peer as early as possible, before impacting the netdef, allowing us to keep the netdef without provably erroneous data. While we're in there, we inserted some minimal cleanup, although a more thorough leak plugging will be the object of a later patch.
The previous version would simply disregard any failed checks if the caller provided error as NULL. This is problematic as the caller could simply not be interested in the detail of the errors, but would still like to know if they have a functional configuration or not...
Co-authored-by: Lukas Märdian <lukas.maerdian@canonical.com> Co-authored-by: Lukas Märdian <slyon@ubuntu.com>
schopin-pro
force-pushed
the
error-code-flow
branch
from
0e5a0e3
to
b2176f1
Compare
Codecov Report
@@ Coverage Diff @@
## main #234 +/- ##
=======================================
Coverage 99.06% 99.06%
=======================================
Files 58 58
Lines 9707 9713 +6
=======================================
+ Hits 9616 9622 +6
Misses 91 91
Continue to review full report at Codecov.
|
slyon
added a commit
that referenced
this pull request
Oct 19, 2021
New APIs for the generator APIs of libnetplan, with proper error handling and support for the NetplanState feature. API and ABI compatibility is retained for now. Depends on #232 and #234, as usual see the commits after the merge. COMMITS: * lib: networkd: normalize the API with proper error handling Note that we duplicated a function, as I'd like to remove it from the API seeing as it's really not netplan-specific, but we need to keep it in the ABI. The new API has the ability to report on errors, which allows us to remove all the calls to exit() within networkd.c. Those have nothing to do in library code. We also followed the current convention of using the return value to signal errors, forcing us to use an out-parameter for the "has_been_written" return value. * lib: nm: normalize the API with error handling The NM generator directly called exit() when something went wrong, which is not ideal in library code! We copied the API from the netword generator, hence the has_been_written new capability. * lib: openvswitch: normalize the API with proper error handling The API is now the same as for the networkd and NM generators. No more exit()ing in the middle of a library call! * openvswitch: formatting cleanup * test_routing: fix coverage * nm: formatting cleanup * generate: do not drop comments * networkd: formatting & comments Co-authored-by: Lukas Märdian <slyon@ubuntu.com>
slyon
pushed a commit
that referenced
this pull request
Nov 24, 2021
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. COMMITS: * lib: new functions *_clear to safely clear heap-allocated complex structs 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 * parse: clear the cur_addr_options pointer once the parsing is done 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. * lib: yaml: change the error goto label to err_path `error` is usually a GError** argument, and the new name is just as legible. * lib: write_netplan_conf_full: use an early-return This allows us to shift the code left, make it easier to read. * lib: util: netplan_delete_connection: clear the state at the beginning 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. * tests: clear the global state in teardowns to expose inter-test order dependency Some tests were relying on the state having been cleared in a previous state, which isn't necessarily the case. * cli/utils: force a full parse cycle before extracting ids by devtype While the current approach works for now, it'll break when we separate parser state and global state, until we do the actual validation. * lib: use an explicit parser context 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 * libnetplan: rewrite netplan_finish_parse using explicit states 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. * lib: expose new functions to generate YAML from Netplan state or defs The old functions have been rewritten as wrappers around the new APIs V2: * Add some header comments to the new functions * parse-nm: new API using the separate parser state * lib: add accessors to be able to reach filename from netdef ID * lib: rewrite process_input_file and process_yaml_hierarchy using the 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. * lib: move _write_netplan_conf to the abi_compat.c file This function is only there for legacy reasons, and should be replaced by a normalized API. * lib: netplan_get_filename_by_id: move to abi_compat.c This is an old API. The ABI compat version is entirely reimplemented using public, up-to-date functions. * lib: util: reimplement netplan_delete_connection with the new APIs This new implementation does away with global state, creating its own local state instead. V2: fix formatting * netplan:utils: mark the exception path of the devtype iter as nocover * netplan/cli/utils: move the iter ABI check in the wrapper class 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. * lib: networkd: fix a stray call to an old API 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. * generate: migrate to the new libnetplan APIs 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.
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
This PR rework some error handling paths, to achieve two results:
GError**acceptable values. Most of the code assumes that actual error signalling is done via the value returned on the stack, either viaFALSEorNULL, but a couple of places would use the GError mechanisms instead. This meant that providingGError** error = NULLwould mostly work, but for those places where the code would glance over some failures as it wouldn't be able to detect them.Checklist
make checksuccessfully.make check-coverage).