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

Fixed Length OID Processing Fixes #166

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

Conversation

WakkaTrout
Copy link

Sorry for the long wait on this, I have been super busy and didn't manage to get around to this.

For context: This fixes the closed issue #141 that is still present in the main branch after some initial testing.
 
The following pull request restores the Fixed Length OID processing functionality that began to cause crashes for fixed length OID strings after PEP 8 changes were applied. Additionally, this updates the SNMPv2-SMI.py file to use the newer PEP 8 compliant function names.

After some PEP 8 changes in etingof#243, the temporary variable name clashed with the name of the OID being processed.

This change renames the temporary variable to fix this clash.
After the following commit, lextudio@4c71d45, the fixed length accessor functions for the Octet String class were changed to be PEP 8 compliant.

This commit updates SNMPv2-SMI to use the PEP 8 compliant function calls.
@CLAassistant
Copy link

CLAassistant commented Jan 22, 2025

CLA assistant check
All committers have signed the CLA.

@dcvmoole
Copy link

I feel obliged to remark here that while your changes are an excellent improvement as far as I can tell, last week's commit 755e8a7 once again renamed the underlying attribute fixedLength to fixed_length, which means that the actual fixed-length functionality is now once again functionally broken (thus resulting in e.g. generation of wrong OID strings). The corresponding pysmi unit test cases are indeed failing as of last week as well.

@lextm lextm added the priority:low Low priority items. label Jan 24, 2025
This updates the fixed length attributes of the INET-ADDRESS-MIB classes to conform to PEP 8 compliance requirements
This updates the MAC Address fixed length attribute to conform to the PEP 8 compliance changes
Update the SNMPv2-TM MIB to use the updated fixed length attribute to comply to PEP 8 requirements
Updated the fixed length attributes in the TRANSPORT-ADDRESS-MIB to conform to PEP 8 requirements
@WakkaTrout
Copy link
Author

I feel obliged to remark here that while your changes are an excellent improvement as far as I can tell, last week's commit 755e8a7 once again renamed the underlying attribute fixedLength to fixed_length, which means that the actual fixed-length functionality is now once again functionally broken (thus resulting in e.g. generation of wrong OID strings). The corresponding pysmi unit test cases are indeed failing as of last week as well.

I just created a pull request in pysmi for this as well. This should update the unit tests as well as the js file from the mib parsing

@lextm
Copy link

lextm commented Jan 24, 2025

We decided to delay changes related to PEP 8 to a future release, so these two pull requests from @WakkaTrout will not be reviewed or merged until then.

@lextm lextm requested a review from Copilot January 24, 2025 22:54

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

pysnmp/smi/mibs/SNMPv2-SMI.py:1232

  • [nitpick] The variable name fixed_length is inconsistent with the naming convention used in the rest of the code. It should be renamed to fixedLength.
fixed_length = obj.get_fixed_length()
@lextm
Copy link

lextm commented Jan 27, 2025

BTW, this pull request has no test case to show what has been fixed. The information included in #141 is a good start, but not enough.

@WakkaTrout
Copy link
Author

WakkaTrout commented Jan 30, 2025

I can add unit tests (my testing has been ad hoc testing with a remote device I put MIB's on (the device is running netsnmp)). I see the tests in the tests folder and I see them reference the LEXTUDIO-TEST-MIB (which I assume is hosted at https://mibs.pysnmp.com/asn1/ somewhere). Do I have the ability to edit this MIB to add various other tables for testing? If so, how can I go about this?

@lextm
Copy link

lextm commented Jan 30, 2025

@WakkaTrout The MIB documents behind https://mibs.pysnmp.com can be found in this GitHub repo,

https://github.com/lextudio/mibs.pysnmp.com

You can create pull requests there if you want to add your own test MIB files, or you want to modify LEXTUDIO-TEST-MIB to include some objects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:low Low priority items.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants