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

Correct snmp function usage #12714

Merged
merged 3 commits into from Apr 9, 2021
Merged

Conversation

Jellyfrog
Copy link
Member

Please give a short description what your pull request is for

DO NOT DELETE THE UNDERLYING TEXT

Please note

Please read this information carefully. You can run ./lnms dev:check to check your code before submitting.

  • Have you followed our code guidelines?
  • If my Pull Request does some changes/fixes/enhancements in the WebUI, I have inserted a screenshot of it.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

$device->version = snmp_get($device, $firmware_oid, '-Oqv') ?: null;
$device->hardware = trim($device->sysDescr . ' ' . snmp_get($device, $hardware_oid, '-Oqv'));
$device->version = snmp_get($this->getDeviceArray(), $firmware_oid, '-Oqv') ?: null;
$device->hardware = trim($device->sysDescr . ' ' . snmp_get($this->getDeviceArray(), $hardware_oid, '-Oqv'));
Copy link
Member Author

@Jellyfrog Jellyfrog Apr 6, 2021

Choose a reason for hiding this comment

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

I suspect this doesn't break/fixes the tests because we don't have any real test data for it :)

@@ -111,7 +111,7 @@ public function discoverWirelessClients()
// Clients Per SSID
foreach ($ssid_data as $index => $entry) {
$combined_oid = sprintf('%s::%s.%s', $ai_mib, 'aiSSIDClientNum', $index);
$oid = snmp_translate($combined_oid, 'ALL', 'arubaos', '-On', null);
$oid = snmp_translate($combined_oid, 'ALL', 'arubaos', '-On', $this->getDeviceArray());
Copy link
Member

Choose a reason for hiding this comment

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

device is not required for snmp_translate().

Should either omit the 'arubaos' mib directory or the device argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

>>> snmp_translate('foobar'', 'ALL', 'arubaos', '-On', null);
<warning>PHP Notice:  Trying to access array offset on value of type null in librenms/includes/snmp.inc.php on line 50</warning>
<warning>PHP Notice:  Trying to access array offset on value of type null in librenms/includes/snmp.inc.php on line 51</warning>
<warning>PHP Notice:  Trying to access array offset on value of type null in librenms/includes/snmp.inc.php on line 68</warning>

function get_mib_dir($device)
{
$dirs = [];
if (file_exists(Config::get('mib_dir') . '/' . $device['os'])) {
$dirs[] = Config::get('mib_dir') . '/' . $device['os'];
}
if (isset($device['os_group'])) {
if (file_exists(Config::get('mib_dir') . '/' . $device['os_group'])) {
$dirs[] = Config::get('mib_dir') . '/' . $device['os_group'];
}
if ($group_mibdir = Config::get("os_groups.{$device['os_group']}.mib_dir")) {
if (is_array($group_mibdir)) {
foreach ($group_mibdir as $k => $dir) {
$dirs[] = Config::get('mib_dir') . '/' . $dir;
}
}
}
}
if ($os_mibdir = Config::get("os.{$device['os']}.mib_dir")) {
$dirs[] = Config::get('mib_dir') . '/' . $os_mibdir;
}
return $dirs;
}

Copy link
Member

Choose a reason for hiding this comment

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

The default for the device argument is [], just delete the stupid null.

Copy link
Member Author

Choose a reason for hiding this comment

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

I fixed it better instead

* @return string
*/
function snmp_translate($oid, $mib = 'ALL', $mibdir = null, $options = null, $device = [])
function snmp_translate($oid, $mib = 'ALL', $mibdir = null, $options = null, $device = null)
Copy link
Member

Choose a reason for hiding this comment

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

This should only accept array.

Copy link
Member Author

@Jellyfrog Jellyfrog Apr 9, 2021

Choose a reason for hiding this comment

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

No that's what we wanted to get away from.
No point in sending a empty array when we're only interested in it if it's a "device array".
It's sent to mibdir which does a is_array check

Copy link
Member Author

Choose a reason for hiding this comment

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

Best would ofc be to send the device object instead... One day. One day.

includes/snmp.inc.php Outdated Show resolved Hide resolved
@murrant murrant merged commit 643c1ca into librenms:master Apr 9, 2021
@librenms-bot
Copy link

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/21-4-0-changelog/15528/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants