Win32: fix path slashes #3374

Closed
wants to merge 27 commits into
from

Conversation

Projects
None yet
7 participants
Member

Karlson2k commented Oct 4, 2013

Allow to correctly process libbluray-generated filenames with several back- and forwardslashes.

Also contain some removal of CStdString.

This PR was done thanks to original @koying idea (original PR #3267).

Member

Karlson2k commented Oct 4, 2013

jenkins build this please

xbmc/filesystem/HDFile.cpp
{
// file://drive[:]/path
// file:///drive:/path
- CStdString host( url.GetHostName() );
+ std::string host(url.GetHostName());
if(host.size() > 0)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

No big deal, but mayaswell fix it to !host.empty()

@Karlson2k

Karlson2k Oct 4, 2013

Member

Missed it. Will fix.

xbmc/filesystem/windows/WINFileSMB.h
@@ -58,7 +58,7 @@ class CWINFileSMB : public IFile
virtual int IoControl(EIoControl request, void* param);
protected:
- CStdString GetLocal(const CURL &url); /* crate a properly format path from an url */
+ std::string GetLocal(const CURL &url); /* crate a properly format path from an url */
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

mayaswell fix the typo "create" - same above.

xbmc/filesystem/windows/WINSMBDirectory.h
- virtual bool GetDirectory(const CStdString& strPath, CFileItemList &items);
+ virtual bool GetDirectory(const CStdString& strPath, CFileItemList &items)
+ { return GetDirectory((std::string)strPath, items); }
+ virtual bool GetDirectory(const std::string& strPath, CFileItemList &items);
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

IMO remove this - it will just make things less clear once all of IDirectory subclasses have CStdString removed.

xbmc/utils/URIUtils.cpp
- if (strFile.Left(8).CompareNoCase("iso9660:") == 0)
- return true;
+ if (str.length() >= 2 && str.compare(1, 1, ":", 1) == 0)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

str.length check not required, and surely you want str[1] == ':' ?

@Karlson2k

Karlson2k Oct 4, 2013

Member

Actually, required. Got exception thrown when str is empty.
MS string implementation checks offset > size.
EDIT: it's C++ standard,

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

You already return false if str.length() < 2 above.

@Karlson2k

Karlson2k Oct 4, 2013

Member

Right. And after that I can erase first 4 chars.

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

Ah, yup - missed that. You want str[1] == ':' though I think?

@Karlson2k

Karlson2k Oct 4, 2013

Member

Thanks, this is exactly what I want. :)

- if (strFile.Left(5).CompareNoCase("cdda:") == 0)
+ if (str.compare(0, 4, "dvd:", 4) == 0 || str.compare(0, 4, "udf:", 4) == 0 ||
+ str.compare(0, 8, "iso9660:", 8) == 0 || str.compare(0, 5, "cdda:", 5) == 0)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

These can all be StringUtils::StartsWith("dvd:") etc.

return true;
#if defined(TARGET_WINDOWS)
- if (StringUtils::StartsWithNoCase(strFile, "dvd://"))
+ if (strFileLow.compare(0, 5, "dvd://", 5) == 0 || strFileLow.compare(0, 5, "dvd:\\\\", 5) == 0)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

StringUtils::StartsWith

+
+ if (strFileLow.compare(1, std::string::npos, ":\\", 2) != 0
+ && strFileLow.compare(1, std::string::npos, ":/", 2) != 0
+ && strFileLow.compare(1, std::string::npos, ":", 1) != 0)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

Just use StringUtils::EndsWith()

@Karlson2k

Karlson2k Oct 4, 2013

Member

Can't. It should be at offset 1.

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

You check the length already, no? Or are you eliminating the ":" path here?

@Karlson2k

Karlson2k Oct 5, 2013

Member

If strFile isn't "x:", "x:" or "x:/" than this is not DVD :)

@jmarshallnz

jmarshallnz Oct 5, 2013

Member

Let's leave it at this then - it's readable enough.

@t-nelson

t-nelson Oct 5, 2013

Contributor

Just the last test should be enough, no?

@jmarshallnz

jmarshallnz Oct 5, 2013

Member

_: is not a valid path. Nor is a:x.

@Karlson2k

Karlson2k Oct 6, 2013

Member

Right. Last test will match if the whole string is equal to "x:", without any suffixes.
Cases like "#:" or "(:/" will be handled by win32 GetDriveType.

return true;
#else
+ if (strFileLow.length() < 6 || strFileLow.length() > 10)
+ return false;
+
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

I really don't think this is worth doing.

@Karlson2k

Karlson2k Oct 4, 2013

Member

Discovered this while debugging. This functions called often, usually with long names. Size check will be much quicker.

@@ -953,7 +966,7 @@ void URIUtils::RemoveSlashAtEnd(CStdString& strFolder)
}
while (HasSlashAtEnd(strFolder))
- strFolder.Delete(strFolder.size() - 1);
+ strFolder.erase(strFolder.size()-1, 1);
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

