twisted/protocols/ftp.py RESTART_MARKER_REPLY messages are incorrect

[exarkun]

The MARK reply includes two variable parameters. However, in the RESPONSE dictionary, the response string is defined as the constant '110 MARK yyyy-mmmm'. yyyy and mmmm are parameters that need to have values interpolated into them.

[adiroiban]

Looking at the rest of twisted.protocols.ftp and the whole twisted project I can not find the place where RESTART_MARKER_REPLY is used.

The restart marker is used in BLOCK MODE transfers as described by rfc959

As far as I know, twisted.protocols.ftp only support STREAM MODE and BLOCK MODE is not supported.

I guess that this code got here due to an initial import of all reponse codes defined in the RFC at 4.2.2 Numeric Order List of Reply Codes.

I have attached a diff, removing those files.

For the future, I guess that restart will be supported by twisted.protocols.ftp only in STREAM mode using REST command described in  http://tools.ietf.org/html/rfc3659#page-13

[spiv]

I think this patch is the wrong answer. Firstly, these are public names in the API, so we cannot remove them without following the deprecation policy. Secondly, at least ftp.RESTART_MARKER_REPLY seems like something that could be genuinely useful to someone building software on top of twisted.protocols.ftp — sure t.p.ftp itself doesn't use it, but users of it might.

Going back to the original ticket description, t.p.ftp.RESPONSE.RESTART_MARKER_REPLY could probably be just changed to '110 MARK %s-%s'; FTP.reply accepts *args. This would break any hypothetical code that invokes FTP.reply(RESPONSE.MARKER_REPLY) though, so to be strictly compatible we'd need to do something smarter.

[adiroiban]

I have attached a patch that does the renaming.

---

Since no part of the twisted code used RESTART_MARKER_REPLY I would rather have marked as deprecated.

It can be added back when restart block mode support will be implemented in twisted.core.

I am not aware of any FTP client using block mode restart.

I don't think that keeping FTP response codes for features not implemented in Twisted is the right thing to do.

This is not a huge code and the code that implements block mode restart can define its one RESTART_MARKER_REPLY.

Cheers, Adi

[tenth]

Thanks for taking a look a this! But there do seem to be a few problems, unfortunately:

1. According to  rfc959, the format should be "MARK yyyy = mmmm", which is different from the format being used before or after the patch. I am not an FTP expert, but maybe we should change this as well?

2. Speaking of me not being an FTP expert, there is no Test for this change; Tests are required for all changes that affect behavior, especially in this case, where the code isn't otherwise being used in Twisted itself; Third parties might be subclassing t.p.ftp and expecting this constant to contain no parameters...

3. On the same note, there's no documentation explaining how the RESPONSE dict is used, which could also lead to confusion or bugs in the future.

This might seem overly cautious, but simply changing the code with no docs or test could lead to more bugs than simply leaving it as is. (And Tests are a required part of the Twisted Development Process.)

[adiroiban]

Hi,

Many thanks for the review.

For point 1:

You are right regarding "MARK yyyy = mmmm" . In case this will be used, it requires a new format.

For point 2 and 3:

As stated in comment:6 no part of Twisted is using the RESTART_MARKER_REPLY.

In the comment my suggestion is to simple remove RESTART_MARKER_REPLY and have this ticket closed. In addition the MARK message is for block mode restarts and I don't know who is using block transfers in these days.

I don't know how to write a test when the restart functionality is not implemented in Twisted and no part of Twisted is using this this code.

---

I understand that your are overly cautious but in this case I prefer to leave this ticket as it is now and work on other tickets!

Cheers, Adi

[exarkun]

Thanks for your work on this Adi. It seems like the best thing to do for now is leave this alone. When the FTP implementation actually supports the restart feature this response code is for, then there'll be some actual behavior to test.