pysnmp-dev

[Pysnmp-dev] Potential oneliner bugs

[Mark M E. <mar...@ma...>]

I've run into a couple of issues that I think are bugs in the  
oneliner implementation.  I'm still learning my way around PySNMP, so  
apologies if my assumptions or conclusions are incorrect.

Issue #1:  Oneliner commands "leaking" sockets.

UdpTransportTarget.openClientMode() creates a transport via  
UdpSocketTransport().  A side effect of using the default parameters  
is that a new channel/socket is created and is registered in  
asyncore.socket_map.  The socket is never removed from  
asyncore.socket_map, it's never garbage collected and the socket is  
never closed.  The result is that instantiating and destroying large  
numbers of CommandGenerators will consume all of the systems socket  
resources.

I have a patch that works for me which is to pass a throw away  
dictionary as the sockMap.  Example:

pysnmp/entity/rfc3413/oneliner/cmdgen.py:

class UdpTransportTarget:
     ...
     def openClientMode(self):
         self.transport = udp.UdpSocketTransport(sock=None,
                                                 sockMap= 
{}).openClientMode()
         return self.transport

Issue #2:  CommandGenerator().nextCmd() does not work with V1 devices.

V1 devices terminate the list list of OIDs by returning an  
errorStatus of noSuchName(2), not by returning a value of endOfMib.   
CommandGenerator().nextCmd() considers a non-false errorStatus as an  
error (makes sense) and passes that failure to the caller.

I've hacked my local version to explicitly check for an errorStatus  
of 2, and clears that errorStatus/Index.  This seems to work, but  
there may be a cleaner solution.

pysnmp/entity/rfc3413/oneliner/cmdgen.py:

class CommandGenerator(AsynCommandGenerator):
     ...
     def nextCmd(self, authData, transportTarget, *varNames):
         def __cbFun(
             sendRequestHandle, errorIndication, errorStatus,  
errorIndex,
             varBindTable, (varBindHead, varBindTotalTable, appReturn)
             ):
             if errorStatus == 2:
                 # V1 terminates GETNEXT with an errorStatus of  
noSuchName
                 # rather than a value of endOfMib (which v2c.PDUAPI  
replaces
                 # with None).
                 errorStatus = errorStatus.clone(0)
                 errorIndex = errorIndex.clone(0)
                 pass
             if errorIndication or errorStatus:
             ...

Hopefully this information is helpful.

Regards,
Mark

Re: [Pysnmp-dev] Potential oneliner bugs

[Ilya E. <il...@gl...>]

Both issues hopefully fixed in CVS (not with your patches). Please, take a 
look.

Thanks,
ilya

On Tue, 14 Aug 2007, Mark M Evans wrote:

> I've run into a couple of issues that I think are bugs in the  
> oneliner implementation.  I'm still learning my way around PySNMP, so  
> apologies if my assumptions or conclusions are incorrect.
> 
> Issue #1:  Oneliner commands "leaking" sockets.
> 
> UdpTransportTarget.openClientMode() creates a transport via  
> UdpSocketTransport().  A side effect of using the default parameters  
> is that a new channel/socket is created and is registered in  
> asyncore.socket_map.  The socket is never removed from  
> asyncore.socket_map, it's never garbage collected and the socket is  
> never closed.  The result is that instantiating and destroying large  
> numbers of CommandGenerators will consume all of the systems socket  
> resources.
> 
> I have a patch that works for me which is to pass a throw away  
> dictionary as the sockMap.  Example:
> 
> pysnmp/entity/rfc3413/oneliner/cmdgen.py:
> 
> class UdpTransportTarget:
>      ...
>      def openClientMode(self):
>          self.transport = udp.UdpSocketTransport(sock=None,
>                                                  sockMap= 
> {}).openClientMode()
>          return self.transport
> 
> Issue #2:  CommandGenerator().nextCmd() does not work with V1 devices.
> 
> V1 devices terminate the list list of OIDs by returning an  
> errorStatus of noSuchName(2), not by returning a value of endOfMib.   
> CommandGenerator().nextCmd() considers a non-false errorStatus as an  
> error (makes sense) and passes that failure to the caller.
> 
> I've hacked my local version to explicitly check for an errorStatus  
> of 2, and clears that errorStatus/Index.  This seems to work, but  
> there may be a cleaner solution.
> 
> pysnmp/entity/rfc3413/oneliner/cmdgen.py:
> 
> class CommandGenerator(AsynCommandGenerator):
>      ...
>      def nextCmd(self, authData, transportTarget, *varNames):
>          def __cbFun(
>              sendRequestHandle, errorIndication, errorStatus,  
> errorIndex,
>              varBindTable, (varBindHead, varBindTotalTable, appReturn)
>              ):
>              if errorStatus == 2:
>                  # V1 terminates GETNEXT with an errorStatus of  
> noSuchName
>                  # rather than a value of endOfMib (which v2c.PDUAPI  
> replaces
>                  # with None).
>                  errorStatus = errorStatus.clone(0)
>                  errorIndex = errorIndex.clone(0)
>                  pass
>              if errorIndication or errorStatus:
>              ...
> 
> Hopefully this information is helpful.
> 
> Regards,
> Mark

