From f16edd2632930e3fbfead4d6d52eeac87824f1a6 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sat, 16 Nov 2013 20:46:54 +0400 Subject: [PATCH] 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 --- src/node_file.cc | 78 +++++++++++++++++++++++++++-------- test/simple/test-fs-error-messages.js | 29 ++++++++++++- 2 files changed, 89 insertions(+), 18 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 54923b1..469ccba 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -50,14 +50,22 @@ using namespace v8; class FSReqWrap: public ReqWrap { 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(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 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 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 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 --- 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'); -- 2.7.4