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

Wrong value used for error handling in snmp module #81

Open
stveit opened this issue Oct 6, 2023 · 3 comments · Fixed by #116
Open

Wrong value used for error handling in snmp module #81

stveit opened this issue Oct 6, 2023 · 3 comments · Fixed by #116

Comments

@stveit
Copy link
Contributor

stveit commented Oct 6, 2023

Currently the query used for a snmp command is put directly into _handle_errors like so:

 query = self._oid_to_object_type(*oid)
        try:
            error_indication, error_status, error_index, var_binds = await getCmd(
                _get_engine(),
                self.community_data,
                self.udp_transport_target,
                ContextData(),
                query,
            )
        except MibNotFoundError as error:
            _log.error("%s: %s", self.device.name, error)
            return
        if self._handle_errors(error_indication, error_status, error_index, query):
            return

According to the getCmd documentation, the errorIndex specifically refers to the varBinds return value:

    Yields
    ------
    errorIndication : str
        True value indicates SNMP engine error.
    errorStatus : str
        True value indicates SNMP PDU error.
    errorIndex : int
        Non-zero value refers to `varBinds[errorIndex-1]`
    varBinds : tuple
        A sequence of :py:class:`~pysnmp.smi.rfc1902.ObjectType` class
        instances representing MIB variables returned in SNMP response.

It seems to be working to use query, as query and var_binds probably contain the exact same things most of the time.

But to avoid problems that may show up, var_binds should be used in _handle_errors instead of query

@stveit
Copy link
Contributor Author

stveit commented Nov 28, 2023

Turns out making this changed caused some problems, ref #128

Sometimes the ObjectType objects returned in varBinds contain an error message in its [0] index where you would normally find an OID. Continuing with the original way we did this would maintain the OID so it can be used for logging purposes.

Reopenin this issue so maybe we can find a better way to do this without breaking things. maybe theres a way to get both the OID and the error message, and utilize this error message for more exact logging?

Or maybe we just scrap this and continue as before since it works. pysnmp is inconsistent at best anyways..

@stveit stveit reopened this Nov 28, 2023
@lunkwill42
Copy link
Member

I'm re-uploading my comment from #128 here:

I don't think this is pysnmp being weird. This is probably just a result of how SNMP v2c works. I think I recently made some similar comments on how errors were handled / not handled in NAV during the recent SNMP v3 additions there.

SNMP v2c introduces a concept of "exceptions" being raised by an SNMP agent, and these "exceptions" are flagged as specific return value types in the varbinds of a response PDU (SNMP v1 only had the error-flag and error-index way of reporting errors back to the querying entity).

In practice, this means that if you send an SNMP request with multiple varbinds in it (i.e. you're querying multiple objects in the same request), and the responding agent encountered an error with one of those varbinds, it can still return a response that contains a value for the ok varbinds and an error for the problematic varbinds.

I don't think I have the specifics on what actually failed in your case...

@lunkwill42
Copy link
Member

I would add, however, that I would still expect the response varbind to include a reference to the failing OID from the request, where the value would be some SNMP exception type.

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

Successfully merging a pull request may close this issue.

2 participants