Provide more information to logerror().

[GoogleCodeExporter]

What steps will reproduce the problem?
1. Trigger an error condition in the FTPHandler.
2. Observe that the traceback is passed as the msg parameter to logerror().

What is the expected output? What do you see instead?

While this information is useful, there is a lot of information that could be 
gathered from the FTPHandler instance that would be invaluable for 
troubleshooting the failure.

I propose passing the FTPHandler instance as a second (optional) parameter to 
logerror() allowing that function to gather additional details for 
troubleshooting purposes.

For example, my implementation of logerror() sends an email to administrators 
containing the traceback, it would also be useful to include the username, 
remote address/port, last command etc. in this email. The only way I see to 
collect this information is to provide the FTPHandler instance to logerror() so 
it can collect whatever details are desired.

Original issue reported on code.google.com by btimby@gmail.com on 28 Mar 2011 at 4:12

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

For consistency with the current implementation which already provides 
FTPHandler.log() and FTPHandler.logline() methods I think it makes more sense 
to add a new FTPHandler.logerror() method which you can override and do 
whatever you want in there.

This would be called with a "traceback_msg" argument for all the exceptions 
which are raised in:

- handlers (FTPHandler, TLS_FTPHandler)
- acceptors / listeners (PassiveDTP, ActiveDTP)
- data handlers (DTPHandler, ThrottledDTPHandler, TLS_DTPHandler)

The only class which would be left out is FTPServer, in which case you'll have 
to override it's handle_error() method.

Original comment by g.rodola on 28 Mar 2011 at 4:35

[GoogleCodeExporter]

Sounds like a good plan. I will attach another patch shortly.

Original comment by btimby@gmail.com on 28 Mar 2011 at 6:47

[GoogleCodeExporter]

The attached patch adds a logerror() method to the classes listed below. This 
method in turn calls the global logerror() function. The purpose is to provide 
a location to override the default error logging and a means to collect as much 
environmental information as is available.

ActiveDTP
PassiveDTP
DTPHandler
FTPHandler
FTPServer

Original comment by btimby@gmail.com on 28 Mar 2011 at 7:24

Attachments:

• logerror-methods.path

[GoogleCodeExporter]

This is a new version of the patch. In this version, the DTPHandler, PassiveDTP 
and ActiveDTP call self.cmd_channel.logerror(). This way only 
FTPServer.logerror() and FTPHandler.logerror() must be overridden.

Otherwise things get pretty nasty pretty quickly.

Original comment by btimby@gmail.com on 28 Mar 2011 at 8:33

Attachments:

• logerror-methods.path

[GoogleCodeExporter]

Committed in r848.
I avoided to provide a logerror() method for ActiveDTP, PassiveDTP and 
DTPHandler classes and delegated everything to FTPHandler.logerror() instead.
Again, I did this for consistency because we have no log() method in ActiveDTP, 
PassiveDTP and DTPHandler.

Let me know if you have objections.

Original comment by g.rodola on 1 Apr 2011 at 10:21

[GoogleCodeExporter]

Original comment by g.rodola on 4 Apr 2011 at 3:31

• Changed state: FixedInSVN

[GoogleCodeExporter]

Original comment by g.rodola on 4 Apr 2011 at 3:44

• Added labels: Milestone-0.6.1, Performance, Version-0.6.0

[GoogleCodeExporter]

r848 looks good with a few additional changes.

1. PassiveFTP.handle_error() was still calling logerror(). I think it should be 
changed to call self.cmd_channel.log_error().

2. contrib.handlers.SSLConnection.handle_error() is still calling logerror().

The problem with SSLConnection is that sometimes it is mixed with FTPHandler, 
and sometimes with DTPHandler, thus there is no consistent way to determine if 
it should call self.cmd_channel.logerror() or self.logerror().

I am not sure what is the best way to handle this. When DTPHandler and 
FTPHandler both had a logerror() method, this was no problem, but now I see two 
solutions.

a. Have SSLConnection look for a cmd_channel attribute, then a logerror 
attribute and call the appropriate logerror()...

if hasattr(self, 'cmd_channel'):
    self.cmd_channel.logerror(traceback.format_exc())
elif hasattr(self, 'logerror'):
    self.logerror(traceback.format_exc())

b. Implement handle_error() in both the TLS_FTPHandler and TLS_DTPHandler 
classes. 

I attached a patch addressing items 1 & 2. I chose method b for this patch.

Original comment by btimby@gmail.com on 21 Dec 2011 at 7:28

Attachments:

• logerror-methods.patch

[GoogleCodeExporter]

In r942 I added a FTPHandler.log_exception method which provides information 
about the instance where the exception was generated. Also, logerror now 
includes information about username, ip and port.
An unhandled exception will now be logged as such:

[giampaolo]@127.0.0.1:48827 unhandled exception in instance 
<pyftpdlib.contrib.handlers.TLS_FTPHandler object at 0x1ad0450>
Traceback (most recent call last):
  File "/usr/lib/python2.7/asyncore.py", line 83, in read
    obj.handle_read_event()
  File "build/bdist.linux-x86_64/egg/pyftpdlib/contrib/handlers.py", line 136, in handle_read_event
    super(SSLConnection, self).handle_read_event()
  File "/usr/lib/python2.7/asyncore.py", line 444, in handle_read_event
    self.handle_read()
  File "/usr/lib/python2.7/asynchat.py", line 158, in handle_read
    self.found_terminator()
  File "build/bdist.linux-x86_64/egg/pyftpdlib/ftpserver.py", line 2226, in found_terminator
    self.process_command(cmd, arg, **kwargs)
  File "build/bdist.linux-x86_64/egg/pyftpdlib/contrib/handlers.py", line 393, in process_command
    FTPHandler.process_command(self, cmd, *args, **kwargs)
  File "build/bdist.linux-x86_64/egg/pyftpdlib/ftpserver.py", line 2235, in process_command
    method(*args, **kwargs)
  File "build/bdist.linux-x86_64/egg/pyftpdlib/ftpserver.py", line 2676, in ftp_PORT
    self._make_eport(ip, port)
  File "build/bdist.linux-x86_64/egg/pyftpdlib/ftpserver.py", line 2584, in _make_eport
    1 / 0
ZeroDivisionError: integer division or modulo by zero

Original comment by g.rodola on 23 Dec 2011 at 6:43

[GoogleCodeExporter]

Original comment by g.rodola on 3 Jan 2012 at 11:40

• Added labels: Milestone-0.7.0

[GoogleCodeExporter]

0.7.0 is out. Closing this out as definitively fixed.

Original comment by g.rodola on 25 Jan 2012 at 7:24

• Changed state: Fixed