Re: [Pysnmp-dev] Potential oneliner bugs

[Mark M E. <mar...@ma...>]

Thanks for the quick fixes (and sorry for the slow response, I've  
been swamped).  I've verified the socket leak is fixed.  The changes  
for V1 GET NEXT looks good, but I haven't been able to test it  
against real hardware yet.

On Aug 15, 2007, at 6:31 AM, Ilya Etingof wrote:

>
> Both issues hopefully fixed in CVS (not with your patches). Please,  
> take a
> look.
>
> Thanks,
> ilya
>
> On Tue, 14 Aug 2007, Mark M Evans wrote:
>
>> I've run into a couple of issues that I think are bugs in the
>> oneliner implementation.  I'm still learning my way around PySNMP, so
>> apologies if my assumptions or conclusions are incorrect.
>>
>> Issue #1:  Oneliner commands "leaking" sockets.
>>
>> UdpTransportTarget.openClientMode() creates a transport via
>> UdpSocketTransport().  A side effect of using the default parameters
>> is that a new channel/socket is created and is registered in
>> asyncore.socket_map.  The socket is never removed from
>> asyncore.socket_map, it's never garbage collected and the socket is
>> never closed.  The result is that instantiating and destroying large
>> numbers of CommandGenerators will consume all of the systems socket
>> resources.
>>
>> I have a patch that works for me which is to pass a throw away
>> dictionary as the sockMap.  Example:
>>
>> pysnmp/entity/rfc3413/oneliner/cmdgen.py:
>>
>> class UdpTransportTarget:
>>      ...
>>      def openClientMode(self):
>>          self.transport = udp.UdpSocketTransport(sock=None,
>>                                                  sockMap=
>> {}).openClientMode()
>>          return self.transport
>>
>> Issue #2:  CommandGenerator().nextCmd() does not work with V1  
>> devices.
>>
>> V1 devices terminate the list list of OIDs by returning an
>> errorStatus of noSuchName(2), not by returning a value of endOfMib.
>> CommandGenerator().nextCmd() considers a non-false errorStatus as an
>> error (makes sense) and passes that failure to the caller.
>>
>> I've hacked my local version to explicitly check for an errorStatus
>> of 2, and clears that errorStatus/Index.  This seems to work, but
>> there may be a cleaner solution.
>>
>> pysnmp/entity/rfc3413/oneliner/cmdgen.py:
>>
>> class CommandGenerator(AsynCommandGenerator):
>>      ...
>>      def nextCmd(self, authData, transportTarget, *varNames):
>>          def __cbFun(
>>              sendRequestHandle, errorIndication, errorStatus,
>> errorIndex,
>>              varBindTable, (varBindHead, varBindTotalTable,  
>> appReturn)
>>              ):
>>              if errorStatus == 2:
>>                  # V1 terminates GETNEXT with an errorStatus of
>> noSuchName
>>                  # rather than a value of endOfMib (which v2c.PDUAPI
>> replaces
>>                  # with None).
>>                  errorStatus = errorStatus.clone(0)
>>                  errorIndex = errorIndex.clone(0)
>>                  pass
>>              if errorIndication or errorStatus:
>>              ...
>>
>> Hopefully this information is helpful.
>>
>> Regards,
>> Mark

--
This signature intentionally left blank.

[Pysnmp-dev] More potential socket_map issues

[Mark M E. <mar...@ma...>]

First of all, in case I haven't mentioned it already, I'm really  
impressed by PySNMP.  I think it
is an amazing piece of work.  Also, I'm new to PySNMP and SNMP, so  
I'm still learning my
way around.

4.1.9a (aka CVS) fixes the socket leak, but it's still creating "side  
effects" in the
asyncore.socket_map.  Consider the following code:

===========================8<==============================
from pysnmp.carrier.asynsock import dispatch
from pysnmp.entity import engine
from pysnmp.entity.rfc3413.oneliner import cmdgen


security_name = 'default'
community_name = 'xxx'
address = '10.0.1.1' # My Airport Extreme Wireless router
port = 161
# iso.org.dod.internet.private.enterprises.apple.airport.baseStation3.
#     physicalInterfaces.physicalInterfacesTable.physicalInterface.
#     physicalInterfaceNumRX.1
object_name = (1, 3, 6, 1, 4, 1, 63, 501, 3, 4, 2, 1, 8, 1)

auth_data = cmdgen.CommunityData(security_name,
                                  community_name,
                                  1)
transportTarget = cmdgen.UdpTransportTarget((address, port))
snmpEngine = engine.SnmpEngine()
transportDispatcher = dispatch.AsynsockDispatcher()
transportDispatcher.setSocketMap({})
snmpEngine.registerTransportDispatcher(transportDispatcher)
errorIndication, errorStatus, errorIndex, varBinds = (
     cmdgen.CommandGenerator(snmpEngine).getCmd(
         auth_data, transportTarget,
         object_name
         )
     )
===========================>8==============================

In so far as this creates a blocking command that uses it's own  
socket map
in runDispatcher(), this works great.  But internally, the transport  
is being added initially to the
asyncore.socket_map and then it is also being added the the __sockMap  
of the
transportDispatcher.  I'm concerned about the transport ever being  
added to
asyncore.socket_map when it is being created for an  
transportDispatcher that has it's own map.
The reason that I'm concerned is that I'm trying to make sure that I  
can invoke a blocking command
in a thread without effecting some other package that is using  
asyncore in another thread (like
Medussa).  Although my example is using blocking commands, I think  
that any attempt to use a
non-default socket map will result in the same behavior.

It seems like the correct socket map is explicitly managed via  
registerTransport() and
unregisterTransport().  Therefore it seems like a  
AbstractSocketTransport should not implicitly
added to asyncore.socket_map, but rather defer that decision until  
registerTransport() is
called.

The following worked for me (similar to my original hack, but closer  
to the source of the problem):

class AbstractSocketTransport(asyncore.dispatcher):
     sockFamily = sockType = None
     retryCount = 0; retryInterval = 0
     def __init__(self, sock=None, sockMap=None):
         # @patch begin
         # The socket map is managed by the AsynsockDispatcher on  
which this
         # transport is registered.
         if sockMap is None:
             sockMap = {}
         # @patch end
         if sock is None:
             try:

The temporary socket map is immediately discarded and then the  
AbstractSocketTransport
only participates in the socket map of the AsynsockDispatcher on  
which it is registered.

Hopefully what I've said made sense.

Regards,
Mark

Re: [Pysnmp-dev] More potential socket_map issues

[Ilya E. <il...@gl...>]

Your patch has been committed into CVS. Sorry for delay.

Thanks,
ilya

[ skipped ]

> The following worked for me (similar to my original hack, but closer  
> to the source of the problem):
> 
> class AbstractSocketTransport(asyncore.dispatcher):
>      sockFamily = sockType = None
>      retryCount = 0; retryInterval = 0
>      def __init__(self, sock=None, sockMap=None):
>          # @patch begin
>          # The socket map is managed by the AsynsockDispatcher on  
> which this
>          # transport is registered.
>          if sockMap is None:
>              sockMap = {}
>          # @patch end
>          if sock is None:
>              try:
> 
> The temporary socket map is immediately discarded and then the  
> AbstractSocketTransport
> only participates in the socket map of the AsynsockDispatcher on  
> which it is registered.