Implement File API

src/DefaultFileSystem.cpp

 #include <AdblockPlus/DefaultFileSystem.h>

[Wladimir Palant, 2013/04/16 13:03:41]

A general concern with this API: what about non-ASCII file paths? Our strings
use UTF-8, my understanding is that it should work on most Linux systems. On
Windows we need to use the wide-char variants of the functions however.

[Felix Dahlke, 2013/04/16 14:00:09]

Which functions in particular? rename and remove below? I assumed they'd use
UTF-8 even on Windows, but I didn't test it yet.

Generally, we agreed to make libadblockplus use UTF-8 only, that means anyone
implementing FileSystem needs to work with UTF-8 paths.

According to the docs for Windows' _stat, we might get into trouble here:
"_stat automatically handles multibyte-character string arguments as
appropriate, recognizing multibyte-character sequences according to the
multibyte code page currently in use."

I'd rather tackle that once it's committed though.

[Wladimir Palant, 2013/04/16 14:10:13]

I suspect that all of cstdio and fstream maps to ASCII variants of Windows
functions - or does OEM conversion if we are lucky. I doubt that any of these
functions recognize UTF-8.

Looking into this after commit is fine with me.

 #include <cstdio>
 #include <fstream>
 #include <stdexcept>
 
 #ifndef WIN32
 #include <cerrno>
 #include <sys/stat.h>
 #endif
 
 #include "../src/Utils.h"
 
 using namespace AdblockPlus;
 
+namespace
+{
+  class RuntimeErrorWithErrno : public std::runtime_error
+  {
+  public:
+    explicit RuntimeErrorWithErrno(const std::string& message)
+      : std::runtime_error(message + " (" + strerror(errno) + ")")
+    {
+    }
+  };
+}
+
 std::tr1::shared_ptr<std::istream>
 DefaultFileSystem::Read(const std::string& path) const

[Wladimir Palant, 2013/04/16 13:03:41]

Is this expecting an absolute path? I'm not sure whether this is wise - the JS
code has no way of knowing in which directory it needs to store its files. That
directory is app/os dependent and it would probably be easiest to use relative
paths here - and the base path would be passed in to the DefaultFileSystem
constructor. Alternatively the JS code would need a way to get the base
directory (e.g. via info module).

[Felix Dahlke, 2013/04/16 14:00:09]

This supports both relative and absolute paths, I'm using relative paths in the
tests. The client can just change the working directory to get the behaviour
you'd like.

 {
-  std::ifstream* file = new std::ifstream;
-  file->open(path.c_str());

[Wladimir Palant, 2013/04/16 13:03:41]

Why not open the file in the constructor already? Same in ::Write() below.

[Felix Dahlke, 2013/04/16 14:00:09]

Done.

-  return std::tr1::shared_ptr<std::istream>(file);
+  return std::tr1::shared_ptr<std::istream>(new std::ifstream(path.c_str()));
 }
 
 void DefaultFileSystem::Write(const std::string& path,
                               std::tr1::shared_ptr<std::ostream> data)
 {
-  std::ofstream file;
-  file.open(path.c_str());
+  std::ofstream file(path.c_str());
   file << Utils::Slurp(*data);
 }
 
 void DefaultFileSystem::Move(const std::string& fromPath,
                              const std::string& toPath)
 {
   if (rename(fromPath.c_str(), toPath.c_str()))
-    throw std::runtime_error("Failed to move " + fromPath + " to " + toPath);

[Wladimir Palant, 2013/04/16 13:03:41]

How about adding strerror(errno) to the error string? Otherwise we might have a
hard time debugging what went wrong.

[Felix Dahlke, 2013/04/16 14:00:09]

Done.

+    throw RuntimeErrorWithErrno("Failed to move " + fromPath + " to " + toPath);
 }
 
 void DefaultFileSystem::Remove(const std::string& path)
 {
   if (remove(path.c_str()))
-    throw std::runtime_error("Failed to remove " + path);

[Wladimir Palant, 2013/04/16 13:03:41]

Same here, how about adding strerror(errno) to the output?

[Felix Dahlke, 2013/04/16 14:00:09]

Done.

+    throw RuntimeErrorWithErrno("Failed to remove " + path);
 }
 
 FileSystem::StatResult DefaultFileSystem::Stat(const std::string& path) const
 {
 #ifdef WIN32
-  throw std::runtime_error("Stat is not implemented on Windows");

[Wladimir Palant, 2013/04/16 13:03:41]

Why not use _stat64 on Windows? You have the right header file included already.

[Felix Dahlke, 2013/04/16 14:00:09]

I really didn't expect Windows to support POSIX stat :D Using _stat on Windows
and stat on other systems now. stat64 appears to be deprecated, at least
according to Clang. My book "Advanced Programming in the UNIX Environment"
doesn't even mention it.

+  struct _stat64 nativeStat;
+  const int failure = _stat64(path.c_str(), &nativeStat);
 #else
-  struct stat nativeStat;
-  if (stat(path.c_str(), &nativeStat)) {
+  struct stat64 nativeStat;
+  const int failure = stat64(path.c_str(), &nativeStat);
+#endif
+  if (failure) {
     if (errno == ENOENT)
       return FileSystem::StatResult();
-    throw std::runtime_error("Unable to stat " + path);
+    throw RuntimeErrorWithErrno("Unable to stat " + path);
   }
   FileSystem::StatResult result;
   result.exists = true;
   result.isFile = S_ISREG(nativeStat.st_mode);
   result.isDirectory = S_ISDIR(nativeStat.st_mode);
   result.lastModified = nativeStat.st_mtime;
   return result;
-#endif
 }