strFolder.pop_back() ?

@Karlson2k

Karlson2k Oct 4, 2013

Member

It's C++11, available only with MSVC (GCC in C++11 mode can't compile XBMC)

xbmc/win32/WIN32Util.cpp
- return strRetPath;
+ if (strPath.length() > 2 && strPath.compare(0, 2, "\\\\", 2) == 0 && strPath.compare(2, 2, "?\\", 2) != 0)
+ return "smb://" + strPath.substr(2);
+ return strPath;
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

The returned path is likely to contain mixed slashes. Why?

@Karlson2k

Karlson2k Oct 4, 2013

Member

It's in commit comment.
We don't care about it here. It's handled in File/Directory class anyway. No need to do it twice.

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

You KNOW you're return a path with invalid stuff here, and it's almost certain that someone will go and use this function somewhere else, assuming that it does the right thing.

@Karlson2k

Karlson2k Oct 4, 2013

Member

It's question of definitions. This path will be valid inside XBMC.
Just like path "c:/windows\system32\Recovery/" is valid on win32 (without \?\ prefix).

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

The path is only valid inside XBMC if all code that deals with them knows to expect \ in a smb:// path. The trick here is that \ is already a valid character in smb:// paths, so it's ambiguous.

It just so happens that you're probably safe doing so, due to CUtil::ValidatePath() being done in CURL::Parse().

IMO it's good code to just transform things into the correct form whenever you know the form you want. In this case you want to take a DOS path and return a smb:// one. Thus, you know you want to convert the slashes. Mayaswell get it right.

- return strRetPath;
+ if(StringUtils::StartsWithNoCase(strPath, "smb://"))
+ return "\\\\" + strPath.substr(6);
+ return strPath;
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

The return path is likely to contain mixed slashes. Why?

xbmc/win32/WIN32Util.cpp
+std::string CWIN32Util::SmartFixWin32PathSlashes(const std::string& path)
+{
+ size_t startReplaceFrom;
+ if (path.compare(0, 4, "\\\\?\\", 4) == 0)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

StringUtils::StartsWith

@Karlson2k

Karlson2k Oct 4, 2013

Member

Right. It's equal. But this way is faster.
Almost same readability. :)

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

It is faster, I agree (mostly because you've manually computed the strlen). It's easier to screw up though (I'm I supposed to count all those slashes to figure out that the length is 4 :p)

@Karlson2k

Karlson2k Oct 4, 2013

Member

This is why I think that form with explicit size is clearer. :)
And each function call cost a lot. With this short strings is much better to have minimal number of calls (as each call will produce relatively huge overhead).

xbmc/win32/WIN32Util.cpp
+ return result;
+}
+
+std::wstring CWIN32Util::ConverLocalPathToWin32Form(const std::string& pathUtf8)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

Typo. Convert is spelt with a t.

@Karlson2k

Karlson2k Oct 4, 2013

Member

Ooops. Thanks!

@wsoltys

wsoltys Oct 4, 2013

Member

I would have expect this method to do local and unc paths rather than doing unc manually in the SMB classes.

xbmc/win32/WIN32Util.cpp
+ return SimpleFixWin32PathSlashes(path, startReplaceFrom);
+}
+
+std::string CWIN32Util::SimpleFixWin32PathSlashes(const std::string& path, size_t startPos /*= 0*/)
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

Do you ever need to call the "simple" version directly? Why not always call the "smart" version (thus eliminating the need for 2 separate functions?)

