Opened 14 years ago

Closed 13 years ago

#2902 defect closed fixed (fixed)

bad flag value in twisted.conch.ssh.filetransfer

Reported by: mmoskwa Owned by:
Priority: highest Milestone:
Component: conch Keywords:
Cc: Branch: branches/acmodtime-sftp-2902
branch-diff, diff-cov, branch-cov, buildbot
Author: mwh, exarkun

Description

flag FILEXFER_ATTR_ACMODTIME is set to 0x09 and it should be 0x08. Calling setAttrs(path, attrs) when attrs contains keys for 'atime' and 'mtime' causes program to hang indefinitely.

Attachments (3)

blargh.diff (52.8 KB) - added by Michael Hudson-Doyle 13 years ago.
blargh.2.diff (5.6 KB) - added by Michael Hudson-Doyle 13 years ago.
blargh.3.diff (6.4 KB) - added by Michael Hudson-Doyle 13 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 14 years ago by mmoskwa

Component: coreconch
Owner: changed from Glyph to z3p

comment:2 Changed 13 years ago by Jean-Paul Calderone

#3512 was a duplicate of this

Changed 13 years ago by Michael Hudson-Doyle

Attachment: blargh.diff added

comment:3 Changed 13 years ago by Michael Hudson-Doyle

Keywords: review added
Priority: normalhighest

The attached patch fixes this, complete with rather ridiculous test.

comment:4 Changed 13 years ago by Michael Hudson-Doyle

Oh, I should say that I know the test has style issues. If the approach seems acceptable, I can clean those up.

comment:5 Changed 13 years ago by Michael Hudson-Doyle

Owner: z3p deleted

comment:6 Changed 13 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Michael Hudson-Doyle

I guess since only about 40 lines of the draft spec are being used, just those lines (that is, the #defines on page 5) should be included in the test with a link to the whole draft and a brief comment about why -02 is being used instead of a more recent draft.

comment:7 Changed 13 years ago by Michael Hudson-Doyle

Keywords: review added

Like this?

Changed 13 years ago by Michael Hudson-Doyle

Attachment: blargh.2.diff added

comment:8 Changed 13 years ago by Michael Hudson-Doyle

And, in all seriousness, it is taking a ridiculous amount of effort to fix a one-character typo. Something is broken in the Twisted world (maybe it's just the fact that conch should really be distributed separately from Twisted).

comment:9 Changed 13 years ago by Michael Hudson-Doyle

Owner: Michael Hudson-Doyle deleted

comment:10 Changed 13 years ago by Jean-Paul Calderone

Sorry this is so hard. If the goal is just to fix the issue ASAP, I would have tried for a more conventional testing approach. I mentioned this on the channel, but maybe I should have included it in a comment on the ticket as well.

comment:11 Changed 13 years ago by Glyph

Keywords: review removed
Owner: set to Michael Hudson-Doyle
  1. The sibpath import added in this patch is no longer necessary, please remove it.
  2. filexfer_spec_excerpts and test_constants_against_spec are both coding-standard violations. They should be filexferSpecExcerpts and test_constantsAgainstSpec respectively.
  3. Both the test case class and method need docstrings. I suggest moving filexferSpecExcerpts an attribute of the test case, and moving the comment describing it into an @ivar in the class docstring. The test method can simply say that it verifies the filetransfer constants against a pasted excerpt of the spec.

Given that none of this is functional feedback, please apply the changes to the patch and feel free to commit it.

Changed 13 years ago by Michael Hudson-Doyle

Attachment: blargh.3.diff added

comment:12 Changed 13 years ago by Michael Hudson-Doyle

Keywords: review added
Owner: Michael Hudson-Doyle deleted

comment:13 Changed 13 years ago by Glyph

Keywords: review removed
Owner: set to Glyph
Status: newassigned

comment:14 Changed 13 years ago by Glyph

Owner: changed from Glyph to Michael Hudson-Doyle
Status: assignednew

Now that you have commit, you can merge it yourself... :)

comment:15 Changed 13 years ago by Michael Hudson-Doyle

Resolution: fixed
Status: newclosed

(In [25218]) Merge my patch: Fix the value of FILEXFER_ATTR_ACMODTIME in the sftp implementation.

Fix the value of FILEXFER_ATTR_ACMODTIME in twisted.conch.ssh.filetransfer, and add tests that parse chunks of the relevant draft standard to make sure all the constants are correct.

Author: mwh Reviewer: glyph Fixes: #2902

comment:16 Changed 13 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

(In [25219]) Revert 25218 - test suite regression

Reopens #2902

The newly added test fails on Python 2.3:

===============================================================================
[FAIL]: twisted.conch.test.test_filetransfer.TestConstants.test_constantsAgainstSpec

Traceback (most recent call last):
  File "/srv/bb-slave/Run/slave/full2.3/Twisted/twisted/conch/test/test_filetransfer.py", line 676, in test_constantsAgainstSpec
    self.assertEqual(v, getattr(filetransfer, k))
twisted.trial.unittest.FailTest: not equal:
a = -2147483648
b = 2147483648L

-------------------------------------------------------------------------------

This appears to be due to the behavior of int("0x80000000", 0) on Python 2.3 (which differs from the behavior on later versions of Python).

comment:17 Changed 13 years ago by Jean-Paul Calderone

Author: , exarkun
Branch: branches/acmodtime-sftp-2902

(In [25228]) Branching to 'acmodtime-sftp-2902'

comment:18 Changed 13 years ago by Jean-Paul Calderone

(In [25230]) use int instead of long to ensure a proper interpretation of the unsigned 32 bit values in the spec

refs #2902

comment:19 Changed 13 years ago by Jean-Paul Calderone

(In [25231]) use assertTrue instead of assert_ and add a message to make the test failure a bit more friendly

refs #2902

comment:20 Changed 13 years ago by Jean-Paul Calderone

Author: , exarkunmwh, exarkun
Keywords: review added
Owner: Michael Hudson-Doyle deleted
Status: reopenednew

comment:21 Changed 13 years ago by Glyph

Building again since that build had a bunch of spurious failures. On the other hand that builder seems to have some chronic problems, so maybe it won't go away. Results will be here:

http://buildbot.twistedmatrix.com/builders/winxp32-py2.4-select/builds/57

comment:22 Changed 13 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

All right, that builder seems hosed for reasons unrelated to this branch. Everything else checks out, though.

(Also, what button did you press to make the buildbot actually build that branch on all supported platforms? I'd really like to do that more in the future.)

Please merge.

comment:23 Changed 13 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [25243]) Merge acmodtime-sftp-2902

Author: mwh, exarkun Reviewer: glyph Fixes: #2902

Fix the value of FILEXFER_ATTR_ACMODTIME in the sftp implementation and add tests which parse chunks of the relevant draft standard to make sure all the constants are correct.

comment:24 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.