Restart support in binary upload for ftplib

[pablomouzo]

BPO	6845
Nosy	@facundobatista, @gpshead, @pitrou, @giampaolo
Files	issue6845-trunk.diff issue6845-py3k.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2009-11-27.13:25:21.638>
created_at = <Date 2009-09-05.18:00:44.256>
labels = ['type-feature', 'library']
title = 'Restart support in binary upload for ftplib'
updated_at = <Date 2009-11-27.13:25:21.637>
user = 'https://bugs.python.org/pablomouzo'

bugs.python.org fields:

activity = <Date 2009-11-27.13:25:21.637>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2009-11-27.13:25:21.638>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2009-09-05.18:00:44.256>
creator = 'pablomouzo'
dependencies = []
files = ['15401', '15402']
hgrepos = []
issue_num = 6845
keywords = ['patch']
message_count = 13.0
messages = ['92287', '92290', '92503', '92970', '93469', '94286', '94292', '94294', '94301', '95100', '95754', '95756', '95765']
nosy_count = 6.0
nosy_names = ['facundobatista', 'gregory.p.smith', 'pitrou', 'giampaolo.rodola', 'alejolp', 'pablomouzo']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'patch review'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue6845'
versions = ['Python 2.7', 'Python 3.2']

[pablomouzo]

FPT class doesn't support the rest parameter in storbinary method.

[pablomouzo]

I attached a patch.

[pablomouzo]

I'm changing the title to something clearer (I hope).
The patch I submitted adds the retr optional parameter to the storbinary
method.

[facundobatista]

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!

[pablomouzo]

I attached some tests.

[pitrou]

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?)

[pablomouzo]

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.

[giampaolo]

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.

[pitrou]

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.

[pablomouzo]

Here is a new patch. It works with both ints and strings.

I'm working on a patch for py3k.

[pitrou]

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.

[pablomouzo]

Thanks Antoine, I'm attaching both patches. Now the test isn't failing.

[pitrou]

The patch has been committed in r76546 and r76547. Thank you for your
contribution!