search

gridcf / UberFTP

Interactive GridFTP client
Other
2 stars 2 forks source link

Simple fix for github issue #3 #4

Closed msalle closed 4 years ago

msalle commented 4 years ago

This is a simple and not very clean fix for gridcf/UberFTP/issues/3 Either (one of) the l_write() or the l_close() can fail if the remote file already exists, since the data goes over a different channel than the control messages. For large files, it will typically be one of the later l_write() statements, for small files probably the l_close(). Hence we effectively only have the errmsg to find out what happened, and therefore do a simple str(n)cmp with this errmsg. For this we also need to add a function to get it from the opaque errcode_t. Note that there are other FTP error codes that probably should prevent the delete. Feedback on this would be highly appreciated (-;

msalle commented 4 years ago

@msalle: Is the current state of the patches up to date with what was discussed/decided in #3?

No, this PR is still in the state where it would only NOT delete when the failure is a 550. For other failure modes it would still follow the code leading to the original deletion of the remote file. I have an additional branch https://github.com/msalle/UberFTP/tree/no_delete_file / https://github.com/msalle/UberFTP/commit/520540710382a47623656bb99271743bb553e0d0 with almost trivial change just disabling (with an #if 0 to keep the original code around) the deletion. No pull request from that one (yet). Paul's intermediate solution in https://github.com/gridcf/UberFTP/issues/3#issuecomment-633734711 would be nicer, but difficult to implement.

And do the changes work as expected during a test replicating the situation described in the original GGUS ticket?

Yes, I've extensively tested it with a locally built Uberftp from this branch and both prometheus.desy.de and gridftp.grid.sara.nl.

fscheiner commented 4 years ago

@msalle: Well, I'd go for the simple change then, though I'd just remove the relevant code, instead of masking it. I mean, it's still kept in the history, so no need to keep it around.

@matyasselmeci @ellert What would you suggest?

msalle commented 4 years ago

I'm closing this PR in favour of #5