@Karlson2k

Karlson2k Oct 4, 2013

Member

"Simple" version is called from WinSMBFile/Directory

@Karlson2k

Karlson2k Oct 4, 2013

Member

They can be changed to overloads, but I'm not sure that overload is better.

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

The Smart version is only called once, so I'd either cover that in the simple version anyway (it doesn't hurt to do the check + skip) or eliminate the smart version in the one spot it's called.

@wsoltys

wsoltys Oct 4, 2013

Member

Just for my understanding: What is this method doing exactly?
Isn't it just this (without the startPos) or do I miss something?
if(URIUtils::IsDOSPath(path))
path.replace("/","");
else
path.replace(","/");

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

Yes, but it also removes duplicate slashes (my version below might be a bit more obvious?)

xbmc/win32/WIN32Util.cpp
+ do e++; while ((str[e] == '\\' || str[e] == '/')); // str is null-terminated, no need to check for buffer overrun
+ s = e; // start of next directory name
+ if (e == len)
+ break;
@jmarshallnz

jmarshallnz Oct 4, 2013

Member

All to stop the ++e, right? Seems to me like a different looping construct might yield more readable code? Either something using find_first_of/find_first_not_of, or just a per-character routine:

size_t pos = 0, len = str.size();
while (str[pos])
{
  if (str[pos] == '/' || str[pos] == '\\')
  {
    result += '\\';
    // skip extra slashes (safe due to null-termination)
    while (str[pos] == '/' || str[pos] == '\\')
      pos++;
  }
  else
    result += str[pos++];
}
@Karlson2k

Karlson2k Oct 4, 2013

Member

Right, this 'if' can be removed. It was needed in previous version of code.

@jmarshallnz

jmarshallnz Oct 4, 2013

Member

I think the check is needed in your current code, as you're avoiding the pre-increment in the while condition, which will screw up the s != e check later.

IMO my version is just as good and easier to read.

@Karlson2k

Karlson2k Oct 6, 2013

Member

@jmarshallnz
I'll implement this in your way, but with small change: while .c_str() is guaranteed to be null-terminated, operator[] at pos == length has undefined behavior. So I'll convert string to C-string at first. And processing C-array is much faster that using std::string::operator[].

Member

wsoltys commented Oct 4, 2013

While this is really nice work I would like to see that you keep things simple. The last amount of speed isn't that important over readability. Just keep that in mind please.

Member

Karlson2k commented Oct 6, 2013

After some code analysis I think that best way is to do:

  1. In CUtil::ValidatePath() process all possible path types, including smb:// and "\server\share" form on Win32. Correct slashes at this point.
  2. in CURL::Parse() process "\server\share" paths on Win32 just like "smb://" paths
  3. In all xFile/xDirectory classes do not apply additional correction (slashes), as path in always processed by CURL
  4. Remove SmbToUnc and UncToSmb function and use standard CFile interface to open, move, rename files.
Member

wsoltys commented Oct 6, 2013

Remove SmbToUnc and UncToSmb function and use standard CFile interface to open, move, rename files.

Please reconsider this. At least for winexception I used the plain windows api intentional. At this point I don't want to depend on any XBMC api.

Member

Karlson2k commented Oct 6, 2013

@wsoltys OK, no problem.
For winexpection we can use ValidatePath -> SmbToUnc. Both are only string processing function.

Member

jmarshallnz commented Oct 6, 2013

I strongly recommend not relying on CUtil::ValidatePath. Ideally we'd completely remove it from CURL, as ideally CURL(string).Get() == string. (I suspect this is wishful thinking, but at least it's something we should aspire to).

i.e. in functions where you know you're the form the URL must be in, convert it directly. In other functions, don't - we don't want paths mysteriously changing due to passing through CURL at a point we weren't expecting.

@ghost

ghost commented Oct 6, 2013

imo the constructor should be made explicit to avoid such surprises.

Member

jmarshallnz commented Oct 6, 2013

I don't think that functions that take CURL are necessarily the problem (though no reason not to make it explicit as you say), rather it's some path processing that goes via CURL and is then reconstructed.

Things like URIUtils::AddFileToFolder() can process the string via CURL, and the "folder" bit may well change due to this behaviour, even though it shouldn't need to.

