Revert "Deprecate string interface for fs.read()"

This reverts commit cbbf9e4.

doc/api.markdown

@@ -1410,24 +1410,20 @@ specifies how many _bytes_ were written.
 
 Synchronous version of `fs.write()`. Returns the number of bytes written.
 
-### fs.read(fd, buffer, offset, length, position, callback)
+### fs.read(fd, length, position, encoding, 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 the two arguments, `(err, bytesRead)`.
+The callback is given three arguments, `(err, data, bytesRead)` where `data`
+is a string--what was read--and `bytesRead` is the number of bytes read.
 
-### fs.readSync(fd, buffer, offset, length, position)
+### fs.readSync(fd, length, position, encoding)
 
-Synchronous version of `fs.read`. Returns the number of `bytesRead`.
+Synchronous version of `fs.read`. Returns an array `[data, 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 = fs.read(fd, 4*1024, pos, encoding)) && r[0]) {
+  while ((r = binding.read(fd, 4*1024, pos, encoding)) && r[0]) {
     content += r[0];
     pos += r[1]
   }
@@ -143,54 +143,14 @@ fs.openSync = function (path, flags, mode) {
   return binding.open(path, stringToFlags(flags), mode);
 };
 
-fs.read = function (fd, buffer, offset, length, position, callback) {
-  if (!(buffer instanceof Buffer)) {
-    // 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.read = function (fd, length, position, encoding, callback) {
+  encoding = encoding || "binary";
+  binding.read(fd, length, position, encoding, callback || noop);
 };
 
-fs.readSync = function (fd, buffer, offset, length, position) {
-  var legacy = false;
-  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.readSync = function (fd, length, position, encoding) {
+  encoding = encoding || "binary";
+  return binding.read(fd, length, position, encoding);
 };
 
 fs.write = function (fd, buffer, offset, length, position, callback) {

src/node_file.cc

@@ -106,9 +106,18 @@ static int After(eio_req *req) {
 
       case EIO_READ:
       {
-        // Buffer interface
-        argv[1] = Integer::New(req->result);
-        argc = 2;
+        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;
       }
 
@@ -575,6 +584,16 @@ 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;
@@ -586,47 +605,105 @@ 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])) {
-    return ThrowException(Exception::Error(
-                String::New("Second argument needs to be a buffer")));
-  }
+  if (Buffer::HasInstance(args[1])) {
+    legacy = false;
+    // 0 fd        integer. file descriptor
+    // 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();
+    if (off >= buffer->length()) {
+      return ThrowException(Exception::Error(
+            String::New("Offset is out of bounds")));
+    }
 
-  Buffer * buffer = ObjectWrap::Unwrap<Buffer>(args[1]->ToObject());
+    len = args[3]->Int32Value();
+    if (off + len > buffer->length()) {
+      return ThrowException(Exception::Error(
+            String::New("Length is extends beyond buffer")));
+    }
 
-  size_t off = args[2]->Int32Value();
-  if (off >= buffer->length()) {
-    return ThrowException(Exception::Error(
-          String::New("Offset is out of bounds")));
-  }
+    pos = GET_OFFSET(args[4]);
 
-  len = args[3]->Int32Value();
-  if (off + len > buffer->length()) {
-    return ThrowException(Exception::Error(
-          String::New("Length is extends beyond buffer")));
-  }
+    buf = (char*)buffer->data() + off;
+
+    cb = args[5];
 
-  pos = GET_OFFSET(args[4]);
+  } 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 = (char*)buffer->data() + off;
+    buf = NULL; // libeio will allocate and free it.
+
+    cb = args[4];
+  }
 
-  cb = args[5];
 
   if (cb->IsFunction()) {
-    ASYNC_CALL(read, cb, fd, buf, len, pos);
+    // WARNING: HACK AGAIN, PROCEED WITH CAUTION
+    // 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;
 
-    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);
+    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);
+    }
   }
 }