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

@@ -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`

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) {

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<char*>(req->ptr2);
-  }
-
   TryCatch try_catch;
 
   (*callback)->Call(Context::GetCurrent()->Global(), argc, argv);
@@ -536,14 +531,6 @@ static Handle<Value> 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<Value> Write(const Arguments& args) {
   HandleScope scope;
 
@@ -553,98 +540,34 @@ static Handle<Value> Write(const Arguments& args) {
 
   int fd = args[0]->Int32Value();
 
-  off_t pos;
-  ssize_t len;
-  char * buf;
-  ssize_t written;
-
-  Local<Value> 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<Buffer>(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<Buffer>(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<Value> 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<Value> 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<char*>(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));
   }

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);
+});
+

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');