-
Notifications
You must be signed in to change notification settings - Fork 72
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
Replace faulty solution to translate YANG union to SNMP #461
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is just the cbuf leak it could easily be mitigated with a cbuf_free() after its use on line .
214.
What is the logic for removing the sub-type check? Is it un-necessary?
* Revert previous solution to translate YANG union into SNMP types that had memory leak due to cbuf allocation * Add simple mapping from YANG union to SNMP string
5c9d7af
to
283f94b
Compare
The sub-type check was evaluated as over-engineering and for the most cases it should be ok to fallback to 'string' type |
The CI failures have to do with the more ambitious union testing. |
@olofhagsand do I understand correctly that integers are interpreted as strings of bytes and every byte is encoded by setting bits 5 and 6? |
I dont think so, where do you see that, can you provide an example? |
see failed test https://github.com/clicon/clixon/actions/runs/6707062396/job/18224931237?pr=461 |
@olofhagsand I need your suggestion about this change. It works for us well for now because we don't use YANG unions with non-strings, but it will create a technical debt till the time we start to use. So here we might want to fix leak 1st, because it's a major issue (at least for us), but after that we still need another solution to translate YANG unions to SNMP types, because the current one has significant performance issue so we definitely want to replace it. |
Yes, regarding the leak: I can easily add that patch. |
I will update this one to contain only fix for leak |
@paul-sirin I think we can close this PR, correct? |
This PR replaces faulty solution to translate YANG union type to SNMP type: