[1/1] netsnmp: install all MIB files

Message ID 20170908134548.23645-1-julien.floret@6wind.com
State Accepted
Headers show
Series
  • [1/1] netsnmp: install all MIB files
Related show

Commit Message

Julien Floret Sept. 8, 2017, 1:45 p.m.
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(-)

Comments

Arnout Vandecappelle Sept. 11, 2017, 11:08 p.m. | #1
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
Julien Floret Sept. 12, 2017, 8:48 a.m. | #2
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?
Arnout Vandecappelle Sept. 12, 2017, 11:44 a.m. | #3
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
Julien Floret Sept. 13, 2017, 12:16 p.m. | #4
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?
Arnout Vandecappelle Sept. 13, 2017, 9:24 p.m. | #5
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
Julien Floret Sept. 14, 2017, 12:22 p.m. | #6
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!

Patch

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 \