Member

Karlson2k commented Oct 7, 2013

There is no problem to make CURL to simple process string as is, without any correction. In that case CURL will fill host/protocol/username etc. only when incoming string is stricly fit one of URL templates, in other cases store string as filepath. Then we get CURL(string).Get() ==string.
But we need to rewrite all file/directory classes as most of them process paths through CURL to correct it. Is it what we want?

Member

wsoltys commented Oct 7, 2013

Rome wasn't fixed in one day either ;)
Why not just create a convert method which handles unc and local path (slash stripping, conversion, etc) which is then called in the file/directory classes as we do already?
Then the bug is fixed and we have enough time to discuss further fixes (if any needed).

Member

Karlson2k commented Oct 9, 2013

jenkins build this please

Member

Karlson2k commented Oct 9, 2013

@jmarshallnz Now path is minimally modified by CURL or by ValidatePath. Slashes are fixed inside File/Directory classes.
@wsoltys ConverPathToWin32Form now works with all types of Win32 path. HDFile and Win32SMBFile works similar. UncToSmb was removed, SmbToUnc still in place.

Collaborator

wsoltys commented on f751de9 Oct 10, 2013

UNC paths are supported directly

Mind explaining that a little? Goal was that internally only smb:// paths should be used. Is this path translated later or do we again mixing unc and smb paths inside XBMC?

Owner

Karlson2k replied Oct 10, 2013

Path is translated to smb:// in CURL::Parse.
So CURL("\server\share\folder\file").Get() == "smb://server/share/folder/file"
See e9ec286

Collaborator

wsoltys commented on 1c4a2b9 Oct 10, 2013

mmh, wouldn't it better then to rename this function?

Owner

Karlson2k replied Oct 10, 2013

Than we have to rename it in many places.
undef is enough and used in safe way (ifdef - undef -endif)

Collaborator

wsoltys commented on c90c0d0 Oct 10, 2013

Wouldn't it be better to fix IsLocal() directly?

Owner

Karlson2k replied Oct 10, 2013

IsLocal is OK for itself name, just wrongly used here.

Member

wsoltys commented Oct 10, 2013

You really show great effort in fixing stuff which I appreciate therefore don't get me wrong on the following ;)
What was the pr about at first place? A bugfix. What is it now? A mixture of cosmetic changes, CStdString removal, optimisations and somewhere hidden the bugfix.
I would assume the bugfix would have been already accepted if it would be the only change. But now this pr gets bigger and bigger for every iteration.
I would suggest that you again put some thoughts on the bugfix only and split that from the rest. The first approach doesn't have to be 100% correct, 80% is enough :P

Member

Karlson2k commented Oct 10, 2013

@wsoltys I don't want to use CStdString in new code, that force me to rewrite other functions.
I can try to split this PR to several PRs, just share your thoughts - what's good and what's wrong in this PR.

+ || strProtocol2 == "hdhomerun"
+ || strProtocol2 == "rtsp"
+ || strProtocol2 == "apk"
+ || strProtocol2 == "zip")
@elupus

elupus Oct 11, 2013

Member

Try to keep the strProtocol... checks in line with the first one.

Member

elupus commented Oct 11, 2013

It's rather unreviewable due to the removal of cstdtring thou :(

Member

Karlson2k commented Oct 11, 2013

@elupus Bug fix only: #3408

@ghost ghost assigned wsoltys Oct 17, 2013

Member

wsoltys commented Nov 6, 2013

@Karlson2k can we close this in favor of #3408 or do you want to update it?

Member

Karlson2k commented Nov 6, 2013

I'd like to update it, it has more fixes

Member

wsoltys commented Dec 7, 2013

Member

Karlson2k commented Dec 7, 2013

@wsoltys Will do it after UTF-8 fixes.

Owner

MartijnKaijser commented May 21, 2014

nudge if still needed

Member

Karlson2k commented May 21, 2014

Need to review it.
OK.

@MartijnKaijser MartijnKaijser removed this from the Pending for inclusion milestone Jun 14, 2015

Owner

MartijnKaijser commented Apr 24, 2016

Let me close it for now. New PR can be opened if wanted/needed.

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