Curl tweaks #2428

Merged
merged 3 commits into from Apr 1, 2013

Projects

None yet

6 participants

@arnova
Member
arnova commented Mar 12, 2013

This PR fixes a minor issue in Curl Stat causing it to have a too short timeout in some cases. This is probably a remnant of when Stat() and Exists() were roughly the same function. It furthermore improves the error logging for Curl, which should make inspecting (user) logs much easier for Curl related issues.

@arnova
Member
arnova commented Mar 12, 2013

@elupus / @ulion / @koying : Let me know what you think..

@koying
Member
koying commented Mar 12, 2013

Seems fine. Did you make sure to implement on CurlFile descendant (e.g. DAXxxx), too, if needed?

@ulion
Contributor
ulion commented Mar 12, 2013

be careful the new added logs, consider the Stat/Exists is not expect it must exists, it should be not in warning level.

@arnova
Member
arnova commented Mar 16, 2013

@koying: What do you main by descendant? The webdav/ftp stuff?
@ulion: I think the logging I added should only occur when things go wrong, as far as I can tell. I think it's inline with eg. our smb filesystem now. Please correct me if I'm wrong.

@koying
Member
koying commented Mar 16, 2013

Yep

On 16 March 2013 11:43, arnova notifications@github.com wrote:

@koying https://github.com/koying: What do you main by descendant? The
webdav/ftp stuff?
@ulion https://github.com/ulion: I think the logging I added should
only occur when things go wrong, as far as I can tell. I think it's inline
with eg. our smb filesystem now. Please correct me if I'm wrong.


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2428#issuecomment-15002796
.

@arnova
Member
arnova commented Mar 16, 2013

@koying: Then we're fine since those all directly use CCurlFile...

@ulion
Contributor
ulion commented Mar 16, 2013

a http/ftp file fail to stat or exists doesn't mean an error or something wrong, e.g. when you delete a file, first check it exists, if not, nothing would produce warning or error.
so, do not log stat/exists error in warning/error loglevel.

@ulion ulion commented on the diff Mar 16, 2013
xbmc/filesystem/CurlFile.cpp
@@ -1217,7 +1228,6 @@ int CCurlFile::Stat(const CURL& url, struct __stat64* buffer)
SetCommonOptions(m_state);
SetRequestHeaders(m_state);
- g_curlInterface.easy_setopt(m_state->m_easyHandle, CURLOPT_TIMEOUT, 5);
@ulion
ulion Mar 16, 2013 Contributor

without a timeout, it may take any longer time if the server just hang us there, the timeout must existed for some reason, totally remove it looks not reasonable.

@arnova
arnova Mar 16, 2013 Member

We already set a global timeout (check SetCommonOptions()), this overruled it with a shorter timeout which is bad IMO.

@ulion
ulion Mar 16, 2013 Contributor

the one set in SetCommonOptions() is CURLOPT_CONNECTTIMEOUT, not CURLOPT_TIMEOUT

@arnova
Member
arnova commented Mar 16, 2013

@ulion: Please have a close look at the PR, those conditions should all be catched (and not logged). Obviously we shouldn't log warnings on non-existing files, which is the case AFAIK.

@ulion
Contributor
ulion commented Mar 16, 2013

what @elupus think?

@elupus
Member
elupus commented Mar 16, 2013

I think that timeout call should just be raised, not removed. We don't want
a hang here. If server hang, not providing data but did accept the
connection we won't time out otherwise.
On Mar 16, 2013 12:14 PM, "ulion" notifications@github.com wrote:

what @elupus https://github.com/elupus think?


Reply to this email directly or view it on GitHubhttps://github.com/xbmc/xbmc/pull/2428#issuecomment-15003125
.

@arnova
Member
arnova commented Mar 16, 2013

@elupus: Perhaps just use AdvancedSettings.m_curlconnecttimeout for that then? It defaults to 10....

@elupus
Member
elupus commented Mar 16, 2013

Yea that is fine.

@arnova
Member
arnova commented Mar 16, 2013

@elupus: And what about the log-stuff, are you fine with it being LOGWARNING as well?

@elupus
Member
elupus commented Mar 16, 2013

If it's a uncommon case yes. But couldn't really tell from the diff. Info
might be better suited.

@Memphiz
Member
Memphiz commented Mar 16, 2013

What ulion said - please don't log anything if exists failed. We had this and i have removed it some month ago from all our handlers (only smb and nfs had it iirc). It spammed the log a hell when trying to generate the thumbnail directory structure in conjunction with path substitutions. (unlikly to be the case for curl - but we should handle it consistent).

@elupus
Member
elupus commented Mar 16, 2013

From what I can tell it only logs in corner cases.

@arnova
Member
arnova commented Mar 16, 2013

@memphiz: What @Elupus says: it only logs when there are real failures, like with eg. smb, that's why there's some special casing implemented in the PR, like the 404 exclude but it's possible we need to add more in the future, we'll see. But I've extensively tested this and for the scenarios I could think of, it's fine..

@Memphiz
Member
Memphiz commented Mar 16, 2013

k

@arnova arnova merged commit 42bc248 into xbmc:master Apr 1, 2013
@mkortstiege

FYI: CurlFile.cpp:1092:79: warning: too many arguments for format [-Wformat-extra-args]

@mkortstiege

Same as above.

Member

Thanks for the heads-up, don't understand why I missed those. It least I noticed a small discrepancy between those 2 which I'll also fix.

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