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

SNMPd patches and enhancements #6276

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anand-kumar-subramanian
Copy link
Contributor

@anand-kumar-subramanian anand-kumar-subramanian commented Dec 23, 2020

SNMPd patches and enhancements including memory leak fix

Verified by running SNMP walk at the top.

Following are the various patch files and what it does.

003-Enable-IPv6.patch - This patch enables support for IPv6
0004-arp_netlink-should-drop-AF_BRIDGE-netlink-messages - Some address families, notably AF_BRIDGE, have RTM_NEWNEIGH and RTM_DELNEIGH messages that may not contain an IP address that we need later. So we drop everything that is not AF_INET or AF_INET6 silently
0005-if_index1.patch - Ignore log as we dont want show internal interfaces like Bridge, docker etc, Fixed index to keep ipAddressTable ifindex same as ifTable ifIndex
0005-ioctl-fail-debug.patch - Prints errno and ifname on ioctl failure
0010-boot_reason.patch - Reading reboot reason from file to generate coldStart/warmStart trap based on reboot reasonwarmStart trap
0011-Add-fopen-check-for-dev_snmp6.patch - Add strerror to SNMP file open failure. There is a possibility for snmpd try to open a device file( for reading statistics) which is already deleted by other applications. So in such cases we are skipping logging as error as this is not error.
0012-Use-empty-string-for-default-sysContact-and-sysLocat.patch - Removed default SNMP community public. Also made changes to have empty strings for sysLocation and sysContact with factory default configuration. As we are moving towards having a ConfigDB based SNMP configuration, made changes to the code to allow snmpd to startup without any errors if /etc/sonic/snmp.yml is not found.
0013-Reduce-log-message-severity-of-duplicate-address-ale.patch - Reduce severity level for duplicate ip error log msg in snmpd. As part of unnumbered interface configuration, one interface is assigned an ip address from a donor interface. As a result two different interfaces will have same ip address. This is being flagged as a concern in snmpd because while populating data structure for IP-MIB, some of the tables have ip address as the primary key. So duplicates are not expected. The fix is to reduce the log severity seen in snmpd from ERR to WARN as such configuration is possible.
0014-Disable-ipv6-mib.patch - Disabled ipv6-mib. Disabled deprecated IPV6-MIB as same is available in IP-MIB
0015-HR-index.patch - Fixed hrSWInstalledTable index. hrSWInstalledIndex should not be zero.
0016-Do-not-save-persistent-usm-configuration.patch - Do not save SNMP usm persistent configuration
0017-snmpv2-smi.patch - Added sysObjectId based on platform. Patch for compiling mib files.
0018-brcm-sonic-prod-mib.patch - Support for VENDOR details via SNMP, new mib file for config change trap
0019-meminfo.patch - added fix to show memAvailReal as per /proc/meminfo this is useful in debugging memory leaks
0020-ip_arp.patch - If there are more( >8k) arp entries, snmpwalk was getting timeout as snmpd takes time to load entries. So to prevent this autoreload is enabled.
0021-Disable-at-mib.patch - disabled atTable as this is depricated and enabled auto reload option for ipNetToPhysicalTable. Disable atTable as this is obsoleted.
0022-etherlike-mib-memleak.patch - Memory leak fix:freeing memory allocated for interface name before freeing ifstat structure

@lguohan lguohan added this to Assgined in brcm upstream Dec 23, 2020
@lguohan
Copy link
Collaborator

lguohan commented Dec 23, 2020

is there any behavior change and do we have description of each patch?

rc = ioctl(fd, which, ifrq);
if (rc < 0) {
- snmp_log(LOG_ERR,"ioctl %d returned %d\n", which, rc);
+ snmp_log(LOG_ERR,"ioctl %d returned %d errno:%d ifname:%s ifr_name:%s\n", which, rc, errno, name, ifrq->ifr_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

   [](start = 1, length = 2)

keep old indentation?

+ int i=0;
+ char *reboot_reason[] = {"User issued 'warm-reboot'",
+ NULL};
+ fp = fopen("/host/reboot-cause/previous-reboot-cause.txt", "r");
Copy link
Collaborator

Choose a reason for hiding this comment

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

fopen [](start = 10, length = 5)

This is feature relating to sonic. Is it possible to implement outside snmpd, like in sonic-snmpagent?

done = 1;
}
kern_db = ke->next;
+ free(ke->name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

free [](start = 9, length = 4)

This is a valid bug fix. Could you submit a PR to upstream repo?

@anand-kumar-subramanian anand-kumar-subramanian requested a review from a team as a code owner June 10, 2022 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
brcm upstream
  
Assgined
Development

Successfully merging this pull request may close these issues.

None yet

4 participants