Skip to content

Commit

Permalink
fs: report correct path when EEXIST
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@indutny
indutny committed Dec 10, 2013
1 parent 4a2792c commit f16edd2
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 18 deletions.
78 changes: 61 additions & 17 deletions src/node_file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
};


Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) { \
Expand All @@ -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

Expand Down Expand Up @@ -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();
}
}
Expand All @@ -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();
}
}
Expand Down Expand Up @@ -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();
}
}
Expand Down
29 changes: 28 additions & 1 deletion test/simple/test-fs-error-messages.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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));
});
Expand All @@ -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));
});
Expand Down Expand Up @@ -134,6 +145,14 @@ try {
assert.ok(0 <= err.message.indexOf(fn));
}

try {
++expected;
fs.linkSync(existingFile, existingFile2);
} catch (err) {
errors.push('link');
assert.ok(0 <= err.message.indexOf(existingFile2));
}

try {
++expected;
fs.unlinkSync(fn);
Expand Down Expand Up @@ -174,6 +193,14 @@ try {
assert.ok(0 <= err.message.indexOf(fn));
}

try {
++expected;
fs.renameSync(existingDir, existingDir2);
} catch (err) {
errors.push('rename');
assert.ok(0 <= err.message.indexOf(existingDir2));
}

try {
++expected;
fs.readdirSync(fn);
Expand Down

0 comments on commit f16edd2

Please sign in to comment.