Opened 4 years ago

Closed 2 years ago

Last modified 2 years ago

#4913 enhancement closed fixed (fixed)

FTP.ftp_RETR() should pass FTPCmdError errors back to the client

Reported by: exarkun Owned by: spiv
Priority: low Milestone:
Component: ftp Keywords:
Cc: itamarst Branch: branches/ftp-retr-errors-4913
(diff, github, buildbot, log)
Author: adiroiban Launchpad Bug:

Description

This is like #4909. Any error encountered while reading a file and sending it to a client, to respond to a RETR command, gets logged and a CNX_CLOSED_TXFR_ABORTED is sent back to the client. If the failure is an FTPCmdError though, it can be presented to the client to give them more information about what went wrong.

Attachments (1)

4913-ftp-stor-err-1.diff (2.1 KB) - added by adiroiban 3 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 4 years ago by DefaultCC Plugin

  • Cc itamarst added

Changed 3 years ago by adiroiban

comment:2 Changed 3 years ago by adiroiban

  • Keywords review added

I have attached a patch based on the implementation from #4909

Please let me know if it requires changes.

Cheers,
Adi

comment:3 Changed 2 years ago by spiv

  • Keywords review removed
  • Owner set to spiv

This looks acceptable to me. The code for ftp_RETR is alarmingly similar to ftp_STOR, and this leads to annoying similar tests, unsurprisingly… but that's not the fault of this patch, and in fact this patch makes the situation slightly better by making ftp_RETR more similar to ftp_STOR. So if buildbot is happy with this patch I'll land it for you.

These two methods almost certainly would be better off refactored into a common helper (perhaps involving an actual object with state rather than a bunch of nested functions) that can be unit tested thoroughly (once), but that will be a separate ticket.

comment:4 Changed 2 years ago by spiv

  • Author set to spiv
  • Branch set to branches/ftp-retr-errors-4913

(In [34782]) Branch to ftp-retr-errors-4913.

comment:5 Changed 2 years ago by spiv

  • Author changed from spiv to adiroiban

comment:6 Changed 2 years ago by spiv

  • Resolution set to fixed
  • Status changed from new to closed

(In [34785]) Merge ftp-retr-errors-4913: Pass FTPCmdErrors during FTP.ftp_RETR back to the client.

Author: adiroiban
Reviewer: spiv
Fixes: #4913

comment:7 Changed 2 years ago by adiroiban

Thanks for the review and sorry for the late reply.

I agree that there is some code repetition. I plan to fix that as part of #4180

Cheers,
Adi

Note: See TracTickets for help on using tickets.