Deprecate string interface for fs.read()

This patch makes buffers the preferred output for fs.read() and
fs.readSync(). The old string interface is still supported by
converting buffers to strings dynamically. This allows to remove the
C++ code for string handling which is also part of this patch.

doc/api.markdown

@@ -1410,20 +1410,24 @@ specifies how many _bytes_ were written.
 
 Synchronous version of `fs.write()`. Returns the number of bytes written.
 
-### fs.read(fd, length, position, encoding, callback)
+### fs.read(fd, buffer, offset, length, position, callback)
 
 Read data from the file specified by `fd`.
 
+`buffer` is the buffer that the data will be written to.
+
+`offset` is offset within the buffer where writing will start.
+
 `length` is an integer specifying the number of bytes to read.
 
 `position` is an integer specifying where to begin reading from in the file.
+If `position` is `null`, data will be read from the current file position.
 
-The callback is given three arguments, `(err, data, bytesRead)` where `data`
+The callback is given the two arguments, `(err, bytesRead)`.
-is a string--what was read--and `bytesRead` is the number of bytes read.
 
-### fs.readSync(fd, length, position, encoding)
+### fs.readSync(fd, buffer, offset, length, position)
 
-Synchronous version of `fs.read`. Returns an array `[data, bytesRead]`.
+Synchronous version of `fs.read`. Returns the number of `bytesRead`.
 
 ### fs.readFile(filename, [encoding,] callback)
 

lib/fs.js

@@ -92,7 +92,7 @@ fs.readFileSync = function (path, encoding) {
   var pos = null;   // leave null to allow reads on unseekable devices
   var r;
 
-  while ((r = binding.read(fd, 4*1024, pos, encoding)) && r[0]) {
+  while ((r = fs.readSync(fd, 4*1024, pos, encoding)) && r[0]) {
     content += r[0];
     pos += r[1]
   }
@@ -143,14 +143,52 @@ fs.openSync = function (path, flags, mode) {
   return binding.open(path, stringToFlags(flags), mode);
 };
 
-fs.read = function (fd, length, position, encoding, callback) {
+fs.read = function (fd, buffer, offset, length, position, callback) {
-  encoding = encoding || "binary";
+  if (!(buffer instanceof Buffer)) {
-  binding.read(fd, length, position, encoding, callback || noop);
+    // legacy string interface (fd, length, position, encoding, callback)
+    var cb = arguments[4],
+        encoding = arguments[3];
+    position = arguments[2];
+    length = arguments[1];
+    buffer = new Buffer(length);
+    offset = 0;
+
+    callback = function(err, bytesRead) {
+      if (!cb) return;
+
+      var str = (bytesRead > 0)
+        ? buffer.toString(encoding, 0, bytesRead)
+        : '';
+
+      (cb)(err, str, bytesRead);
+    };
+  }
+
+  binding.read(fd, buffer, offset, length, position, callback || noop);
 };
 
-fs.readSync = function (fd, length, position, encoding) {
+fs.readSync = function (fd, buffer, offset, length, position) {
-  encoding = encoding || "binary";
+  var legacy = false;
-  return binding.read(fd, length, position, encoding);
+  if (!(buffer instanceof Buffer)) {
+    // legacy string interface (fd, length, position, encoding, callback)
+    legacy = true;
+    encoding = arguments[3];
+    position = arguments[2];
+    length = arguments[1];
+    buffer = new Buffer(length);
+
+    offset = 0;
+  }
+
+  var r = binding.read(fd, buffer, offset, length, position);
+  if (!legacy) {
+    return r;
+  }
+
+  var str = (r > 0)
+      ? buffer.toString(encoding, 0, r)
+      : '';
+  return [str, r];
 };
 
 fs.write = function (fd, buffer, offset, length, position, callback) {

src/node_file.cc

@@ -106,18 +106,9 @@ static int After(eio_req *req) {
 
       case EIO_READ:
       {
-        if (req->int3) {
-          // legacy interface
-          Local<Object> obj = Local<Object>::New(*callback);
-          Local<Value> enc_val = obj->GetHiddenValue(encoding_symbol);
-          argv[1] = Encode(req->ptr2, req->result, ParseEncoding(enc_val));
-          argv[2] = Integer::New(req->result);
-          argc = 3;
-        } else {
         // Buffer interface
         argv[1] = Integer::New(req->result);
         argc = 2;
-        }
         break;
       }
 
@@ -584,16 +575,6 @@ static Handle<Value> Write(const Arguments& args) {
  * 3 length    integer. length to read
  * 4 position  file position - null for current position
  *
- *  - OR -
- *
- * [string, bytesRead] = fs.read(fd, length, position, encoding)
- *
- * 0 fd        integer. file descriptor
- * 1 length    integer. length to read
- * 2 position  if integer, position to read from in the file.
- *             if null, read from the current position
- * 3 encoding
- *
  */
 static Handle<Value> Read(const Arguments& args) {
   HandleScope scope;
@@ -605,21 +586,17 @@ static Handle<Value> Read(const Arguments& args) {
   int fd = args[0]->Int32Value();
 
   Local<Value> cb;
-  bool legacy;
 
   size_t len;
   off_t pos;
-  enum encoding encoding;
 
   char * buf = NULL;
 
-  if (Buffer::HasInstance(args[1])) {
+  if (!Buffer::HasInstance(args[1])) {
-    legacy = false;
+    return ThrowException(Exception::Error(
-    // 0 fd        integer. file descriptor
+                String::New("Second argument needs to be a buffer")));
-    // 1 buffer    instance of Buffer
+  }
-    // 2 offset    integer. offset to start reading into inside buffer
+
-    // 3 length    integer. length to read
-    // 4 position  file position - null for current position
   Buffer * buffer = ObjectWrap::Unwrap<Buffer>(args[1]->ToObject());
 
   size_t off = args[2]->Int32Value();
@@ -640,72 +617,18 @@ static Handle<Value> Read(const Arguments& args) {
 
   cb = args[5];
 
-  } else {
-    legacy = true;
-    // 0 fd        integer. file descriptor
-    // 1 length    integer. length to read
-    // 2 position  if integer, position to read from in the file.
-    //             if null, read from the current position
-    // 3 encoding
-    len = args[1]->IntegerValue();
-    pos = GET_OFFSET(args[2]);
-    encoding = ParseEncoding(args[3]);
-
-    buf = NULL; // libeio will allocate and free it.
-
-    cb = args[4];
-  }
-
-
   if (cb->IsFunction()) {
-    // WARNING: HACK AGAIN, PROCEED WITH CAUTION
+    ASYNC_CALL(read, cb, fd, buf, len, pos);
-    // Normally here I would do
-    //   ASYNC_CALL(read, args[4], fd, NULL, len, offset)
-    // but I'm trying to support a legacy interface where it's acceptable to
-    // return a string in the callback. As well as a new Buffer interface
-    // which reads data into a user supplied buffer.
-
-    // Set the encoding on the callback
-    if (legacy) {
-      Local<Object> obj = cb->ToObject();
-      obj->SetHiddenValue(encoding_symbol, args[3]);
-    }
-
-    if (legacy) assert(buf == NULL);
-
-    
-    eio_req *req = eio_read(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();
-
   } else {
     // SYNC
     ssize_t ret;
 
-    if (legacy) {
-#define READ_BUF_LEN (16*1024)
-      char buf2[READ_BUF_LEN];
-      ret = pos < 0 ? read(fd, buf2, MIN(len, READ_BUF_LEN))
-                    : pread(fd, buf2, MIN(len, READ_BUF_LEN), pos);
-      if (ret < 0) return ThrowException(ErrnoException(errno));
-      Local<Array> a = Array::New(2);
-      a->Set(Integer::New(0), Encode(buf2, ret, encoding));
-      a->Set(Integer::New(1), Integer::New(ret));
-      return scope.Close(a);
-    } else {
     ret = pos < 0 ? read(fd, buf, len) : pread(fd, buf, len, pos);
     if (ret < 0) return ThrowException(ErrnoException(errno));
     Local<Integer> bytesRead = Integer::New(ret);
     return scope.Close(bytesRead);
   }
 }
-}
 
 /* fs.chmod(fd, mode);
  * Wrapper for chmod(1) / EIO_CHMOD

test/simple/test-fs-read-buffer.js

@@ -0,0 +1,25 @@
+require('../common');
+var path = require('path'),
+    Buffer = require('buffer').Buffer,
+    fs = require('fs'),
+    filepath = path.join(fixturesDir, 'x.txt'),
+    fd = fs.openSync(filepath, 'r'),
+    expected = 'xyz\n',
+    bufferAsync = new Buffer(expected.length),
+    bufferSync = new Buffer(expected.length),
+    readCalled = 0;
+
+fs.read(fd, bufferAsync, 0, expected.length, 0, function(err, bytesRead) {
+  readCalled++;
+
+  assert.equal(bytesRead, expected.length);
+  assert.deepEqual(bufferAsync, new Buffer(expected));
+});
+
+var r = fs.readSync(fd, bufferSync, 0, expected.length, 0);
+assert.deepEqual(bufferSync, new Buffer(expected));
+assert.equal(r, expected.length);
+
+process.addListener('exit', function() {
+  assert.equal(readCalled, 1);
+});

test/simple/test-fs-read.js

@@ -0,0 +1,23 @@
+require('../common');
+var path = require('path'),
+    fs = require('fs'),
+    filepath = path.join(fixturesDir, 'x.txt'),
+    fd = fs.openSync(filepath, 'r'),
+    expected = 'xyz\n',
+    readCalled = 0;
+
+fs.read(fd, expected.length, 0, 'utf-8', function(err, str, bytesRead) {
+  readCalled++;
+
+  assert.ok(!err);
+  assert.equal(str, expected);
+  assert.equal(bytesRead, expected.length);
+});
+
+var r = fs.readSync(fd, expected.length, 0, 'utf-8');
+assert.equal(r[0], expected);
+assert.equal(r[1], expected.length);
+
+process.addListener('exit', function() {
+  assert.equal(readCalled, 1);
+});