classification
Title: Restart support in binary upload for ftplib
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alejolp, facundobatista, giampaolo.rodola, gregory.p.smith, pablomouzo, pitrou
Priority: normal Keywords: patch

Created on 2009-09-05 18:00 by pablomouzo, last changed 2009-11-27 13:25 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
issue6845-trunk.diff pablomouzo, 2009-11-26 23:43
issue6845-py3k.diff pablomouzo, 2009-11-26 23:43
Messages (13)
msg92287 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-09-05 18:00
FPT class doesn't support the rest parameter in storbinary method.
msg92290 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-09-05 18:42
I attached a patch.
msg92503 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-09-11 02:43
I'm changing the title to something clearer (I hope).
The patch I submitted adds the retr optional parameter to the storbinary
method.
msg92970 - (view) Author: Facundo Batista (facundobatista) * (Python committer) Date: 2009-09-22 02:20
I like this. I'd love to see a test of this, though.

Pablo, do you think you could came up with a test? Thanks!
msg93469 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-10-03 01:28
I attached some tests.
msg94286 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-20 15:20
According to the RFC, the argument to REST can be any string of
printable characters. However, does it happen for clients to put
non-digits in there?
(that is, are there any implementations where the argument to REST is
something else than the byte offset from the start of file?)
msg94292 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-10-20 16:50
I think that a non-digit REST argument is an error. But I've tested some
ftp servers and they don't return an error code, they just set REST to 0
and return some OK code. Finally, ftplib doesn't check that, so if
someone accidentally passes a non-digit argument it could be a difficult
bug to spot.
msg94294 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2009-10-20 17:58
The patch looks good to me.
Only two little remarks:

1: I'd create two new separate tests rather than appending them to
existent ones.

2: > self.client.retrbinary('retr', received.append, rest=str(rest))

str() should be useless here.

> According to the RFC, the argument to REST can be any string of
> printable characters. However, does it happen for clients to put
> non-digits in there?

It shouldn't happen but in any case I woulnd't want ftplib to check for
such a kind of thing.
Deciding whether the REST argument is invalid is up to the server, in
which case it will send a 4xx/5xx error response.

> But I've tested some ftp servers and they don't return an
> error code, they just set REST to 0 and return some OK code.

IMHO a bad design choice.
msg94301 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-10-20 21:23
> I think that a non-digit REST argument is an error. But I've tested some
> ftp servers and they don't return an error code, they just set REST to 0
> and return some OK code. Finally, ftplib doesn't check that, so if
> someone accidentally passes a non-digit argument it could be a difficult
> bug to spot.

What I mean is that integer arguments should be accepted as well, since
that's what is logical to produce.
msg95100 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-11-10 02:19
Here is a new patch. It works with both ints and strings.

I'm working on a patch for py3k.
msg95754 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-26 22:58
Now that FTP_TLS has been integrated into the trunk, the patch should be
augmented in order to also cover that class. Right now a test is failing:

======================================================================
ERROR: test_storbinary_rest (test.test_ftplib.TestTLS_FTPClassMixin)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/antoine/cpython/__svn__/Lib/test/test_ftplib.py", line
485, in test_storbinary_rest
    self.client.storbinary('stor', f, rest=r)
TypeError: storbinary() got an unexpected keyword argument 'rest'

Apart from that, the patch looks ok.
msg95756 - (view) Author: Pablo Mouzo (pablomouzo) Date: 2009-11-26 23:43
Thanks Antoine, I'm attaching both patches. Now the test isn't failing.
msg95765 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2009-11-27 13:25
The patch has been committed in r76546 and r76547. Thank you for your
contribution!
History
Date User Action Args
2009-11-27 13:25:21pitrousetstatus: open -> closed
resolution: fixed
messages: + msg95765
2009-11-26 23:43:50pablomouzosetfiles: + issue6845-py3k.diff
2009-11-26 23:43:25pablomouzosetfiles: + issue6845-trunk.diff

messages: + msg95756
2009-11-26 23:40:19pablomouzosetfiles: - issue6845-trunk.diff
2009-11-26 22:58:19pitrousetmessages: + msg95754
2009-11-10 02:19:56pablomouzosetfiles: - issue6845.diff
2009-11-10 02:19:39pablomouzosetfiles: + issue6845-trunk.diff

messages: + msg95100
2009-10-20 21:23:42pitrousetmessages: + msg94301
2009-10-20 17:59:00giampaolo.rodolasetmessages: + msg94294
2009-10-20 16:50:39pablomouzosetmessages: + msg94292
2009-10-20 15:20:15pitrousetmessages: + msg94286
2009-10-20 14:53:37pitrousetnosy: + giampaolo.rodola
2009-10-08 16:52:18pitrousetpriority: normal
nosy: facundobatista, gregory.p.smith, pitrou, alejolp, pablomouzo
versions: + Python 3.2
components: + Library (Lib), - None
stage: patch review
2009-10-08 16:52:03pitrousetnosy: + pitrou
2009-10-08 16:25:37pablomouzosetnosy: + gregory.p.smith
2009-10-07 02:09:24pablomouzosetfiles: - issue6845.patch
2009-10-03 01:28:02pablomouzosetfiles: + issue6845.diff

messages: + msg93469
2009-09-22 02:20:36facundobatistasetmessages: + msg92970
2009-09-22 02:03:09alejolpsetnosy: + alejolp
2009-09-11 02:43:48pablomouzosetfiles: + issue6845.patch

messages: + msg92503
title: ftplib rest support for storbinary -> Restart support in binary upload for ftplib
2009-09-11 02:41:50pablomouzosetfiles: - issue6845.patch
2009-09-06 22:28:21pablomouzosetnosy: + facundobatista
2009-09-05 18:42:35pablomouzosetfiles: + issue6845.patch
keywords: + patch
messages: + msg92290
2009-09-05 18:00:44pablomouzocreate