[URIUtils] Fix IsSmb, IsFTP, IsAfp, IsDAV and IsNfs.

[ace20022]

Due to protocol stacking, e.g., files in a zip on a smb share were not recognized as files on a network share.

There might be more is* methods that could benefit from analogue changes.
@Karlson2k for review

[ace20022]

jenkins build this please

[ace20022]

@Karlson2k thoughts? I'm a bit unsure if it should go in for helix since it may break some stuff

[topfs2]

It looks like a very nice patch, so I want to approve it. Not sure what the stuff which might break might be though?

[Karlson2k]

@ace20022 Constructing CURL object for each check is too costly from my point of view.
I'd start from adding overloads for CURL.

[topfs2]

Its mostly costly because of our shitty implementation of URL parsing (to support our stupid stupid urls at times :P)

Could move the CURL usage to only be called if it actually needs the CURL instance though, afaict now it seems to be created always at the top, which seems a bit unnecessary (perhaps the compiler fixes this though)

[ace20022]

Not sure what the stuff which might break might be though?

It changes the retun value of some calls... Additionally I'm not aware of any bug report. IIRC I found it while debugging and opening a zip with pictures on a smb share.

[ace20022]

Could move the CURL usage to only be called if it actually needs the CURL instance though, afaict now it seems to be created always at the top, which seems a bit unnecessary (perhaps the compiler fixes this though)

@topfs2 like that?

bool URIUtils::IsSmb(const CStdString& strFile)
{
  if (IsStack(strFile))
    return IsSmb(CStackDirectory::GetFirstStackedFile(strFile));

  if (IsSpecial(strFile))
    return IsSmb(CSpecialProtocol::TranslatePath(strFile));

  CURL url(strFile);
  if (HasParentInHostname(url))
    return IsSmb(url.GetHostName());

  return IsProtocol(strFile, "smb");
}

@ace20022 Constructing CURL object for each check is too costly from my point of view.
I'd start from adding overloads for CURL.

@Karlson2k
There're no CURL objects available on the callers side (only checked for isSMB()). So I'm not sure what you mean. The only thing that I can imagine is to simply search for "smb://" in the string, but that's too fragile I guess.

[topfs2]

IMO its our curl that's bad, we shouldn't design around that. So I think
use it if it makes sense, at some time we are bound to redactor it (and
remove the damn fs stats from it)

Agreed that we can't search for smb in the path, seems very error prone.
On 26 Oct 2014 11:49, "Andreas Zelend" notifications@github.com wrote:

Could move the CURL usage to only be called if it actually needs the CURL
instance though, afaict now it seems to be created always at the top, which
seems a bit unnecessary (perhaps the compiler fixes this though)

@topfs2 https://github.com/topfs2 like that?

bool URIUtils::IsSmb(const CStdString& strFile){
if (IsStack(strFile))
return IsSmb(CStackDirectory::GetFirstStackedFile(strFile));

if (IsSpecial(strFile))
return IsSmb(CSpecialProtocol::TranslatePath(strFile));

CURL url(strFile);
if (HasParentInHostname(url))
return IsSmb(url.GetHostName());

return IsProtocol(strFile, "smb");}

@ace20022 https://github.com/ace20022 Constructing CURL object for
each check is too costly from my point of view.
I'd start from adding overloads for CURL.

@Karlson2k https://github.com/Karlson2k
There're no CURL objects available on the callers side (only checked for
isSMB()). So I'm not sure what you mean. The only thing that I can imagine
is to simply search for "smb://" in the string, but that's too fragile I
guess.

—
Reply to this email directly or view it on GitHub
#5430 (comment).

[ace20022]

Sorry, but I can't decipher your comment. Are you okay with this if I change it the way I posted above?

[topfs2]

Haha. Sorry I tend to ramble at times :) I am fine with it. But would love
others point of view on it though.

And it might be scary for helix. But it fixed a real issue. Hmm, need to
ponder on it. But IMO go ahead and change it around if you have time and
I'll start poking people.
On 27 Oct 2014 09:21, "Andreas Zelend" notifications@github.com wrote:

Sorry, but I can't decipher your comment. Are you okay with this if I
change it the way I posted above?

—
Reply to this email directly or view it on GitHub
#5430 (comment).

[ace20022]

updated...

[Karlson2k]

@ace20022 As IsSpecial() has an additional check for IsStack(), I'd use direct check IsProtocol("special", strFile) for small optimization and to reduce chance of infinite looping with future changes.

[ace20022]

@ace20022 As IsSpecial() has an additional check for IsStack(), I'd use direct check IsProtocol("special", strFile) for small optimization and to reduce chance of infinite looping with future changes.

But when someone changes the check in IsSpecial, e.g., to return IsProtocol(strFile2, "xxx");
we have a problem. Ofc this would be very odd. btw all these hard coded strings are bad.

[ace20022]

@topfs2 I think we should postpone this pr. Please change the label if you agree

[ace20022]

ping @Karlson2k okay as is?

[ace20022]

jenkins build this please

[Karlson2k]

Removing extra URL-decoding is one of the goals for I*.
But currently you are right, it's decoded.
So it's OK to merge if you tested it.

[ace20022]

So it's OK to merge if you tested it.

mhm, I don't use such a set-up ;)

[Karlson2k]

@ace20022 Didn't you try to setup a test environment? ;)

[ace20022]

Didn't you try to setup a test environment? ;)

Nah, just found it by accident while testing bdj over smb...

[ace20022]

@MilhouseVH could you include this pr in your test builds? I guess your users have such setups.

[MilhouseVH]

OK

[MilhouseVH]

@ace20022: Needs rebasing on master after the CStdString changes...

[ace20022]

@MilhouseVH done and thx for the ping. Any reports so far?

[MilhouseVH]

Any reports so far?

None whatsoever. :)

[ace20022]

@MilhouseVH Any news? If no negative ones I'll hit the button.

[MilhouseVH]

Nothing negative reported - looks like it's safe to hit the button.

[ace20022]

Thanks a lot!