Fix #80661: Add string output format function for SNMP #6633
Conversation
- This function provides support for the SNMP Value Output Format when handling strings - There are three options GUESS, HEX, and ASCII - This functionality is available in the snmp tools command set, but not in PHP
| case NETSNMP_STRING_OUTPUT_ASCII: | ||
| case NETSNMP_STRING_OUTPUT_HEX: | ||
| netsnmp_ds_set_int(NETSNMP_DS_LIBRARY_ID, NETSNMP_DS_LIB_STRING_OUTPUT_FORMAT, a1); | ||
| RETURN_TRUE; |
There was a problem hiding this comment.
This return value doesn't make much sense to me. I think it would be better to make this a void function.
There was a problem hiding this comment.
See my comment about snmp_set_oid_value_format() above.
There was a problem hiding this comment.
I agree, shall we also modify the snmp_set_oid_value_format()?
|
|
||
| function snmp_set_oid_output_format(int $format): bool {} | ||
|
|
||
| function snmp_set_string_output_format(int $format): bool {} |
There was a problem hiding this comment.
Okay, now I at least see why you picked the bool return value... I would still go for : void here, as the : bool is really a backwards-compatibility leftover from a time where these functions could return both true and false.
There was a problem hiding this comment.
Taking your word for it, I'm the old new guy ;)
| echo "Checking working\n"; | ||
| var_dump(snmp_set_string_output_format(SNMP_STRING_OUTPUT_GUESS)); | ||
| var_dump(snmp_set_string_output_format(SNMP_STRING_OUTPUT_ASCII)); | ||
| var_dump(snmp_set_string_output_format(SNMP_STRING_OUTPUT_HEX)); |
There was a problem hiding this comment.
We should also fetch something here to see that the output format actually has some effect.
There was a problem hiding this comment.
Yea, the trick is, it'll be different on each system. I'll guess we can request that the sysLocation be set in the snmpd.conf and that way it'll always succeed. Add that to the test configuration.
| --SKIPIF-- | ||
| <?php | ||
| require_once(__DIR__.'/skipif.inc'); | ||
| if (!function_exists('snmp_set_string_output_format')) die('skip This function is only available if using NET_SNMP'); |
There was a problem hiding this comment.
As far as I can tell, a separate check for the function is not needed here, just the skipif.inc include is enough. If the extension is loaded, then the function will always exist.
There was a problem hiding this comment.
As I said before, I copied whole cloth from a similar function. Give it some thought as making that decision here should/will impact others as well.
|
I just developed the test case and tested, and upon further inspection, the string output functions need quite a bit of work. I won't close the pull request, but it's not ready. Just debating ripping and replacing it vs. retrofitting what's in php-snmp at the moment. |
|
@TheWitness, I'm going to convert this PR to draft; please fix the merge conflicts and improve as necessary when you have time. |
|
Yea, that's fine. Unless there are some string formatting functions that I can find, I'll have to borrow code whole cloth. |
|
There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken. |
|
Let's close for now. I hope to take this forward, but have been busy with the day job. |
This fix adds minor functionality to the php-snmp module to allow the caller to set the desired string output format for strings. The default SNMP behavior will check and if it finds that a character within a string that does not match a visible ASCII character, it will convert the entire string to a HEX-STRING: instead.
However, there are cases where the HEX-STRING is preferred even when the hex data within the string map to a ASCII character, say for example a MAC address that happens to fall 100% in the visible ASCII range.
This minor patch also includes a test to verify that the settings are working.
If the php team desires, I can backport this as applicable. Thanks!