From a7152270f435d07eb0be336a66a1f7922a66df96 Mon Sep 17 00:00:00 2001 From: janwas Date: Wed, 4 May 2011 12:16:51 +0000 Subject: [PATCH] Path now goes to some trouble to prevent mixing / and \ slashes (causes trouble when hotloading, and some Windows APIs can't handle it, either) WARNING: that means stuff like Path(nativeDataPath/"art/") is forbidden and will raise errors on Windows when nativeDataPath contains \. always use /"" to add a trailing slash. never embed "/" in OsPath component strings (it's OK for VFS strings since they consistently use /). wdir_watch, CmdLineArgs: avoid mixed separators wutil: remove overzealous assertion (infinite recursion if an error arose before we create a window) refs #781 This was SVN commit r9424. --- source/lib/path.cpp | 1 + source/lib/path.h | 98 +++++++++++++++++-------- source/lib/sysdep/os/win/wdir_watch.cpp | 25 ++++--- source/lib/sysdep/os/win/wutil.cpp | 2 +- source/ps/GameSetup/CmdLineArgs.cpp | 14 +++- source/ps/GameSetup/CmdLineArgs.h | 5 +- source/ps/GameSetup/GameSetup.cpp | 4 +- source/ps/GameSetup/Paths.cpp | 22 +++--- source/ps/Replay.cpp | 6 +- 9 files changed, 113 insertions(+), 64 deletions(-) diff --git a/source/lib/path.cpp b/source/lib/path.cpp index 3e9b79ffb7..3f21b96cb3 100644 --- a/source/lib/path.cpp +++ b/source/lib/path.cpp @@ -33,6 +33,7 @@ STATUS_DEFINE(ERR, PATH_CHARACTER_ILLEGAL, L"illegal path character", -1); STATUS_DEFINE(ERR, PATH_CHARACTER_UNSAFE, L"unsafe path character", -1); STATUS_DEFINE(ERR, PATH_NOT_FOUND, L"path not found", -1); +STATUS_DEFINE(ERR, PATH_MIXED_SEPARATORS, L"path contains both slash and backslash separators", -1); static bool path_is_dir_sep(wchar_t c) diff --git a/source/lib/path.h b/source/lib/path.h index d0b107a8dd..ac94fe3b33 100644 --- a/source/lib/path.h +++ b/source/lib/path.h @@ -48,6 +48,7 @@ namespace ERR const Status PATH_CHARACTER_ILLEGAL = -100300; const Status PATH_CHARACTER_UNSAFE = -100301; const Status PATH_NOT_FOUND = -100302; + const Status PATH_MIXED_SEPARATORS = -100303; } /** @@ -69,18 +70,48 @@ LIB_API const wchar_t* path_name_only(const wchar_t* path); // NB: there is a need for 'generic' paths (e.g. for Trace entry / archive pathnames). -// converting via c_str would be inefficient, and the Os/VfsPath typedefs are hopefully -// sufficient to avoid errors. +// converting between specialized variants via c_str would be inefficient, and the +// Os/VfsPath typedefs are hopefully sufficient to avoid errors. class Path { public: typedef std::wstring String; - Path() {} - Path(const char* p) : path(p, p+strlen(p)) {} - Path(const wchar_t* p) : path(p, p+wcslen(p)) {} - Path(const std::string& s) : path(s.begin(), s.end()) {} - Path(const std::wstring& s) : path(s) {} + Path() + { + DetectSeparator(); + } + + Path(const char* p) + : path(p, p+strlen(p)) + { + DetectSeparator(); + } + + Path(const wchar_t* p) + : path(p, p+wcslen(p)) + { + DetectSeparator(); + } + + Path(const std::string& s) + : path(s.begin(), s.end()) + { + DetectSeparator(); + } + + Path(const std::wstring& s) + : path(s) + { + DetectSeparator(); + } + + Path& operator=(const Path& rhs) + { + path = rhs.path; + DetectSeparator(); // (warns if separators differ) + return *this; + } bool empty() const { @@ -111,38 +142,22 @@ public: { if(empty()) // (ensure length()-1 is safe) return true; // (the VFS root directory is represented as an empty string) - - // note: ideally, path strings would only contain '/' or even SYS_DIR_SEP. - // however, windows-specific code (e.g. the sound driver detection) - // uses these routines with '\\' strings. converting them all to - // '/' and then back before passing to OS functions would be annoying. - // also, the self-tests verify correct operation of such strings. - // it would be error-prone to only test the platform's separators. - // we therefore allow all separators here. - return path[path.length()-1] == '/' || path[path.length()-1] == '\\'; + return path[path.length()-1] == separator; } Path Parent() const { - size_t idxSlash = path.find_last_of('/'); + const size_t idxSlash = path.find_last_of(separator); if(idxSlash == String::npos) - { - idxSlash = path.find_last_of('\\'); - if(idxSlash == String::npos) - return L""; - } + return L""; return path.substr(0, idxSlash); } Path Filename() const { - size_t idxSlash = path.find_last_of('/'); + const size_t idxSlash = path.find_last_of(separator); if(idxSlash == String::npos) - { - idxSlash = path.find_last_of('\\'); - if(idxSlash == String::npos) - return path; - } + return path; return path.substr(idxSlash+1); } @@ -173,8 +188,13 @@ public: Path operator/(Path rhs) const { Path ret = *this; + if(ret.path.empty()) // (empty paths assume '/') + ret.separator = rhs.separator; if(!ret.IsDirectory()) - ret.path += '/'; + ret.path += ret.separator; + + if(rhs.path.find((ret.separator == '/')? '\\' : '/') != String::npos) + DEBUG_WARN_ERR(ERR::PATH_MIXED_SEPARATORS); ret.path += rhs.path; return ret; } @@ -182,7 +202,27 @@ public: static Status Validate(String::value_type c); private: + void DetectSeparator() + { + const size_t idxBackslash = path.find('\\'); + + if(path.find('/') != String::npos && idxBackslash != String::npos) + DEBUG_WARN_ERR(ERR::PATH_MIXED_SEPARATORS); + + // (default to '/' for empty strings) + separator = (idxBackslash == String::npos)? '/' : '\\'; + } + String path; + + // note: ideally, path strings would only contain '/' or even SYS_DIR_SEP. + // however, Windows-specific code (e.g. the sound driver detection) + // uses these routines with '\\' strings. the boost::filesystem approach of + // converting them all to '/' and then back via external_file_string is + // annoying and inefficient. we allow either type of separators, + // appending whichever was first encountered. when modifying the path, + // we ensure the same separator is used. + wchar_t separator; }; static inline std::wostream& operator<<(std::wostream& s, const Path& path) diff --git a/source/lib/sysdep/os/win/wdir_watch.cpp b/source/lib/sysdep/os/win/wdir_watch.cpp index 7093c45a66..9c3f30cafd 100644 --- a/source/lib/sysdep/os/win/wdir_watch.cpp +++ b/source/lib/sysdep/os/win/wdir_watch.cpp @@ -111,6 +111,7 @@ public: WARN_IF_FALSE(CloseHandle(m_ovl->hEvent)); if(ret == WAIT_OBJECT_0 || GetLastError() == ERROR_OPERATION_ABORTED) { + SetLastError(0); delete[] m_data; free(m_ovl); } @@ -130,14 +131,12 @@ public: return m_path; } - /** - * (this is the handle to be associated with the completion port) - **/ - HANDLE GetDirHandle() const + void AttachTo(HANDLE hIOCP) const { - return m_dirHandle; + AttachToCompletionPort(m_dirHandle, hIOCP, (uintptr_t)this); } + // (called again after each notification, so it mustn't AttachToCompletionPort) Status Issue() { if(m_dirHandle == INVALID_HANDLE_VALUE) @@ -150,8 +149,7 @@ public: // not set: FILE_NOTIFY_CHANGE_ATTRIBUTES, FILE_NOTIFY_CHANGE_LAST_ACCESS, FILE_NOTIFY_CHANGE_SECURITY DWORD undefined = 0; // (non-NULL pointer avoids BoundsChecker warning) m_ovl->Internal = 0; - const BOOL ok = ReadDirectoryChangesW(m_dirHandle, m_data, dataSize, watchSubtree, filter, &undefined, m_ovl, 0); - WARN_IF_FALSE(ok); + WARN_IF_FALSE(ReadDirectoryChangesW(m_dirHandle, m_data, dataSize, watchSubtree, filter, &undefined, m_ovl, 0)); return INFO::OK; } @@ -163,10 +161,13 @@ public: const FILE_NOTIFY_INFORMATION* fni = (const FILE_NOTIFY_INFORMATION*)m_data; for(;;) { - // convert name from BSTR (non-zero-terminated) to OsPath + // convert (non-zero-terminated) BSTR to Path::String cassert(sizeof(wchar_t) == sizeof(WCHAR)); - const size_t nameChars = fni->FileNameLength / sizeof(WCHAR); - const OsPath name(Path::String(fni->FileName, nameChars)); + const size_t length = fni->FileNameLength / sizeof(WCHAR); + Path::String name(fni->FileName, length); + // since we watch subtrees, name may contain '\\'. OsPath forbids + // mixing directory separators, so convert them all to '/'. + std::replace(name.begin(), name.end(), '\\', '/'); const OsPath pathname = m_path / name; const DirWatchNotification::EType type = TypeFromAction(fni->Action); @@ -195,7 +196,7 @@ private: return DirWatchNotification::Changed; default: - ENSURE(0); + DEBUG_WARN_ERR(ERR::LOGIC); return DirWatchNotification::Changed; } } @@ -325,7 +326,7 @@ public: } PDirWatchRequest request(new DirWatchRequest(path)); - AttachToCompletionPort(request->GetDirHandle(), hIOCP, (uintptr_t)request.get()); + request->AttachTo(hIOCP); RETURN_STATUS_IF_ERR(request->Issue()); dirWatch.reset(new DirWatch(&m_sentinel, request)); return INFO::OK; diff --git a/source/lib/sysdep/os/win/wutil.cpp b/source/lib/sysdep/os/win/wutil.cpp index 2b1dc50035..2de69b0d26 100644 --- a/source/lib/sysdep/os/win/wutil.cpp +++ b/source/lib/sysdep/os/win/wutil.cpp @@ -508,7 +508,7 @@ HWND wutil_AppWindow() if(!hAppWindow) { WARN_IF_FALSE(EnumWindows(FindAppWindowByPid, 0)); - ENSURE(hAppWindow != 0); + // (hAppWindow may still be 0 if we haven't created a window yet) } return hAppWindow; diff --git a/source/ps/GameSetup/CmdLineArgs.cpp b/source/ps/GameSetup/CmdLineArgs.cpp index 6ec767bd3a..285c2eeae1 100644 --- a/source/ps/GameSetup/CmdLineArgs.cpp +++ b/source/ps/GameSetup/CmdLineArgs.cpp @@ -16,13 +16,21 @@ */ #include "precompiled.h" - #include "CmdLineArgs.h" +#include "lib/sysdep/filesystem.h" // wrealpath + + CmdLineArgs::CmdLineArgs(int argc, const char* argv[]) { if (argc >= 1) - m_Arg0 = argv[0]; + { + std::string arg0(argv[0]); + // avoid OsPath complaining about mixing both types of separators, + // which happens when running in the VC2010 debugger + std::replace(arg0.begin(), arg0.end(), '\\', '/'); + m_Arg0 = wrealpath(arg0); + } for (int i = 1; i < argc; ++i) { @@ -85,7 +93,7 @@ std::vector CmdLineArgs::GetMultiple(const char* name) const return values; } -CStr CmdLineArgs::GetArg0() const +OsPath CmdLineArgs::GetArg0() const { return m_Arg0; } diff --git a/source/ps/GameSetup/CmdLineArgs.h b/source/ps/GameSetup/CmdLineArgs.h index 90c4690735..042ba1987c 100644 --- a/source/ps/GameSetup/CmdLineArgs.h +++ b/source/ps/GameSetup/CmdLineArgs.h @@ -19,6 +19,7 @@ #define INCLUDED_CMDLINEARGS #include "ps/CStr.h" +#include "lib/os_path.h" class CmdLineArgs { @@ -58,12 +59,12 @@ public: * Get the value of argv[0], which is typically meant to be the name/path of * the program (but the actual value is up to whoever executed the program). */ - CStr GetArg0() const; + OsPath GetArg0() const; private: typedef std::vector > ArgsT; ArgsT m_Args; - CStr m_Arg0; + OsPath m_Arg0; }; #endif // INCLUDED_CMDLINEARGS diff --git a/source/ps/GameSetup/GameSetup.cpp b/source/ps/GameSetup/GameSetup.cpp index 68b8dac2f3..e32a4531b3 100644 --- a/source/ps/GameSetup/GameSetup.cpp +++ b/source/ps/GameSetup/GameSetup.cpp @@ -448,8 +448,8 @@ static void InitVfs(const CmdLineArgs& args) const size_t cacheSize = ChooseCacheSize(); g_VFS = CreateVfs(cacheSize); - g_VFS->Mount(L"screenshots/", paths.Data()/"screenshots/"); - const OsPath readonlyConfig = paths.RData()/"config/"; + g_VFS->Mount(L"screenshots/", paths.Data()/"screenshots"/""); + const OsPath readonlyConfig = paths.RData()/"config"/""; g_VFS->Mount(L"config/", readonlyConfig); if(readonlyConfig != paths.Config()) g_VFS->Mount(L"config/", paths.Config()); diff --git a/source/ps/GameSetup/Paths.cpp b/source/ps/GameSetup/Paths.cpp index 18b009df1c..0ad754eeea 100644 --- a/source/ps/GameSetup/Paths.cpp +++ b/source/ps/GameSetup/Paths.cpp @@ -31,9 +31,9 @@ Paths::Paths(const CmdLineArgs& args) m_root = Root(args.GetArg0()); #ifdef INSTALLED_DATADIR - m_rdata = WIDEN(STRINGIZE(INSTALLED_DATADIR)) L"/"; + m_rdata = WIDEN(STRINGIZE(INSTALLED_DATADIR))/L""; #else - m_rdata = m_root/"data/"; + m_rdata = m_root/"data"/""; #endif const char* subdirectoryName = args.Has("writableRoot")? 0 : "0ad"; @@ -42,18 +42,18 @@ Paths::Paths(const CmdLineArgs& args) if(!subdirectoryName) { m_data = m_rdata; - m_config = m_data/"config/"; - m_cache = m_data/"cache/"; - m_logs = m_root/"logs/"; + m_config = m_data/"config"/""; + m_cache = m_data/"cache"/""; + m_logs = m_root/"logs"/""; } else { #if OS_WIN const OsPath appdata = wutil_AppdataPath() / subdirectoryName/""; - m_data = appdata/"data/"; - m_config = appdata/"config/"; - m_cache = appdata/"cache/"; - m_logs = appdata/"logs/"; + m_data = appdata/"data"/""; + m_config = appdata/"config"/""; + m_cache = appdata/"cache"/""; + m_logs = appdata/"logs"/""; #else const char* envHome = getenv("HOME"); ENSURE(envHome); @@ -63,8 +63,8 @@ Paths::Paths(const CmdLineArgs& args) const OsPath xdgCache = XDG_Path("XDG_CACHE_HOME", home, home/".cache/" ) / subdirectoryName; m_data = xdgData/""; m_cache = xdgCache/""; - m_config = xdgConfig/"config/"; - m_logs = xdgConfig/"logs/"; + m_config = xdgConfig/"config"/""; + m_logs = xdgConfig/"logs"/""; #endif } } diff --git a/source/ps/Replay.cpp b/source/ps/Replay.cpp index b054c6875a..afb070661c 100644 --- a/source/ps/Replay.cpp +++ b/source/ps/Replay.cpp @@ -58,15 +58,13 @@ CReplayLogger::CReplayLogger(ScriptInterface& scriptInterface) : // to avoid accidentally overwriting earlier logs. std::wstringstream name; - name << L"sim_log/" << getpid(); + name << getpid(); static int run = -1; if (++run) name << "-" << run; - name << L"/commands.txt"; - - OsPath path = psLogDir() / name.str(); + OsPath path = psLogDir() / L"sim_log" / name.str() / L"commands.txt"; CreateDirectories(path.Parent(), 0700); m_Stream = new std::ofstream(OsString(path).c_str(), std::ofstream::out | std::ofstream::trunc); }