Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#4913 enhancement closed fixed (fixed)

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

Reported by: Jean-Paul Calderone Owned by: spiv
Priority: low Milestone:
Component: ftp Keywords:
Cc: itamarst Branch: branches/ftp-retr-errors-4913
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

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 Adi Roiban 5 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: itamarst added

Changed 5 years ago by Adi Roiban

Attachment: 4913-ftp-stor-err-1.diff added

comment:2 Changed 5 years ago by Adi Roiban

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 5 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 5 years ago by spiv

Author: spiv
Branch: branches/ftp-retr-errors-4913

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

comment:5 Changed 5 years ago by spiv

Author: spivadiroiban

comment:6 Changed 5 years ago by spiv

Resolution: fixed
Status: newclosed

(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 5 years ago by Adi Roiban

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.