Skip to content

Fix for appending duplicated value for m_curlAliasList. #2263

Closed
wants to merge 1 commit into from

3 participants

@ulion
ulion commented Feb 19, 2013

it is used only in this place, but SetCommonOptions can be called multiple times.

@elupus
Team Kodi member
@ulion
ulion commented Feb 19, 2013

global search in xcode project indicate that m_curlAliasList is only used in CurlFile, and it's a protected var, no other place used it, so I added the check.

@elupus
Team Kodi member
@ulion
ulion commented Feb 20, 2013

well, then I had to wrapper the slist to make it clear, that let me feel better, how about this?

@davilla
davilla commented Feb 20, 2013

@ulion , never depend on global search in xcode project, it will not catch all possible across all platforms. See the xxx.a static libs we link to, those are built during the make xcode_depends stage.

@ulion
ulion commented Feb 20, 2013

thx, I known the xcode search is limited, and can not be relied 100%, is there some shell grep command recommend for doing this?
in this case, unless CurlFile was inherited by other class, the member var will not be referred out of the class, and we all known that. now I already removed the member var and change it into a local static var, it will make it clear. oh, a bug found in my commit, re-pushed.

@davilla
davilla commented Feb 20, 2013

grep -r xbmc
grep -r lib

@ulion ulion Use a simple wrapper to handle curl slist.
so we can make the constant list local static.
this will fix appending duplicated value for m_curlAliasList problem,
which is used only in one place, but SetCommonOptions can be called multiple times.
aeed826
@ulion
ulion commented Feb 20, 2013

If no objection, I will merge this after 2 days, will this be ok? @elupus @davilla

@elupus elupus commented on the diff Feb 20, 2013
xbmc/filesystem/CurlFile.cpp
@@ -393,13 +406,8 @@ void CCurlFile::Close()
m_cookie.Empty();
/* cleanup */
- if( m_curlAliasList )
- g_curlInterface.slist_free_all(m_curlAliasList);
@elupus
Team Kodi member
elupus added a note Feb 20, 2013

shouldn't the alias list be released here?

@elupus
Team Kodi member
elupus added a note Feb 20, 2013

Nevermind..

@ulion
ulion added a note Feb 20, 2013

It is a static local var now, will be released when xbmc exit. since it do not change, we do not need to reinit it for each curl request.

@elupus
Team Kodi member
elupus added a note Feb 20, 2013
@ulion
ulion added a note Feb 20, 2013

Static local will be first released than the global var, even earlier than static class member var, for both static local var, the release order is reversed to the init order, will this be ok?

@ulion
ulion added a note Feb 22, 2013

@elupus so is local static ok consider it's life time less than global curl instance?

@elupus
Team Kodi member
elupus added a note Feb 22, 2013
@ulion
ulion added a note Feb 22, 2013

you are right, I didn't notice the unload stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elupus elupus commented on the diff Feb 20, 2013
xbmc/filesystem/CurlFile.cpp
@@ -467,8 +475,8 @@ void CCurlFile::SetCommonOptions(CReadState* state)
g_curlInterface.easy_setopt(h, CURLOPT_FAILONERROR, 1);
// enable support for icecast / shoutcast streams
- m_curlAliasList = g_curlInterface.slist_append(m_curlAliasList, "ICY 200 OK");
- g_curlInterface.easy_setopt(h, CURLOPT_HTTP200ALIASES, m_curlAliasList);
+ static CurlSList curlAliasList("ICY 200 OK");
@elupus
Team Kodi member
elupus added a note Feb 20, 2013

Just handle the alias list the same as the other list even if it only has one now.

@ulion
ulion added a note Feb 20, 2013

The old code put them together is just because curl need to use the content of the list after setopt and during the curl perform, and the set opt and the perform is in different method of the class, so they are put as class member.
But the 2 usage of the slist is not same, one for header is non const, need be a class member var and the alias one is const, It not change anyway. local static is the best for it as I see, but if you insist, I can change it be the way like the non const, or split it to 2 step commits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ulion ulion closed this Feb 22, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.