search

sleepinggenius2 / gosmi

Parsing MIBs so you don't have to!
MIT License
101 stars 35 forks source link

Only a single global state #47

Open stampflit opened 1 month ago

stampflit commented 1 month ago

When loading any MIBs with this library, it's global state is modified. From what I understand, there is no (reasonable) way to have two different Gosmi instances which have the SMIs from different MIBs loaded.

To me, this feels like an anti-pattern. In certain cases one wants to use different sets of MIBs for different SNMP endpoints; e.g. to work around conflicting MIBs. One could also want to try loading some more MIBs amidst regular operation of the software, to check whether certain MIBs would be helpful. And when testing code which builds on top of this library, when interactively working on a MIB definition itself and repeatedly trying it out or when analyzing a big pile of MIB files, this limitation to a single instance becomes a major hurdle.

I'm not suggesting to overthrow the entire api as that would lead to a lot of breakage. I suggest we create a new struct for Gosmi state and turn all existing functions into methods working on that state. We then instantiate one single global instance of it and create wrapper functions for all functions that call the corresponding method on the global Gosmi instance.

GoSNMP handles this similarly, except for the compat wrapper functions which they don't have. They also have a global default instance, but allow the user to create more instances.

Am I missing the obvious reason why this wasn't already done or can't be achieved?

sleepinggenius2 commented 1 month ago

What I believe you are asking for is already present in the library. It follows this libsmi functionality:

"The smiInit() function can also be used to support multiple sets of MIB data. In this case, the tag argument may be appended by a colon and a name to differentiate the data sets. Any library function call subsequent to an smiInit("tag:dataset") call is using the specified data set."

The only difference is that calling Init on the wrapper currently reloads the config each time, which probably wouldn't be ideal if you are switching between handles frequently. The state is fully captured within a handle, so just exposing those in a different way might be sufficient for your use case, but was not functionality originally present in libsmi and thus is not currently implemented.

The goal was always to implement a richer API, while maintaining backwards compatibility with the original libsmi semantics, but I unfortunately do not have the time to work on anything like that at the moment.

stampflit commented 1 month ago

Thanks for your quick reply. I missed that functionality entirely, It also didn't come to my mind, that I could just look at the libsmi documentation. Thanks for pointing me to the smiInit function.

I had a quick glance at it. And while I think this functionality might satisfy my needs (or only require minor adjustments), I also think some larger changes are required for it to be properly usable with multiple data sets.

The goal was always to implement a richer API, while maintaining backwards compatibility with the original libsmi semantics, but I unfortunately do not have the time to work on anything like that at the moment.

I fully understand that. In the future I'll maybe have a shot at implementing this, but for now, this isn't a priority for me.

Also I'd suggest to put some links to the libsmi docs (e.g https://www.ibr.cs.tu-bs.de/projects/libsmi/smi_config.html) into the readme. I was annoyed by the lack of documentation in this repo and overlooked that this reimplementation follows libsmi so closely, that it's docs are useful here as well.