| Message ID | 20170908134548.23645-1-julien.floret@6wind.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] netsnmp: install all MIB files | expand |
On 08-09-17 15:45, Julien Floret wrote: > Since commit be8e32d585f3 ("netsnmp: configurable MIB modules"), > the list of MIB modules can be selected with a configuration option. > > However, there was still an hardcoded list of MIB files to exclude from > the target filesystem. > Since it is complicated to know which MIB files are necessary according > to the configuration, let's install all of them. > > Cc: przemyslaw <przemyslaw.wrzos@calyptech.com> > Cc: Gustavo Zacarias <gustavo@zacarias.com.ar> > Signed-off-by: Julien Floret <julien.floret@6wind.com> Looks good to me, except: shouldn't the default of BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES so that all the previous BLOAT_MIBS are there? Regards, Arnout
Hello Arnout, 2017-09-12 1:08 GMT+02:00 Arnout Vandecappelle <arnout@mind.be>: > > > On 08-09-17 15:45, Julien Floret wrote: >> Since commit be8e32d585f3 ("netsnmp: configurable MIB modules"), >> the list of MIB modules can be selected with a configuration option. >> >> However, there was still an hardcoded list of MIB files to exclude from >> the target filesystem. >> Since it is complicated to know which MIB files are necessary according >> to the configuration, let's install all of them. >> >> Cc: przemyslaw <przemyslaw.wrzos@calyptech.com> >> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar> >> Signed-off-by: Julien Floret <julien.floret@6wind.com> > > Looks good to me, except: shouldn't the default of > BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES so that all the previous BLOAT_MIBS are > there? Hum, maybe we could parse BR2_PACKAGE_NETSNMP_{WITH,WITHOUT}_MIB_MODULES to deduce BLOAT_MIBS, but to me it does not seem trivial nor elegant... What about adding a new config option BR2_NETSNMP_WITHOUT_MIB_FILES whose default value is the content of BLOAT_MIBS?
On 12-09-17 10:48, Julien Floret wrote: > Hello Arnout, > > 2017-09-12 1:08 GMT+02:00 Arnout Vandecappelle <arnout@mind.be>: >> >> >> On 08-09-17 15:45, Julien Floret wrote: >>> Since commit be8e32d585f3 ("netsnmp: configurable MIB modules"), >>> the list of MIB modules can be selected with a configuration option. >>> >>> However, there was still an hardcoded list of MIB files to exclude from >>> the target filesystem. >>> Since it is complicated to know which MIB files are necessary according >>> to the configuration, let's install all of them. >>> >>> Cc: przemyslaw <przemyslaw.wrzos@calyptech.com> >>> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar> >>> Signed-off-by: Julien Floret <julien.floret@6wind.com> >> >> Looks good to me, except: shouldn't the default of >> BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES so that all the previous BLOAT_MIBS are >> there? > > Hum, maybe we could parse > BR2_PACKAGE_NETSNMP_{WITH,WITHOUT}_MIB_MODULES to deduce BLOAT_MIBS, > but to me it does not seem trivial nor elegant... > What about adding a new config option BR2_NETSNMP_WITHOUT_MIB_FILES > whose default value is the content of BLOAT_MIBS? Let me rephrase my comment. What this patch does, basically, is to clean up the discrepancy between BR2_PACKAGE_NETSNMP_{WITH,WITHOUT}_MIB_MODULES and BLOAT_MIBS. That is absolutely a good thing. It looks to me like the current BLOAT_MIBS option is removing stuff that is already disabled by the default value of BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES. So that part is fine - your patch is not actually changing anything in the default configuration. However, it looks to me like the current BLOAT_MIBS is also removing things that are *not* included in the current default value of BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES. Therefore, after this change, we end up with a bigger rootfs (using defaults) than before. My suggestion is: wouldn't it make sense to extend the default value of BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES? As far as I can see, the current default removes DISMAN-EVENT and DISMAN-SCHEDULE already. Are there additional modules we can disable, so that also BRIDGE DISMAN-SCRIPT EtherLike RFC-1215 RFC1155-SMI RFC1213 SCTP and SMUX are gone? And can we do this without loosing functionality that we did have before? Probably extending BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES should be a separate patch anyway. Regards, Arnout
2017-09-12 13:44 GMT+02:00 Arnout Vandecappelle <arnout@mind.be>: > > > On 12-09-17 10:48, Julien Floret wrote: >> Hello Arnout, >> >> 2017-09-12 1:08 GMT+02:00 Arnout Vandecappelle <arnout@mind.be>: >>> >>> >>> On 08-09-17 15:45, Julien Floret wrote: >>>> Since commit be8e32d585f3 ("netsnmp: configurable MIB modules"), >>>> the list of MIB modules can be selected with a configuration option. >>>> >>>> However, there was still an hardcoded list of MIB files to exclude from >>>> the target filesystem. >>>> Since it is complicated to know which MIB files are necessary according >>>> to the configuration, let's install all of them. >>>> >>>> Cc: przemyslaw <przemyslaw.wrzos@calyptech.com> >>>> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar> >>>> Signed-off-by: Julien Floret <julien.floret@6wind.com> >>> >>> Looks good to me, except: shouldn't the default of >>> BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES so that all the previous BLOAT_MIBS are >>> there? >> >> Hum, maybe we could parse >> BR2_PACKAGE_NETSNMP_{WITH,WITHOUT}_MIB_MODULES to deduce BLOAT_MIBS, >> but to me it does not seem trivial nor elegant... >> What about adding a new config option BR2_NETSNMP_WITHOUT_MIB_FILES >> whose default value is the content of BLOAT_MIBS? > > Let me rephrase my comment. > > What this patch does, basically, is to clean up the discrepancy between > BR2_PACKAGE_NETSNMP_{WITH,WITHOUT}_MIB_MODULES and BLOAT_MIBS. That is > absolutely a good thing. > > It looks to me like the current BLOAT_MIBS option is removing stuff that is > already disabled by the default value of > BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES. So that part is fine - your patch is > not actually changing anything in the default configuration. > > However, it looks to me like the current BLOAT_MIBS is also removing things > that are *not* included in the current default value of > BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES. Therefore, after this change, we end up > with a bigger rootfs (using defaults) than before. > > My suggestion is: wouldn't it make sense to extend the default value of > BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES? As far as I can see, the current > default removes DISMAN-EVENT and DISMAN-SCHEDULE already. Are there additional > modules we can disable, so that also BRIDGE DISMAN-SCRIPT EtherLike RFC-1215 > RFC1155-SMI RFC1213 SCTP and SMUX are gone? And can we do this without loosing > functionality that we did have before? Thanks for the detailed response, now I understand better your comment. Honestly, I could not tell... netsnmp is quite complicated. Ensuring we don't lose any functionality after changing the default modules would probably require a deeper investigation and knowledge of netsnmp code that I'm not able to do right now. And it seems netsnmp "make install" installs MIB description files, whether or not the module(s) using them are enabled or not. What's more, if I recall correctly, modules can still work without their MIB description file - they just cannot translate object IDs into human-readable names. But without this patch, netsnmp is unusable for us, because we need the object names and this hardcoded BLOAT_MIBS list removes mandatory MIB files whatever the configuration options. (By the way, some of the MIBS in BLOAT_MIBS (RFC-1215 and RFC1155-SMI) don't seem to be suppressed, because the suffix added in NETSNMP_REMOVE_BLOAT_MIBS ("-MIB.txt") doesn't apply for them...) > Probably extending BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES should be a separate > patch anyway. Yes, probably. I'm not sure these default values are the most common case, however. Also, IMHO having two options {WITH,WITHOUT}_MIB_MODULES makes quite difficult to know which modules are really enabled in the end. I'm starting to think that maybe a better approach would be to have one Config.in option per module... This would be clearer and would perhaps also allow removing unused MIB files according to the modules that are enabled or not. So do you think this patch is acceptable for now?
On 13-09-17 14:16, Julien Floret wrote: [snip] > Honestly, I could not tell... netsnmp is quite complicated. Ensuring > we don't lose any functionality after changing the default modules > would probably require a deeper investigation and knowledge of netsnmp > code that I'm not able to do right now. And it seems netsnmp "make > install" installs MIB description files, whether or not the module(s) > using them are enabled or not. > What's more, if I recall correctly, modules can still work without > their MIB description file - they just cannot translate object IDs > into human-readable names. OK. Well, the only thing this patch does, in the end, is make the target filesystem a bit bigger. By how much, do you know? Users can always remove unneeded MIBs in a post-build script. > But without this patch, netsnmp is unusable for us, because we need > the object names and this hardcoded BLOAT_MIBS list removes mandatory > MIB files whatever the configuration options. That's a good reason! > (By the way, some of the MIBS in BLOAT_MIBS (RFC-1215 and RFC1155-SMI) > don't seem to be suppressed, because the suffix added in > NETSNMP_REMOVE_BLOAT_MIBS ("-MIB.txt") doesn't apply for them...) Yeah, the option is from the very first time the package was added 7 years ago, and hasn't been updated through all the version bumps... >> Probably extending BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES should be a separate >> patch anyway. > > Yes, probably. I'm not sure these default values are the most common > case, however. Never mind that, since it's anyway not really related, as you say. > Also, IMHO having two options {WITH,WITHOUT}_MIB_MODULES makes quite > difficult to know which modules are really enabled in the end. > I'm starting to think that maybe a better approach would be to have > one Config.in option per module... This would be clearer and would > perhaps also allow removing unused MIB files according to the modules > that are enabled or not. But that would make for a really long list of options, no? I think that that's a bit too much. > > So do you think this patch is acceptable for now? I've applied, thanks. Perhaps you could write up a CHANGES entry that explains that people should remove unneeded MIBs in a post-build script? Regards, Arnout
2017-09-13 23:24 GMT+02:00 Arnout Vandecappelle <arnout@mind.be>: [snip] > OK. Well, the only thing this patch does, in the end, is make the target > filesystem a bit bigger. By how much, do you know? Approximately 400K... > Users can always remove unneeded MIBs in a post-build script. > > >> But without this patch, netsnmp is unusable for us, because we need >> the object names and this hardcoded BLOAT_MIBS list removes mandatory >> MIB files whatever the configuration options. > > That's a good reason! > >> (By the way, some of the MIBS in BLOAT_MIBS (RFC-1215 and RFC1155-SMI) >> don't seem to be suppressed, because the suffix added in >> NETSNMP_REMOVE_BLOAT_MIBS ("-MIB.txt") doesn't apply for them...) > > Yeah, the option is from the very first time the package was added 7 years ago, > and hasn't been updated through all the version bumps... > > >>> Probably extending BR2_PACKAGE_NETSNMP_WITHOUT_MIB_MODULES should be a separate >>> patch anyway. >> >> Yes, probably. I'm not sure these default values are the most common >> case, however. > > Never mind that, since it's anyway not really related, as you say. > > >> Also, IMHO having two options {WITH,WITHOUT}_MIB_MODULES makes quite >> difficult to know which modules are really enabled in the end. >> I'm starting to think that maybe a better approach would be to have >> one Config.in option per module... This would be clearer and would >> perhaps also allow removing unused MIB files according to the modules >> that are enabled or not. > > But that would make for a really long list of options, no? I think that that's > a bit too much. > That's right, that would make maybe 20 options... OK, it's probably better to stick to the current options. > >> >> So do you think this patch is acceptable for now? > > I've applied, thanks. > > Perhaps you could write up a CHANGES entry that explains that people should > remove unneeded MIBs in a post-build script? OK, I will see what I can do. Thanks!
diff --git a/package/netsnmp/netsnmp.mk b/package/netsnmp/netsnmp.mk index abcf63d56858..742fa6e3b382 100644 --- a/package/netsnmp/netsnmp.mk +++ b/package/netsnmp/netsnmp.mk @@ -38,8 +38,6 @@ NETSNMP_MAKE = $(MAKE1) NETSNMP_CONFIG_SCRIPTS = net-snmp-config NETSNMP_AUTORECONF = YES -NETSNMP_BLOAT_MIBS = BRIDGE DISMAN-EVENT DISMAN-SCHEDULE DISMAN-SCRIPT EtherLike RFC-1215 RFC1155-SMI RFC1213 SCTP SMUX - ifeq ($(BR2_ENDIAN),"BIG") NETSNMP_CONF_OPTS += --with-endianness=big else @@ -101,14 +99,6 @@ else NETSNMP_CONF_OPTS += --disable-applications endif -define NETSNMP_REMOVE_BLOAT_MIBS - for mib in $(NETSNMP_BLOAT_MIBS); do \ - rm -f $(TARGET_DIR)/usr/share/snmp/mibs/$$mib-MIB.txt; \ - done -endef - -NETSNMP_POST_INSTALL_TARGET_HOOKS += NETSNMP_REMOVE_BLOAT_MIBS - ifeq ($(BR2_PACKAGE_NETSNMP_SERVER),y) define NETSNMP_INSTALL_INIT_SYSV $(INSTALL) -D -m 0755 package/netsnmp/S59snmpd \
Since commit be8e32d585f3 ("netsnmp: configurable MIB modules"), the list of MIB modules can be selected with a configuration option. However, there was still an hardcoded list of MIB files to exclude from the target filesystem. Since it is complicated to know which MIB files are necessary according to the configuration, let's install all of them. Cc: przemyslaw <przemyslaw.wrzos@calyptech.com> Cc: Gustavo Zacarias <gustavo@zacarias.com.ar> Signed-off-by: Julien Floret <julien.floret@6wind.com> --- package/netsnmp/netsnmp.mk | 10 ---------- 1 file changed, 10 deletions(-)