Fix CurlFile::Exists/Stat for check ftp directory with proper cwd method. #2274

Merged
merged 2 commits into from Feb 25, 2013

Conversation

Projects
None yet
3 participants
Contributor

ulion commented Feb 22, 2013

using NOCWD for checking ftp directory exists is less standard, and will just return empty list (no error) for non-existed remote directory, e.g. on xbmc's ftp server.

try this command line to known what I said:
curl -v ftp://user:pwd@ftpserveraddr/non_existed_dir/ --ftp-method nocwd

This PR fixed this issue by using singlecwd mode of curl for checking directory exists/stat.

@@ -1086,7 +1086,11 @@ bool CCurlFile::Exists(const CURL& url)
if(url2.GetProtocol() == "ftp")
{
g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_FILETIME, 1);
- g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_FTP_FILEMETHOD, CURLFTPMETHOD_NOCWD);
+ // nocwd is less standard, will return empty list for non-existed remote dir on some ftp server, avoid it.
+ if (url2.GetFileName().Right(1).Equals("/"))
@arnova

arnova Feb 22, 2013

Member

Use HasSlashAtEnd() for that. Same for the one below.

@ulion

ulion Feb 22, 2013

Contributor

it is limited in the ftp scope, so use "/" should be ok, I don't think there is ftp/http url with back-slash, and this is in CurlFile scope, all other code in CurlFile.cpp use "/", so it's ok?

@arnova

arnova Feb 22, 2013

Member

IMO the others in CurlFile should also use AddSlash/HasSlash...

@ulion

ulion Feb 22, 2013

Contributor

name a valid ftp or http url according RFC with '' as path segment separator, then I will agree with you.

@elupus

elupus Feb 22, 2013

Member

Doesn't matter. It's less code to use addslashatend/removeslashatend and it
is not performance critical.

@ulion

ulion Feb 22, 2013

Contributor

I do not agree with it. It's clearly here I only want to check whether the last char of the url path is '/', rather than ''

@elupus

elupus Feb 22, 2013

Member

Probably should have looked at the diff.. I agree.
However trailing slash is not guaranteed for directories. May not matter i
suppose.

@ulion

ulion Feb 22, 2013

Contributor

added commit to make sure ftp dir url ends with slash.

@elupus

elupus Feb 22, 2013

Member

That may work. But we don't guarantee it for our normal file manipulations.
But for this use case I think it should be fine.

Member

arnova commented Feb 22, 2013

For the rest: looks good.

Contributor

ulion commented Feb 23, 2013

will merge after 2 days, if no objection.

xbmc/filesystem/FTPDirectory.cpp
+ {
+ URIUtils::AddSlashAtEnd(file);
+ url.SetFileName(file);
+ }
@elupus

elupus Feb 23, 2013

Member

This is overly complicated. Following will do:

CStdString file = strPath;
URIUtils::AddSlashAtEnd(strPath);
CURL url(file);
@ulion

ulion Feb 23, 2013

Contributor

done.

Contributor

ulion commented Feb 24, 2013

@elupus this one also, going to merge, already updated as you suggested.

Member

elupus commented Feb 25, 2013

Go ahead.

ulion added a commit that referenced this pull request Feb 25, 2013

Merge pull request #2274 from ulion/fix_curl_exists_stat_for_ftpdir
Fix CurlFile::Exists/Stat for check ftp directory with proper cwd method.

@ulion ulion merged commit 744415c into xbmc:master Feb 25, 2013

@ulion ulion deleted the ulion:fix_curl_exists_stat_for_ftpdir branch Apr 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment