From e84395ff8c50a0b54cd2885de8d93a5b39b8477c Mon Sep 17 00:00:00 2001 From: Ryan Dahl Date: Thu, 20 May 2010 16:11:33 -0700 Subject: [PATCH] Revert "Deprecate string interface for fs.read()" This reverts commit cbbf9e43d1770047e98fe65d5f710815f432a5b4. --- doc/api.markdown | 14 ++- lib/fs.js | 54 ++---------- src/node_file.cc | 137 ++++++++++++++++++++++------- test/simple/test-fs-read-buffer.js | 25 ------ test/simple/test-fs-read.js | 23 ----- 5 files changed, 119 insertions(+), 134 deletions(-) delete mode 100644 test/simple/test-fs-read-buffer.js delete mode 100644 test/simple/test-fs-read.js diff --git a/doc/api.markdown b/doc/api.markdown index 6afc4661afd..488035e37b4 100644 --- a/doc/api.markdown +++ b/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) diff --git a/lib/fs.js b/lib/fs.js index fc0843b7306..ec5a113eb88 100644 --- a/lib/fs.js +++ b/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) { diff --git a/src/node_file.cc b/src/node_file.cc index 49ba894dcfc..6e9ef1a4d6d 100644 --- a/src/node_file.cc +++ b/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 obj = Local::New(*callback); + Local 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 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 Read(const Arguments& args) { HandleScope scope; @@ -586,47 +605,105 @@ static Handle Read(const Arguments& args) { int fd = args[0]->Int32Value(); Local 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(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]; + + } 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]; } - 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 (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 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 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 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 bytesRead = Integer::New(ret); + return scope.Close(bytesRead); + } } } diff --git a/test/simple/test-fs-read-buffer.js b/test/simple/test-fs-read-buffer.js deleted file mode 100644 index 5a18177a4df..00000000000 --- a/test/simple/test-fs-read-buffer.js +++ /dev/null @@ -1,25 +0,0 @@ -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); -}); \ No newline at end of file diff --git a/test/simple/test-fs-read.js b/test/simple/test-fs-read.js deleted file mode 100644 index 7f9bf48d4a4..00000000000 --- a/test/simple/test-fs-read.js +++ /dev/null @@ -1,23 +0,0 @@ -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); -}); \ No newline at end of file