From c46cbe0de4613245095115dd909c254a9da2ed1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Wed, 19 May 2010 14:17:50 -0400 Subject: [PATCH] Deprecate string interface for fs.write() This patch makes buffers the preferred input for fs.write() and fs.writeSync(). The old string interface is still supported by converting strings to buffers dynamically. This allows to remove the C++ code for string handling which is also part of this patch. --- doc/api.markdown | 12 ++- lib/fs.js | 29 +++++-- src/node_file.cc | 117 +++++----------------------- test/simple/test-fs-write-buffer.js | 32 ++++++++ test/simple/test-fs-write-sync.js | 12 +++ 5 files changed, 95 insertions(+), 107 deletions(-) create mode 100644 test/simple/test-fs-write-buffer.js create mode 100644 test/simple/test-fs-write-sync.js diff --git a/doc/api.markdown b/doc/api.markdown index 9999148799..488035e37b 100644 --- a/doc/api.markdown +++ b/doc/api.markdown @@ -1392,11 +1392,15 @@ or 'a+'. The callback gets two arguments `(err, fd)`. Synchronous open(2). -### fs.write(fd, data, position, encoding, callback) +### fs.write(fd, buffer, offset, length, position, callback) -Write data to the file specified by `fd`. `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. +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). The callback will be given two arguments `(err, written)` where `written` diff --git a/lib/fs.js b/lib/fs.js index ec68c02e9d..65469287bc 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -153,14 +153,31 @@ fs.readSync = function (fd, length, position, encoding) { return binding.read(fd, length, position, encoding); }; -fs.write = function (fd, data, position, encoding, callback) { - encoding = encoding || "binary"; - binding.write(fd, data, position, encoding, callback || noop); +fs.write = function (fd, buffer, offset, length, position, callback) { + if (!(buffer instanceof Buffer)) { + // legacy string interface (fd, data, position, encoding, callback) + callback = arguments[4]; + position = arguments[2]; + + buffer = new Buffer(''+arguments[1], arguments[3]); + offset = 0; + length = buffer.length; + } + + binding.write(fd, buffer, offset, length, position, callback || noop); }; -fs.writeSync = function (fd, data, position, encoding) { - encoding = encoding || "binary"; - return binding.write(fd, data, position, encoding); +fs.writeSync = function (fd, buffer, offset, length, position) { + if (!(buffer instanceof Buffer)) { + // legacy string interface (fd, data, position, encoding) + position = arguments[2]; + + buffer = new Buffer(''+arguments[1], arguments[3]); + offset = 0; + length = buffer.length; + } + + binding.write(fd, buffer, offset, length, position); }; fs.rename = function (oldPath, newPath, callback) { diff --git a/src/node_file.cc b/src/node_file.cc index f36b79e6e6..cb974c2347 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -150,11 +150,6 @@ static int After(eio_req *req) { } } - if (req->type == EIO_WRITE && req->int3 == 1) { - assert(req->ptr2); - delete [] reinterpret_cast(req->ptr2); - } - TryCatch try_catch; (*callback)->Call(Context::GetCurrent()->Global(), argc, argv); @@ -536,14 +531,6 @@ static Handle Open(const Arguments& args) { // 3 length how much to write // 4 position if integer, position to write at in the file. // if null, write from the current position -// -// - OR - -// -// 0 fd integer. file descriptor -// 1 string the data to write -// 2 position if integer, position to write at in the file. -// if null, write from the current position -// 3 encoding static Handle Write(const Arguments& args) { HandleScope scope; @@ -553,98 +540,34 @@ static Handle Write(const Arguments& args) { int fd = args[0]->Int32Value(); - off_t pos; - ssize_t len; - char * buf; - ssize_t written; - - Local cb; - bool legacy; - - if (Buffer::HasInstance(args[1])) { - legacy = false; - // buffer - // - // 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 - - Buffer * buffer = ObjectWrap::Unwrap(args[1]->ToObject()); - - size_t off = args[2]->Int32Value(); - if (off >= buffer->length()) { - return ThrowException(Exception::Error( - String::New("Offset is out of bounds"))); - } - - len = args[3]->Int32Value(); - if (off + len > buffer->length()) { - return ThrowException(Exception::Error( - String::New("Length is extends beyond buffer"))); - } - - pos = GET_OFFSET(args[4]); - - buf = (char*)buffer->data() + off; - - cb = args[5]; + if (!Buffer::HasInstance(args[1])) { + return ThrowException(Exception::Error( + String::New("Second argument needs to be a buffer"))); + } - } else { - legacy = true; - // legacy interface.. args[1] is a string + Buffer * buffer = ObjectWrap::Unwrap(args[1]->ToObject()); - pos = GET_OFFSET(args[2]); + size_t off = args[2]->Int32Value(); + if (off >= buffer->length()) { + return ThrowException(Exception::Error( + String::New("Offset is out of bounds"))); + } - enum encoding enc = ParseEncoding(args[3]); + ssize_t len = args[3]->Int32Value(); + if (off + len > buffer->length()) { + return ThrowException(Exception::Error( + String::New("Length is extends beyond buffer"))); + } - len = DecodeBytes(args[1], enc); - if (len < 0) { - Local exception = Exception::TypeError(String::New("Bad argument")); - return ThrowException(exception); - } + off_t pos = GET_OFFSET(args[4]); - buf = new char[len]; - written = DecodeWrite(buf, len, args[1], enc); - assert(written == len); - - cb = args[4]; - } + char * buf = (char*)buffer->data() + off; + Local cb = args[5]; if (cb->IsFunction()) { - // WARNING: HACK AHEAD, PROCEED WITH CAUTION - // Normally here I would do - // ASYNC_CALL(write, cb, fd, buf, len, pos) - // however, I'm trying to support a legacy interface; where in the - // String version a chunk of memory is allocated for the string to be - // decoded into. In the After() function it is freed. - // - // However in the Buffer version, we increase the reference count - // to the Buffer while we're in the thread pool. - // - // We have to let After() know which version is being done so it can - // know if it needs to free 'buf' or not. We do that by using - // req->int3. - // - // req->int3 == 1 legacy, String version. Need to free `buf`. - // req->int3 == 0 Buffer version. Don't do anything. - // - eio_req *req = eio_write(fd, buf, len, pos, - EIO_PRI_DEFAULT, - After, - cb_persist(cb)); - assert(req); - req->int3 = legacy ? 1 : 0; - ev_ref(EV_DEFAULT_UC); - return Undefined(); - + ASYNC_CALL(write, cb, fd, buf, len, pos) } else { - written = pos < 0 ? write(fd, buf, len) : pwrite(fd, buf, len, pos); - if (legacy) { - delete [] reinterpret_cast(buf); - } + ssize_t written = pos < 0 ? write(fd, buf, len) : pwrite(fd, buf, len, pos); if (written < 0) return ThrowException(ErrnoException(errno)); return scope.Close(Integer::New(written)); } diff --git a/test/simple/test-fs-write-buffer.js b/test/simple/test-fs-write-buffer.js new file mode 100644 index 0000000000..ca96b2312d --- /dev/null +++ b/test/simple/test-fs-write-buffer.js @@ -0,0 +1,32 @@ +require('../common'); +var path = require('path') + , Buffer = require('buffer').Buffer + , fs = require('fs') + , fn = path.join(fixturesDir, 'write.txt') + , expected = new Buffer('hello') + , openCalled = 0 + , writeCalled = 0; + + +fs.open(fn, 'w', 0644, function (err, fd) { + openCalled++; + if (err) throw err; + + fs.write(fd, expected, 0, expected.length, null, function(err, written) { + writeCalled++; + if (err) throw err; + + assert.equal(expected.length, written); + fs.closeSync(fd); + + var found = fs.readFileSync(fn); + assert.deepEqual(found, expected.toString()); + fs.unlinkSync(fn); + }); +}); + +process.addListener("exit", function () { + assert.equal(openCalled, 1); + assert.equal(writeCalled, 1); +}); + diff --git a/test/simple/test-fs-write-sync.js b/test/simple/test-fs-write-sync.js new file mode 100644 index 0000000000..2e30cc704e --- /dev/null +++ b/test/simple/test-fs-write-sync.js @@ -0,0 +1,12 @@ +require('../common'); +var path = require('path') + , Buffer = require('buffer').Buffer + , fs = require('fs') + , fn = path.join(fixturesDir, 'write.txt'); + +var fd = fs.openSync(fn, 'w'); +fs.writeSync(fd, 'foo'); +fs.writeSync(fd, new Buffer('bar'), 0, 3); +fs.closeSync(fd); + +assert.equal(fs.readFileSync(fn), 'foobar'); \ No newline at end of file