fs: report correct path when EEXIST

When `symlink`, `link` or `rename` report EEXIST, ENOTEMPTY or EPERM -
the destination file name should be included in the error message,
instead of source file name.

fix #6510

diff --git a/src/node_file.cc b/src/node_file.cc
index 54923b1..469ccba 100644 (file)
--- a/src/node_file.cc
+++ b/src/node_file.cc
@@ -50,14 +50,22 @@ using namespace v8;
 
 class FSReqWrap: public ReqWrap<uv_fs_t> {
  public:
+  void* operator new(size_t size, char* storage) { return storage; }
+
   FSReqWrap(const char* syscall)
-    : syscall_(syscall) {
+    : syscall_(syscall),
+      dest_len_(0) {
   }
 
-  const char* syscall() { return syscall_; }
+  inline const char* syscall() const { return syscall_; }
+  inline const char* dest() const { return dest_; }
+  inline unsigned int dest_len() const { return dest_len_; }
+  inline void dest_len(unsigned int dest_len) { dest_len_ = dest_len; }
 
  private:
   const char* syscall_;
+  unsigned int dest_len_;
+  char dest_[1];
 };
 
 
@@ -102,6 +110,14 @@ static void After(uv_fs_t *req) {
       argv[0] = UVException(req->errorno,
                             NULL,
                             req_wrap->syscall());
+    } else if ((req->errorno == UV_EEXIST ||
+                req->errorno == UV_ENOTEMPTY ||
+                req->errorno == UV_EPERM) &&
+               req_wrap->dest_len() > 0) {
+      argv[0] = UVException(req->errorno,
+                            NULL,
+                            req_wrap->syscall(),
+                            req_wrap->dest());
     } else {
       argv[0] = UVException(req->errorno,
                             NULL,
@@ -212,10 +228,22 @@ struct fs_req_wrap {
 };
 
 
-#define ASYNC_CALL(func, callback, ...)                           \
-  FSReqWrap* req_wrap = new FSReqWrap(#func);                     \
-  int r = uv_fs_##func(uv_default_loop(), &req_wrap->req_,        \
-      __VA_ARGS__, After);                                        \
+#define ASYNC_DEST_CALL(func, callback, dest_path, ...)           \
+  FSReqWrap* req_wrap;                                            \
+  char* dest_str = (dest_path);                                   \
+  int dest_len = dest_str == NULL ? 0 : strlen(dest_str);         \
+  char* storage = new char[sizeof(*req_wrap) + dest_len];         \
+  req_wrap = new (storage) FSReqWrap(#func);                      \
+  req_wrap->dest_len(dest_len);                                   \
+  if (dest_str != NULL) {                                         \
+    memcpy(const_cast<char*>(req_wrap->dest()),                   \
+           dest_str,                                              \
+           dest_len + 1);                                         \
+  }                                                               \
+  int r = uv_fs_##func(uv_default_loop(),                         \
+                       &req_wrap->req_,                           \
+                       __VA_ARGS__,                               \
+                       After);                                    \
   req_wrap->object_->Set(oncomplete_sym, callback);               \
   req_wrap->Dispatched();                                         \
   if (r < 0) {                                                    \
@@ -227,13 +255,29 @@ struct fs_req_wrap {
   }                                                               \
   return scope.Close(req_wrap->object_);
 
-#define SYNC_CALL(func, path, ...)                                \
+#define ASYNC_CALL(func, callback, ...)                           \
+  ASYNC_DEST_CALL(func, callback, NULL, __VA_ARGS__)              \
+
+#define SYNC_DEST_CALL(func, path, dest, ...)                     \
   fs_req_wrap req_wrap;                                           \
-  int result = uv_fs_##func(uv_default_loop(), &req_wrap.req, __VA_ARGS__, NULL); \
+  int result = uv_fs_##func(uv_default_loop(),                    \
+                            &req_wrap.req,                        \
+                            __VA_ARGS__,                          \
+                            NULL);                                \
   if (result < 0) {                                               \
     int code = uv_last_error(uv_default_loop()).code;             \
-    return ThrowException(UVException(code, #func, "", path));    \
-  }
+    if (dest != NULL &&                                           \
+        (code == UV_EEXIST ||                                     \
+         code == UV_ENOTEMPTY ||                                  \
+         code == UV_EPERM)) {                                     \
+      return ThrowException(UVException(code, #func, "", dest));  \
+    } else {                                                      \
+      return ThrowException(UVException(code, #func, "", path));  \
+    }                                                             \
+  }                                                               \
+
+#define SYNC_CALL(func, path, ...)                                \
+  SYNC_DEST_CALL(func, path, NULL, __VA_ARGS__)                   \
 
 #define SYNC_REQ req_wrap.req
 
@@ -431,9 +475,9 @@ static Handle<Value> Symlink(const Arguments& args) {
   }
 
   if (args[3]->IsFunction()) {
-    ASYNC_CALL(symlink, args[3], *dest, *path, flags)
+    ASYNC_DEST_CALL(symlink, args[3], *dest, *dest, *path, flags)
   } else {
-    SYNC_CALL(symlink, *path, *dest, *path, flags)
+    SYNC_DEST_CALL(symlink, *path, *dest, *dest, *path, flags)
     return Undefined();
   }
 }
@@ -451,9 +495,9 @@ static Handle<Value> Link(const Arguments& args) {
   String::Utf8Value new_path(args[1]);
 
   if (args[2]->IsFunction()) {
-    ASYNC_CALL(link, args[2], *orig_path, *new_path)
+    ASYNC_DEST_CALL(link, args[2], *new_path, *orig_path, *new_path)
   } else {
-    SYNC_CALL(link, *orig_path, *orig_path, *new_path)
+    SYNC_DEST_CALL(link, *orig_path, *new_path, *orig_path, *new_path)
     return Undefined();
   }
 }
@@ -482,14 +526,14 @@ static Handle<Value> Rename(const Arguments& args) {
   if (len < 2) return TYPE_ERROR("new path required");
   if (!args[0]->IsString()) return TYPE_ERROR("old path must be a string");
   if (!args[1]->IsString()) return TYPE_ERROR("new path must be a string");
-  
+
   String::Utf8Value old_path(args[0]);
   String::Utf8Value new_path(args[1]);
 
   if (args[2]->IsFunction()) {
-    ASYNC_CALL(rename, args[2], *old_path, *new_path)
+    ASYNC_DEST_CALL(rename, args[2], *new_path, *old_path, *new_path)
   } else {
-    SYNC_CALL(rename, *old_path, *old_path, *new_path)
+    SYNC_DEST_CALL(rename, *old_path, *new_path, *old_path, *new_path)
     return Undefined();
   }
 }

diff --git a/test/simple/test-fs-error-messages.js b/test/simple/test-fs-error-messages.js
index 772966b..773faa6 100644 (file)
--- a/test/simple/test-fs-error-messages.js
+++ b/test/simple/test-fs-error-messages.js
@@ -28,7 +28,10 @@ var assert = require('assert');
 var path = require('path'),
     fs = require('fs'),
     fn = path.join(common.fixturesDir, 'non-existent'),
-    existingFile = path.join(common.fixturesDir, 'exit.js');
+    existingFile = path.join(common.fixturesDir, 'exit.js'),
+    existingFile2 = path.join(common.fixturesDir, 'create-file.js'),
+    existingDir = path.join(common.fixturesDir, 'empty'),
+    existingDir2 = path.join(common.fixturesDir, 'keys');
 
 // ASYNC_CALL
 
@@ -49,6 +52,10 @@ fs.link(fn, 'foo', function(err) {
   assert.ok(0 <= err.message.indexOf(fn));
 });
 
+fs.link(existingFile, existingFile2, function(err) {
+  assert.ok(0 <= err.message.indexOf(existingFile2));
+});
+
 fs.unlink(fn, function(err) {
   assert.ok(0 <= err.message.indexOf(fn));
 });
@@ -57,6 +64,10 @@ fs.rename(fn, 'foo', function(err) {
   assert.ok(0 <= err.message.indexOf(fn));
 });
 
+fs.rename(existingDir, existingDir2, function(err) {
+  assert.ok(0 <= err.message.indexOf(existingDir2));
+});
+
 fs.rmdir(fn, function(err) {
   assert.ok(0 <= err.message.indexOf(fn));
 });
@@ -136,6 +147,14 @@ try {
 
 try {
   ++expected;
+  fs.linkSync(existingFile, existingFile2);
+} catch (err) {
+  errors.push('link');
+  assert.ok(0 <= err.message.indexOf(existingFile2));
+}
+
+try {
+  ++expected;
   fs.unlinkSync(fn);
 } catch (err) {
   errors.push('unlink');
@@ -176,6 +195,14 @@ try {
 
 try {
   ++expected;
+  fs.renameSync(existingDir, existingDir2);
+} catch (err) {
+  errors.push('rename');
+  assert.ok(0 <= err.message.indexOf(existingDir2));
+}
+
+try {
+  ++expected;
   fs.readdirSync(fn);
 } catch (err) {
   errors.push('readdir');