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

Added new functions, modified argument types, fixed typo #37

Merged
merged 1 commit into from
Jan 22, 2023

Conversation

aquaticus
Copy link
Contributor

  • const char* oid instead of char* oid as argument
  • use const string& instead of string
  • New dynamic handlers that accepts function pointer as an argument:
    • addDynamicIntegerHandler()
    • addDynamicReadOnlyTimestampHandler()
    • addDynamicReadOnlyStringHandler()
  • Handler to accept int (not only int*)
  • Fixed typo in guage to gauge (as in spec). addGugeHandler() marked as depreciated (new function addGaugeHandler()).
  • minor fixes in variable names to satisfy clang-tidy
  • Version bumped to 2.1.0

…unctions

Signed-off-by: aquaticus <1891400+aquaticus@users.noreply.github.com>
Copy link
Owner

@0neblock 0neblock left a comment

Choose a reason for hiding this comment

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

Thanks heaps for your PR!
The changes look good, I'm just not sure about the change to const char* for the OID handlers.
Do you have thoughts on this?

SortableOIDType* oidType = buildOIDWithPrefix(oid, overwritePrefix);
if(!oidType) return nullptr;
return addHandler(new ReadOnlyStringCallback(oidType, value), false);
}

ValueCallback* SNMPAgent::addOpaqueHandler(char* oid, uint8_t* value, size_t data_len, bool isSettable, bool overwritePrefix){

ValueCallback* SNMPAgent::addOpaqueHandler(const char *oid, uint8_t* value, size_t data_len, bool isSettable, bool overwritePrefix){
Copy link
Owner

Choose a reason for hiding this comment

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

I believe making the oid a const char* will remove the ability to dynamically create OID's when setting up the agent.
I have some projects where I need to dynamically create OID handlers on the fly, that are not known at compile time, so I think this will need to stay as char *.
Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler automatically converts char* to const char*. This is absolutely safe and acceptable (you won't get compilation error). You can pass both const strings and non-const char tables to function with const char* argument.

Opposite (const char* to char*) is treated as an error by any modern compiler (unless you cast to char*).
With const char* oid type both usages are valid:

// non-const dynamic string
char tmp[32];
sprintf(tmp, "Hello");
addOpaqueHandler(tmp,...); //OK

// const string
addOpaqueHandler("Hello",...); //OK

The only scenario you would like char* oid type is when the function modifies oid. As far as I know this is not the case (correct me if I'm wrong).

Copy link
Owner

Choose a reason for hiding this comment

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

Ah of course, You've quite right. Sorry I clearly had a brain fart.
Thanks for the detailed explanation!

@0neblock
Copy link
Owner

Thanks A lot for this contribution, it looks great.
Just going to try and get the CI tests to run on this branch before it is merged, to make sure the tests pass.
I'm going to close & re-open this to try to trigger the CI workflow to run.

@0neblock 0neblock closed this Jan 22, 2023
@0neblock 0neblock reopened this Jan 22, 2023
@0neblock 0neblock self-requested a review January 22, 2023 05:40
@0neblock
Copy link
Owner

Need to fix the CI tests, but I ran locally and things look good.
Thanks again for these changes, appreciated!

@0neblock 0neblock merged commit 812a0d8 into 0neblock:master Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants