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

FTPFILE_NOCWD: Avoid redundant CWDs #4382

Closed
wants to merge 8 commits into from

Conversation

@Zenju
Copy link
Contributor

Zenju commented Sep 19, 2019

Hi,

here's the final integration of my changes from #4331 for FTPFILE_NOCWD when an absolute path is specified. There shouldn't be any change in behavior, only optimizations if it is determined that CWDs are not needed:

FTPFILE_NOCWD + full path:
=> preserve ftpc->prevpath

FTPFILE_NOCWD + relative path:
=> do nothing if prevpath = ""

Starting a new connection:
=> assume prevpath == "" and avoid duplicate "CWD entrypath" not just for FTPFILE_NOCWD, but in all cases.

A few observations:

  1. The code frequently makes the assumption that slashes are not urlencoded in ftp->path: path evaluation often happens before Curl_urldecode. This is not a huge problem, but something libcurl clients need to know, and makes API usage slightly more difficult (e.g. a client has to split his FTP paths by /, then url-encode the component names, then merge again before passing as CURLOPT_URL. FreeFileSync already does that, but this could be simplified.)

  2. When dealing with servers that require "OPTS UTF8" for UTF8 support (e.g. Microsoft FTP Service), this could break libcurl's CWD handling if ftpc->entrypath contains non-ASCII chars. libcurl retrieves ftpc->entrypath (via PWD) before a libcurl client had a chance to set "OPTS UTF8", and once he has, future "CWD ftpc->entrypath" will likely fail. This is not a problem anymore (due to this pull request) for FreeFileSync which is using absolute FTP paths, so libcurl never issues the previous command.

  3. There is further optimization potential when libcurl clients switch between using absolute and relative paths. E.g. first FTPFILE_SINGLECWD access might download "/home/file.txt", while the second one asks for "file2.txt", so a redundant CWD could be avoided, because both operate in "/home".

  4. The pull request is working correctly in my FTP test scenarios, but will likely break a few libcurl tests due to less "CWD"s, which I'll see to patch as well.

Best, Zenju

Zenju added 5 commits Sep 19, 2019
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Sep 19, 2019

The code frequently makes the assumption that slashes are not urlencoded in ftp->path: path evaluation often happens before Curl_urldecode

That sounds like a (separate) bug?

@Zenju

This comment has been minimized.

Copy link
Contributor Author

Zenju commented Sep 19, 2019

The code frequently makes the assumption that slashes are not urlencoded in ftp->path: path evaluation often happens before Curl_urldecode

That sounds like a (separate) bug?

Yes, seems like it. I'll work on a separate pull request once this one is sorted out, because the source code overlaps.

Zenju added 2 commits Sep 19, 2019
@bagder

This comment has been minimized.

Copy link
Member

bagder commented Sep 21, 2019

Thanks. If you get a spare minute, it'd be great to add a test case or two that verifies this optimization...

Zenju added a commit to Zenju/curl that referenced this pull request Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.