DEFVAL improvements #11
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
This commit aims to define clearly which combinations of DEFVAL value
types and underlying syntax types are supported, and to implement
support for those cases. Various bugs in the previous code are resolved
as well. The overall result is that the generated Python code provides
proper default values for object types in more cases than before, and
that it no longer assigns values that are not used or usable by pyasn1.
Concretely, the following DEFVAL-related bugfixes are applied:
- Empty quoted strings are no longer discarded;
- Decimal values are no longer assigned to string-type and OID-type
objects, which used to lead to corrupted default values;
- Integer values with hex/binary notation are no longer assigned
incorrectly, previously resulting in them being discarded;
- For strings, binary notation no longer discards leading zero octets;
- Leading zeroes are added to hex/binary notation values with invalid
lengths, which often used to result in unintended default values;
- Numeric values for enumerated integers that fall outside of the
permitted enumeration values are now discarded;
- Code generation no longer produces extra unnecessary _Xyz_Type type
classes without contents.
In addition, the code is changed so that it no longer fails compiling
MIBs for any case in which a DEFVAL value can be parsed but not
processed. The result is that fewer MIBs fail compilation altogether.
As part of this commit, the functions that process default values are
restructured, primarily to filter by the syntax base type first and only
then by the input type, rather than the other way around. That change
opens up the possibility of applying additional checks to the default
values in a central place, as will be done in a follow-up commit.
The code is cleaned up somewhat as well. The intermediate code no longer
uses the same gen_def_val() method for two different purposes. The
template code now has fewer cases handling default values. Unused DEFVAL
code is removed from the symtable code. A few Python2 remnants are
cleaned up as well.
The test set is extended with many more DEFVAL test cases. As part of
that, all the DEFVAL-specific tests are moved to a new module, as their
original location was getting too large.
This commit slightly alters the JSON output format.
For DEFVAL clauses associated with object type syntaxes that are: 1. derived from octet strings or (non-enumerated) integers, and, 2. sub-typed with value range or size constraints, ..the DEFVAL values are now checked against those constraints, and discarded if they do not adhere to the constraints. As a result, any DEFVAL values that would otherwise cause a MIB loading failure due to pyasn1 value constraint errors, are now kept out of the generated Python code altogether. That allows many more slightly buggy MIBs to be loaded and used than before, albeit without the offending DEFVAL values of course. The test set is extended to cover the new functionality.
|
Perhaps worth noting is that "git diff", and apparently also github, get confused by the changes to |
|
Thanks. We were able to confirm the changes are a very nice addition and resolve many known issues. But we still want to report errors in strict mode if possible to point out common MIB document issues. This might take a little bit longer before we accept and merge this pull request. |
|
Decided to accept and ship them now. Improvements might be added in future releases. |
|
Thank you! |
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.
This branch improves DEFVAL support across the board. The first commit properly defines what is to be supported and implements that, making numerous small improvements to DEFVAL processing as a result. It also restructures the code, thereby paving the way for the second commit. That commit discards DEFVAL values that do not match the object syntax's range/size constraints, which allows many more compiled MIBs to be loaded by pysnmp. It also resolves lextudio/pysnmp#147. Overall, this branch should leave DEFVAL processing in proper shape.
The before-and-after statistics against the mibs.pysnmp.com collection (as before, with the script from PR #3) are as follows:
Note that in the "before" case, the number of successes already improved from 10034 (as from PR #8) to 10075 MIBs. That is due to the merge of regenerated base MIBs in pysnmp, which in particular enabled about 40 MIBs to import ObjectName from SNMPv2-SMI. That type was previously (mistakenly) not exported from there.
In any case, this branch adds 297 successes, bringing the total from the collection to a hair short of 87%. In addition, another ~180 MIBs from this collection end up with improvements with respect to their default values. I still don't know if anybody actually ever uses those, but it certainly can't hurt.
The test set is extended accordingly, and is now up to 653 tests in total. The resulting code coverage of the most important modules in
pysmi/codegen/is starting to look pretty decent, too.Zooming out a bit: this branch resolves the first point from the "Looking further ahead" section of PR #3. I do still plan on addressing the second point (broken imports) as well, but that is going to take a while longer. In addition to that, I think the best long-term plan is basically to get the "failed to load" numbers down to zero, in large part by turning various load-time errors into compile-time errors. As we have seen, debugging load-time errors is difficult for users, as well as confusing ("it compiled, so why doesn't it load?"). I think that if pysmi can at all determine that the compiled file won't load, it should refuse to compile it. That too will require some extra work; I will see what I can contribute.