Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restart support in binary upload for ftplib #51094

Closed
pablomouzo mannequin opened this issue Sep 5, 2009 · 13 comments
Closed

Restart support in binary upload for ftplib #51094

pablomouzo mannequin opened this issue Sep 5, 2009 · 13 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pablomouzo
Copy link
Mannequin

pablomouzo mannequin commented Sep 5, 2009

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
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Sep 5, 2009

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

    @pablomouzo pablomouzo mannequin added the type-feature A feature request or enhancement label Sep 5, 2009
    @pablomouzo
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Sep 5, 2009

    I attached a patch.

    @pablomouzo
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Sep 11, 2009

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

    @pablomouzo pablomouzo mannequin changed the title ftplib rest support for storbinary Restart support in binary upload for ftplib Sep 11, 2009
    @facundobatista
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Oct 3, 2009

    I attached some tests.

    @pitrou pitrou added the stdlib Python modules in the Lib dir label Oct 8, 2009
    @pitrou
    Copy link
    Member

    pitrou commented Oct 20, 2009

    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
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Oct 20, 2009

    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
    Copy link
    Contributor

    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
    Copy link
    Member

    pitrou commented Oct 20, 2009

    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
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Nov 10, 2009

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

    I'm working on a patch for py3k.

    @pitrou
    Copy link
    Member

    pitrou commented Nov 26, 2009

    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
    Copy link
    Mannequin Author

    pablomouzo mannequin commented Nov 26, 2009

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

    @pitrou
    Copy link
    Member

    pitrou commented Nov 27, 2009

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

    @pitrou pitrou closed this as completed Nov 27, 2009
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants