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

DEFVAL improvements #11

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

dcvmoole
Copy link

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:

MIB results: 11925 total, 10075 successful, 1012 failed to compile, 669 failed to load, 102 failed on dependency, 67 failed for other reasons
MIB results: 11925 total, 10372 successful, 995 failed to compile, 400 failed to load, 91 failed on dependency, 67 failed for other reasons

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.

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.
@dcvmoole
Copy link
Author

Perhaps worth noting is that "git diff", and apparently also github, get confused by the changes to test_defval_smiv2_pysnmp.py in the second commit. In contrast to what is being shown, the second commit only adds new code to that file. The output with --diff-algorithm=patience does reflect that.

@lextm lextm added the enhancement New feature or request label Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed to load MIB module
2 participants