From 7ca77eaf38cbf6da451a0fb50c0940a47b642ae4 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 2 Jul 2013 00:27:26 -0700 Subject: [PATCH] fs: write strings directly to disk Prior, strings would first be converted to a Buffer before being written to disk. Now the intermediary step has been removed. Other changes of note: * Class member "must_free" was added to req_wrap so to track if the memory needs to be manually cleaned up after use. * External String Resource support, so the memory will be used directly instead of copying out the data. * Docs have been updated to reflect that if position is not a number then it will assume null. Previously it specified the argument must be null, but that was not how the code worked. An attempt was made to only support == null, but there were too many tests that assumed != number would be enough. * Docs update show some of the write/writeSync arguments are optional. --- doc/api/fs.markdown | 45 +++++++++++++++---- lib/fs.js | 79 ++++++++++++++++++--------------- src/node_file.cc | 125 ++++++++++++++++++++++++++++++++++++++++------------ src/string_bytes.cc | 4 +- 4 files changed, 180 insertions(+), 73 deletions(-) diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown index c0e5363..9457314 100644 --- a/doc/api/fs.markdown +++ b/doc/api/fs.markdown @@ -373,19 +373,18 @@ to the completion callback. Synchronous fsync(2). -## fs.write(fd, buffer, offset, length, position, callback) +## fs.write(fd, buffer, offset, length[, position], callback) Write `buffer` to the file specified by `fd`. `offset` and `length` determine the part of the buffer to be written. `position` refers to the offset from the beginning of the file where this data -should be written. If `position` is `null`, the data will be written at the -current position. -See pwrite(2). +should be written. If `typeof position !== 'number'`, the data will be written +at the current position. See pwrite(2). -The callback will be given three arguments `(err, written, buffer)` where `written` -specifies how many _bytes_ were written from `buffer`. +The callback will be given three arguments `(err, written, buffer)` where +`written` specifies how many _bytes_ were written from `buffer`. Note that it is unsafe to use `fs.write` multiple times on the same file without waiting for the callback. For this scenario, @@ -395,9 +394,39 @@ On Linux, positional writes don't work when the file is opened in append mode. The kernel ignores the position argument and always appends the data to the end of the file. -## fs.writeSync(fd, buffer, offset, length, position) +## fs.write(fd, data[, position[, encoding]], callback) -Synchronous version of `fs.write()`. Returns the number of bytes written. +Write `data` to the file specified by `fd`. If `data` is not a Buffer instance +then the value will be coerced to a string. + +`position` refers to the offset from the beginning of the file where this data +should be written. If `typeof position !== 'number'` the data will be written at +the current position. See pwrite(2). + +`encoding` is the expected string encoding. + +The callback will receive the arguments `(err, written, string)` where `written` +specifies how many _bytes_ the passed string required to be written. Note that +bytes written is not the same as string characters. See +[Buffer.byteLength](buffer.html#buffer_class_method_buffer_bytelength_string_encoding). + +Unlike when writing `buffer`, the entire string must be written. No substring +may be specified. This is because the byte offset of the resulting data may not +be the same as the string offset. + +Note that it is unsafe to use `fs.write` multiple times on the same file +without waiting for the callback. For this scenario, +`fs.createWriteStream` is strongly recommended. + +On Linux, positional writes don't work when the file is opened in append mode. +The kernel ignores the position argument and always appends the data to +the end of the file. + +## fs.writeSync(fd, buffer, offset, length[, position]) + +## fs.writeSync(fd, data[, position[, encoding]]) + +Synchronous versions of `fs.write()`. Returns the number of bytes written. ## fs.read(fd, buffer, offset, length, position, callback) diff --git a/lib/fs.js b/lib/fs.js index b1ca7aa..78e63c9 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -462,50 +462,59 @@ fs.readSync = function(fd, buffer, offset, length, position) { return [str, r]; }; +// usage: +// fs.write(fd, buffer, offset, length[, position], callback); +// OR +// fs.write(fd, string[, position[, encoding]], callback); fs.write = function(fd, buffer, offset, length, position, callback) { - if (!IS_BUFFER(buffer)) { - // legacy string interface (fd, data, position, encoding, callback) - callback = arguments[4]; - position = arguments[2]; - assertEncoding(arguments[3]); - - buffer = new Buffer('' + arguments[1], arguments[3]); - offset = 0; - length = buffer.length; + if (IS_BUFFER(buffer)) { + // if no position is passed then assume null + if (IS_FUNCTION(position)) { + callback = position; + position = null; + } + callback = maybeCallback(callback); + var wrapper = function(err, written) { + // Retain a reference to buffer so that it can't be GC'ed too soon. + callback(err, written || 0, buffer); + }; + return binding.writeBuffer(fd, buffer, offset, length, position, wrapper); } - if (!length) { - if (IS_FUNCTION(callback)) { - process.nextTick(function() { - callback(undefined, 0); - }); + if (IS_STRING(buffer)) + buffer += ''; + if (!IS_FUNCTION(position)) { + if (IS_FUNCTION(offset)) { + position = offset; + offset = null; + } else { + position = length; } - return; + length = 'utf8'; } - - callback = maybeCallback(callback); - - function wrapper(err, written) { - // Retain a reference to buffer so that it can't be GC'ed too soon. + callback = maybeCallback(position); + position = function(err, written) { + // retain reference to string in case it's external callback(err, written || 0, buffer); - } - - binding.write(fd, buffer, offset, length, position, wrapper); + }; + return binding.writeString(fd, buffer, offset, length, position); }; +// usage: +// fs.writeSync(fd, buffer, offset, length[, position]); +// OR +// fs.writeSync(fd, string[, position[, encoding]]); fs.writeSync = function(fd, buffer, offset, length, position) { - if (!IS_BUFFER(buffer)) { - // legacy string interface (fd, data, position, encoding) - position = arguments[2]; - assertEncoding(arguments[3]); - - buffer = new Buffer('' + arguments[1], arguments[3]); - offset = 0; - length = buffer.length; - } - if (!length) return 0; - - return binding.write(fd, buffer, offset, length, position); + if (IS_BUFFER(buffer)) { + if (IS_UNDEFINED(position)) + position = null; + return binding.writeBuffer(fd, buffer, offset, length, position); + } + if (!IS_STRING(buffer)) + buffer += ''; + if (IS_UNDEFINED(offset)) + offset = null; + return binding.writeString(fd, buffer, offset, length, position); }; fs.rename = function(oldPath, newPath, callback) { diff --git a/src/node_file.cc b/src/node_file.cc index 8026610..1390ebc 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -22,8 +22,10 @@ #include "node.h" #include "node_file.h" #include "node_buffer.h" +#include "node_internals.h" #include "node_stat_watcher.h" #include "req_wrap.h" +#include "string_bytes.h" #include #include @@ -62,10 +64,12 @@ using v8::Value; class FSReqWrap: public ReqWrap { public: FSReqWrap(const char* syscall) - : syscall_(syscall) { + : must_free_(false), + syscall_(syscall) { } const char* syscall() { return syscall_; } + bool must_free_; // request is responsible for free'ing memory oncomplete private: const char* syscall_; @@ -97,6 +101,10 @@ static void After(uv_fs_t *req) { FSReqWrap* req_wrap = (FSReqWrap*) req->data; assert(&req_wrap->req_ == req); + // check if data needs to be cleaned + if (req_wrap->must_free_ == true) + delete[] static_cast(req_wrap->data_); + // there is always at least one argument. "error" int argc = 1; @@ -485,7 +493,7 @@ static void Rename(const FunctionCallbackInfo& 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]); @@ -650,56 +658,114 @@ static void Open(const FunctionCallbackInfo& args) { } } -// bytesWritten = write(fd, data, position, enc, callback) + // Wrapper for write(2). // +// bytesWritten = write(fd, buffer, offset, length, position, callback) // 0 fd integer. file descriptor // 1 buffer the data to write // 2 offset where in the buffer to start from // 3 length how much to write // 4 position if integer, position to write at in the file. // if null, write from the current position -static void Write(const FunctionCallbackInfo& args) { +static void WriteBuffer(const FunctionCallbackInfo& args) { HandleScope scope(node_isolate); - if (!args[0]->IsInt32()) { - return THROW_BAD_ARGS; - } + assert(args[0]->IsInt32()); + assert(Buffer::HasInstance(args[1])); int fd = args[0]->Int32Value(); + Local obj = args[1].As(); + const char* buf = Buffer::Data(obj); + size_t buffer_length = Buffer::Length(obj); + size_t off = args[2]->Uint32Value(); + size_t len = args[3]->Uint32Value(); + int64_t pos = GET_OFFSET(args[4]); + Local cb = args[5]; - if (!Buffer::HasInstance(args[1])) { - return ThrowError("Second argument needs to be a buffer"); - } + if (off > buffer_length) + return ThrowRangeError("offset out of bounds"); + if (len > buffer_length) + return ThrowRangeError("length out of bounds"); + if (off + len < off) + return ThrowRangeError("off + len overflow"); + if (off + len > buffer_length) + return ThrowRangeError("off + len > buffer.length"); - Local buffer_obj = args[1]->ToObject(); - char *buffer_data = Buffer::Data(buffer_obj); - size_t buffer_length = Buffer::Length(buffer_obj); + buf += off; - size_t off = args[2]->Int32Value(); - if (off >= buffer_length) { - return ThrowError("Offset is out of bounds"); + if (cb->IsFunction()) { + ASYNC_CALL(write, cb, fd, buf, len, pos) + return; } - ssize_t len = args[3]->Int32Value(); - if (off + len > buffer_length) { - return ThrowError("off + len > buffer.length"); - } + SYNC_CALL(write, NULL, fd, buf, len, pos) + args.GetReturnValue().Set(SYNC_RESULT); +} - ASSERT_OFFSET(args[4]); - int64_t pos = GET_OFFSET(args[4]); - char * buf = (char*)buffer_data + off; - Local cb = args[5]; +// Wrapper for write(2). +// +// bytesWritten = write(fd, string, position, enc, callback) +// 0 fd integer. file descriptor +// 1 string non-buffer values are converted to strings +// 2 position if integer, position to write at in the file. +// if null, write from the current position +// 3 enc encoding of string +static void WriteString(const FunctionCallbackInfo& args) { + HandleScope scope(node_isolate); + + if (!args[0]->IsInt32()) + return ThrowTypeError("First argument must be file descriptor"); + + Local cb; + Local string = args[1]; + int fd = args[0]->Int32Value(); + char* buf = NULL; + int64_t pos; + size_t len; + bool must_free_ = false; + + // will assign buf and len if string was external + if (!StringBytes::GetExternalParts(string, + const_cast(&buf), + &len)) { + enum encoding enc = ParseEncoding(args[3], UTF8); + len = StringBytes::StorageSize(string, enc); + buf = new char[len]; + // StorageSize may return too large a char, so correct the actual length + // by the write size + len = StringBytes::Write(buf, len, args[1], enc); + must_free_ = true; + } + pos = GET_OFFSET(args[2]); + cb = args[4]; if (cb->IsFunction()) { - ASYNC_CALL(write, cb, fd, buf, len, pos) - } else { - SYNC_CALL(write, 0, fd, buf, len, pos) - args.GetReturnValue().Set(SYNC_RESULT); + FSReqWrap* req_wrap = new FSReqWrap("write"); + int err = uv_fs_write(uv_default_loop(), &req_wrap->req_, + fd, buf, len, pos, After); + req_wrap->object()->Set(oncomplete_sym, cb); + req_wrap->must_free_ = must_free_; + req_wrap->Dispatched(); + req_wrap->data_ = buf; + if (err < 0) { + uv_fs_t* req = &req_wrap->req_; + req->result = err; + req->path = NULL; + After(req); + } + return args.GetReturnValue().Set(req_wrap->persistent()); } + + SYNC_CALL(write, NULL, fd, buf, len, pos) + args.GetReturnValue().Set(SYNC_RESULT); + + if (must_free_) + delete[] buf; } + /* * Wrapper for read(2). * @@ -918,7 +984,8 @@ void File::Initialize(Handle target) { NODE_SET_METHOD(target, "symlink", Symlink); NODE_SET_METHOD(target, "readlink", ReadLink); NODE_SET_METHOD(target, "unlink", Unlink); - NODE_SET_METHOD(target, "write", Write); + NODE_SET_METHOD(target, "writeBuffer", WriteBuffer); + NODE_SET_METHOD(target, "writeString", WriteString); NODE_SET_METHOD(target, "chmod", Chmod); NODE_SET_METHOD(target, "fchmod", FChmod); diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 492c135..c616c17 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -239,7 +239,9 @@ bool StringBytes::GetExternalParts(Handle val, return true; } - assert(val->IsString()); + if (!val->IsString()) + return false; + Local str = Local::New(val.As()); if (str->IsExternalAscii()) { -- 2.7.4