#1688 closed defect (fixed)

ftpd returns 0 for all timestamps

Reported by: zooko Owned by: davidsarah
Priority: normal Milestone: 1.9.2
Component: code-frontend Version: 1.9.1
Keywords: ftpd reviewed Cc:
Launchpad Bug:

Description

It appears that the FTP server, unlike the SFTP server, always returns Jan 1 1970 for all timestamps.

Attachments (3)

fix_ftpd_mtime.dpatch (83.7 KB) - added by lebek at 2012-03-21T14:00:24Z.
fix_ftpd_size_type.dpatch (83.6 KB) - added by lebek at 2012-03-22T13:23:06Z.
fix_ftpd_mtime_with_unit_test.dpatch (88.2 KB) - added by lebek at 2012-03-31T00:08:11Z.
------WebKitFormBoundarytFYJYBMb7621074p Content-Disposition: form-data; name="replace" on

Download all attachments as: .zip

Change History (18)

Changed at 2012-03-21T14:00:24Z by lebek

comment:1 Changed at 2012-03-21T14:02:54Z by lebek

That just fixes the modified time. The underlying Twisted implementation doesn't handle create or last access times.

comment:2 Changed at 2012-03-21T15:30:08Z by zooko

lebek: thank you for the patch! It needs a unit test before we can commit it to trunk. Would you be willing to write on?

comment:3 Changed at 2012-03-21T15:30:19Z by zooko

  • Keywords test-needed added

comment:4 Changed at 2012-03-22T12:57:59Z by lebek

Eh, ignore the comment on that attachment.

Changed at 2012-03-22T13:23:06Z by lebek

comment:5 Changed at 2012-03-22T13:24:50Z by lebek

fix_ftpd_mtime.dpatch​ is obsolete, fix_ftpd_mtime_with_unit_test.dpatch​ and fix_ftpd_size_type.dpatch​ should be applied together.

Last edited at 2012-03-22T13:25:45Z by lebek (previous) (diff)

comment:6 Changed at 2012-03-27T21:33:55Z by davidsarah

  • Keywords review-needed added; test-needed removed
  • Owner set to davidsarah
  • Status changed from new to assigned

comment:7 Changed at 2012-03-27T21:34:47Z by davidsarah

  • Keywords ftpd added; ftp removed
  • Summary changed from ftpd return 0 for all timestamps to ftpd returns 0 for all timestamps

comment:8 follow-up: Changed at 2012-03-30T22:10:50Z by davidsarah

  • Keywords review-needed removed
  • Owner changed from davidsarah to lebek
  • Status changed from assigned to new

These look excellent, thanks. Comments:

  • Instead of the constant 626644800 in expected_root, use a class attribute, e.g. self.FALL_OF_THE_BERLIN_WALL.
  • "if name not in children: raise NoSuchChildError(name)" in _modifier is not covered, and not needed (it's ok to let it raise a KeyError on children[name]).
  • The case where "linkmotime" is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "value = metadata["tahoe"]["linkmotime"]" of src/allmydata/frontends/ftpd.py was not run. Of course many other lines of ftpd.py are also not tested yet, but this is a line added by the fix.)
  • It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed.
  • Add a comment above the relevant line in expected_root saying that the timestamp is expected to be 0 if no timestamp metadata is present.

[*] wiki:HowToWriteTests , which is currently being written, will describe how to generate test coverage reports.

Changed at 2012-03-31T00:08:11Z by lebek


Content-Disposition: form-data; name="replace"

on

comment:9 in reply to: ↑ 8 Changed at 2012-03-31T00:10:18Z by lebek

Thanks for the review davidsarah.

Replying to davidsarah:

  • Instead of the constant 626644800 in expected_root, use a class attribute, e.g. self.FALL_OF_THE_BERLIN_WALL.

Done, also added a self.TURN_OF_MILLENIUM in order to differentiate the output in the case where both mtime and linkmotime are set.

  • "if name not in children: raise NoSuchChildError(name)" in _modifier is not covered, and not needed (it's ok to let it raise a KeyError on children[name]).

Agreed.

  • The case where "linkmotime" is in the metadata is not tested. (I saw this by looking at the test coverage [*] and seeing that the relevant line "value = metadata["tahoe"]["linkmotime"]" of src/allmydata/frontends/ftpd.py was not run. Of course many other lines of ftpd.py are also not tested yet, but this is a line added by the fix.)

Fixed.

  • It is not tested that a file added via FTP (rather than by the internal Python API) is created with the right timestamp. This ticket can be closed without that test as long as it is documented on #512 that it is needed.

I have added a note to #512.

  • Add a comment above the relevant line in expected_root saying that the timestamp is expected to be 0 if no timestamp metadata is present.

Done.

comment:10 Changed at 2012-03-31T00:46:08Z by davidsarah

  • Keywords reviewed added
  • Milestone changed from undecided to 1.9.2
  • Priority changed from minor to normal

Excellent! Good tests.

comment:11 Changed at 2012-03-31T00:46:33Z by davidsarah

  • Owner changed from lebek to davidsarah
  • Status changed from new to assigned

comment:12 Changed at 2012-03-31T01:23:35Z by davidsarah

Note that fix_ftpd_size_type.dpatch is equivalent to warner's patch on #680, which we dithered about applying for three years because of an incompatibility with one FTP client, curlftpfs because we misunderstood a different bug exposed by the fix. We eventually decided to apply it, but then missed the 1.9.0 deadline.

Last edited at 2012-03-31T02:09:50Z by davidsarah (previous) (diff)

comment:13 Changed at 2012-03-31T02:10:27Z by davidsarah

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:15 Changed at 2012-03-31T02:24:39Z by davidsarah

with a pyflakes fix in 74cfa65f0d35ea12.

Note: See TracTickets for help on using tickets.