From bc2c85ceef7ac034830e4a4357d0aef69cd6e386 Mon Sep 17 00:00:00 2001 From: Bert Belder Date: Sat, 31 Jan 2015 11:48:34 +0100 Subject: [PATCH] fs: improve error messages * Include a description for the error message * For rename, link, and symlink, include both the source and destination path in the error message. * Expose the destination path as the `dest` property on the error object. * Fix a bug where `ThrowUVException()` would incorrectly delegate to `Environment::TrowErrnoException()`. API impact: * Adds an extra overload for node::UVException() which takes 6 arguments. PR: https://github.com/iojs/io.js/pull/675 Fixes: https://github.com/iojs/io.js/issues/207 Closes: https://github.com/iojs/io.js/pull/293 Reviewed-by: Ben Noordhuis --- src/env-inl.h | 5 +- src/env.h | 4 +- src/node.cc | 109 +++++++++++++----------- src/node.h | 6 ++ src/node_file.cc | 34 ++------ src/node_internals.h | 3 +- test/parallel/test-domain.js | 4 +- test/parallel/test-fs-error-messages.js | 6 ++ 8 files changed, 92 insertions(+), 79 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index c5f2328e1259ea..46267ebd57dce9 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -377,9 +377,10 @@ inline void Environment::ThrowErrnoException(int errorno, inline void Environment::ThrowUVException(int errorno, const char* syscall, const char* message, - const char* path) { + const char* path, + const char* dest) { isolate()->ThrowException( - UVException(isolate(), errorno, syscall, message, path)); + UVException(isolate(), errorno, syscall, message, path, dest)); } inline v8::Local diff --git a/src/env.h b/src/env.h index 24bb0905bc3f14..8dc9c968835ab5 100644 --- a/src/env.h +++ b/src/env.h @@ -61,6 +61,7 @@ namespace node { V(cwd_string, "cwd") \ V(debug_port_string, "debugPort") \ V(debug_string, "debug") \ + V(dest_string, "dest") \ V(detached_string, "detached") \ V(dev_string, "dev") \ V(disposed_string, "_disposed") \ @@ -407,7 +408,8 @@ class Environment { inline void ThrowUVException(int errorno, const char* syscall = nullptr, const char* message = nullptr, - const char* path = nullptr); + const char* path = nullptr, + const char* dest = nullptr); // Convenience methods for contextify inline static void ThrowError(v8::Isolate* isolate, const char* errmsg); diff --git a/src/node.cc b/src/node.cc index 463b69936afd88..7c9032bf02945b 100644 --- a/src/node.cc +++ b/src/node.cc @@ -698,11 +698,10 @@ void ThrowUVException(v8::Isolate* isolate, int errorno, const char* syscall, const char* message, - const char* path) { - Environment::GetCurrent(isolate)->ThrowErrnoException(errorno, - syscall, - message, - path); + const char* path, + const char* dest) { + Environment::GetCurrent(isolate) + ->ThrowUVException(errorno, syscall, message, path, dest); } @@ -752,64 +751,78 @@ Local ErrnoException(Isolate* isolate, } -// hack alert! copy of ErrnoException, tuned for uv errors +static Local StringFromPath(Isolate* isolate, const char* path) { +#ifdef _WIN32 + if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) { + return String::Concat(FIXED_ONE_BYTE_STRING(isolate, "\\\\"), + String::NewFromUtf8(isolate, path + 8)); + } else if (strncmp(path, "\\\\?\\", 4) == 0) { + return String::NewFromUtf8(isolate, path + 4); + } +#endif + + return String::NewFromUtf8(isolate, path); +} + + +Local UVException(Isolate* isolate, + int errorno, + const char* syscall, + const char* msg, + const char* path) { + return UVException(isolate, errorno, syscall, msg, path, nullptr); +} + + Local UVException(Isolate* isolate, int errorno, - const char *syscall, - const char *msg, - const char *path) { + const char* syscall, + const char* msg, + const char* path, + const char* dest) { Environment* env = Environment::GetCurrent(isolate); if (!msg || !msg[0]) msg = uv_strerror(errorno); - Local estring = OneByteString(env->isolate(), uv_err_name(errorno)); - Local message = OneByteString(env->isolate(), msg); - Local cons1 = - String::Concat(estring, FIXED_ONE_BYTE_STRING(env->isolate(), ", ")); - Local cons2 = String::Concat(cons1, message); - - Local e; + Local js_code = OneByteString(isolate, uv_err_name(errorno)); + Local js_syscall = OneByteString(isolate, syscall); + Local js_path; + Local js_dest; - Local path_str; + Local js_msg = js_code; + js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ": ")); + js_msg = String::Concat(js_msg, OneByteString(isolate, msg)); + js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, ", ")); + js_msg = String::Concat(js_msg, js_syscall); - if (path) { -#ifdef _WIN32 - if (strncmp(path, "\\\\?\\UNC\\", 8) == 0) { - path_str = String::Concat(FIXED_ONE_BYTE_STRING(env->isolate(), "\\\\"), - String::NewFromUtf8(env->isolate(), path + 8)); - } else if (strncmp(path, "\\\\?\\", 4) == 0) { - path_str = String::NewFromUtf8(env->isolate(), path + 4); - } else { - path_str = String::NewFromUtf8(env->isolate(), path); - } -#else - path_str = String::NewFromUtf8(env->isolate(), path); -#endif + if (path != nullptr) { + js_path = StringFromPath(isolate, path); - Local cons3 = - String::Concat(cons2, FIXED_ONE_BYTE_STRING(env->isolate(), " '")); - Local cons4 = - String::Concat(cons3, path_str); - Local cons5 = - String::Concat(cons4, FIXED_ONE_BYTE_STRING(env->isolate(), "'")); - e = Exception::Error(cons5); - } else { - e = Exception::Error(cons2); + js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " '")); + js_msg = String::Concat(js_msg, js_path); + js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'")); } - Local obj = e->ToObject(env->isolate()); - // TODO(piscisaureus) errno should probably go - obj->Set(env->errno_string(), Integer::New(env->isolate(), errorno)); - obj->Set(env->code_string(), estring); + if (dest != nullptr) { + js_dest = StringFromPath(isolate, dest); - if (path != nullptr) { - obj->Set(env->path_string(), path_str); + js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, " -> '")); + js_msg = String::Concat(js_msg, js_dest); + js_msg = String::Concat(js_msg, FIXED_ONE_BYTE_STRING(isolate, "'")); } - if (syscall != nullptr) { - obj->Set(env->syscall_string(), OneByteString(env->isolate(), syscall)); - } + Local e = Exception::Error(js_msg)->ToObject(isolate); + + // TODO(piscisaureus) errno should probably go; the user has no way of + // knowing which uv errno value maps to which error. + e->Set(env->errno_string(), Integer::New(isolate, errorno)); + e->Set(env->code_string(), js_code); + e->Set(env->syscall_string(), js_syscall); + if (!js_path.IsEmpty()) + e->Set(env->path_string(), js_path); + if (!js_dest.IsEmpty()) + e->Set(env->dest_string(), js_dest); return e; } diff --git a/src/node.h b/src/node.h index 7d80dc35e0bb04..097441e107009c 100644 --- a/src/node.h +++ b/src/node.h @@ -59,6 +59,12 @@ NODE_EXTERN v8::Local UVException(v8::Isolate* isolate, const char* syscall = NULL, const char* message = NULL, const char* path = NULL); +NODE_EXTERN v8::Local UVException(v8::Isolate* isolate, + int errorno, + const char* syscall, + const char* message, + const char* path, + const char* dest); NODE_DEPRECATED("Use UVException(isolate, ...)", inline v8::Local ErrnoException( diff --git a/src/node_file.cc b/src/node_file.cc index 287d9a56ba116e..b05e9cd55985ba 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -109,23 +109,14 @@ static void After(uv_fs_t *req) { Local argv[2]; if (req->result < 0) { - // If the request doesn't have a path parameter set. - if (req->path == nullptr) { - argv[0] = UVException(req->result, nullptr, req_wrap->syscall()); - } else if ((req->result == UV_EEXIST || - req->result == UV_ENOTEMPTY || - req->result == UV_EPERM) && - req_wrap->dest_len() > 0) { - argv[0] = UVException(req->result, - nullptr, - req_wrap->syscall(), - req_wrap->dest()); - } else { - argv[0] = UVException(req->result, - nullptr, - req_wrap->syscall(), - static_cast(req->path)); - } + // An error happened. + const char* dest = req_wrap->dest_len() > 0 ? req_wrap->dest() : nullptr; + argv[0] = UVException(env->isolate(), + req->result, + req_wrap->syscall(), + nullptr, + req->path, + dest); } else { // error value is empty or null for non-error. argv[0] = Null(env->isolate()); @@ -270,14 +261,7 @@ struct fs_req_wrap { __VA_ARGS__, \ nullptr); \ if (err < 0) { \ - if (dest != nullptr && \ - (err == UV_EEXIST || \ - err == UV_ENOTEMPTY || \ - err == UV_EPERM)) { \ - return env->ThrowUVException(err, #func, "", dest); \ - } else { \ - return env->ThrowUVException(err, #func, "", path); \ - } \ + return env->ThrowUVException(err, #func, nullptr, path, dest); \ } \ #define SYNC_CALL(func, path, ...) \ diff --git a/src/node_internals.h b/src/node_internals.h index cb8a4a802d3749..92da0767befcb8 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -160,7 +160,8 @@ void ThrowUVException(v8::Isolate* isolate, int errorno, const char* syscall = nullptr, const char* message = nullptr, - const char* path = nullptr); + const char* path = nullptr, + const char* dest = nullptr); NODE_DEPRECATED("Use ThrowError(isolate)", inline void ThrowError(const char* errmsg) { diff --git a/test/parallel/test-domain.js b/test/parallel/test-domain.js index a3af1ff15a4cdf..a3f7b76848a581 100644 --- a/test/parallel/test-domain.js +++ b/test/parallel/test-domain.js @@ -48,7 +48,7 @@ d.on('error', function(er) { assert.equal(er.domainThrown, true); break; - case "ENOENT, open 'this file does not exist'": + case "ENOENT: no such file or directory, open 'this file does not exist'": assert.equal(er.domain, d); assert.equal(er.domainThrown, false); assert.equal(typeof er.domainBound, 'function'); @@ -58,7 +58,7 @@ d.on('error', function(er) { assert.equal(typeof er.errno, 'number'); break; - case "ENOENT, open 'stream for nonexistent file'": + case "ENOENT: no such file or directory, open 'stream for nonexistent file'": assert.equal(typeof er.errno, 'number'); assert.equal(er.code, 'ENOENT'); assert.equal(er_path, 'stream for nonexistent file'); diff --git a/test/parallel/test-fs-error-messages.js b/test/parallel/test-fs-error-messages.js index 4913ca255de7a8..e174e1c023ff21 100644 --- a/test/parallel/test-fs-error-messages.js +++ b/test/parallel/test-fs-error-messages.js @@ -29,10 +29,12 @@ fs.link(fn, 'foo', function(err) { }); fs.link(existingFile, existingFile2, function(err) { + assert.ok(0 <= err.message.indexOf(existingFile)); assert.ok(0 <= err.message.indexOf(existingFile2)); }); fs.symlink(existingFile, existingFile2, function(err) { + assert.ok(0 <= err.message.indexOf(existingFile)); assert.ok(0 <= err.message.indexOf(existingFile2)); }); @@ -45,6 +47,7 @@ fs.rename(fn, 'foo', function(err) { }); fs.rename(existingDir, existingDir2, function(err) { + assert.ok(0 <= err.message.indexOf(existingDir)); assert.ok(0 <= err.message.indexOf(existingDir2)); }); @@ -130,6 +133,7 @@ try { fs.linkSync(existingFile, existingFile2); } catch (err) { errors.push('link'); + assert.ok(0 <= err.message.indexOf(existingFile)); assert.ok(0 <= err.message.indexOf(existingFile2)); } @@ -138,6 +142,7 @@ try { fs.symlinkSync(existingFile, existingFile2); } catch (err) { errors.push('symlink'); + assert.ok(0 <= err.message.indexOf(existingFile)); assert.ok(0 <= err.message.indexOf(existingFile2)); } @@ -186,6 +191,7 @@ try { fs.renameSync(existingDir, existingDir2); } catch (err) { errors.push('rename'); + assert.ok(0 <= err.message.indexOf(existingDir)); assert.ok(0 <= err.message.indexOf(existingDir2)); }