From cbbf9e43d1770047e98fe65d5f710815f432a5b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20Geisend=C3=B6rfer?= Date: Thu, 20 May 2010 18:13:22 -0400 Subject: [PATCH] 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 | 14 ++-- lib/fs.js | 54 ++++++++++-- src/node_file.cc | 129 ++++++----------------------- test/simple/test-fs-read-buffer.js | 25 ++++++ test/simple/test-fs-read.js | 23 +++++ 5 files changed, 130 insertions(+), 115 deletions(-) create mode 100644 test/simple/test-fs-read-buffer.js create mode 100644 test/simple/test-fs-read.js diff --git a/doc/api.markdown b/doc/api.markdown index 488035e37b..6afc4661af 100644 --- a/doc/api.markdown +++ b/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` -is a string--what was read--and `bytesRead` is the number of bytes read. +The callback is given the two arguments, `(err, bytesRead)`. -### 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) diff --git a/lib/fs.js b/lib/fs.js index ec5a113eb8..fc0843b730 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 = binding.read(fd, 4*1024, pos, encoding)) && r[0]) { + while ((r = fs.read(fd, 4*1024, pos, encoding)) && r[0]) { content += r[0]; pos += r[1] } @@ -143,14 +143,54 @@ fs.openSync = function (path, flags, mode) { return binding.open(path, stringToFlags(flags), mode); }; -fs.read = function (fd, length, position, encoding, callback) { - encoding = encoding || "binary"; - binding.read(fd, length, position, encoding, callback || noop); +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.readSync = function (fd, length, position, encoding) { - encoding = encoding || "binary"; - return binding.read(fd, length, position, encoding); +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.write = function (fd, buffer, offset, length, position, callback) { diff --git a/src/node_file.cc b/src/node_file.cc index 6e9ef1a4d6..49ba894dcf 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -106,18 +106,9 @@ static int After(eio_req *req) { case EIO_READ: { - 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; - } + // Buffer interface + argv[1] = Integer::New(req->result); + argc = 2; break; } @@ -584,16 +575,6 @@ 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; @@ -605,105 +586,47 @@ 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])) { - 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]); + if (!Buffer::HasInstance(args[1])) { + return ThrowException(Exception::Error( + String::New("Second argument needs to be a buffer"))); + } - buf = NULL; // libeio will allocate and free it. + Buffer * buffer = ObjectWrap::Unwrap(args[1]->ToObject()); - cb = args[4]; + 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"))); + } - if (cb->IsFunction()) { - // 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); + pos = GET_OFFSET(args[4]); - - eio_req *req = eio_read(fd, buf, len, pos, - EIO_PRI_DEFAULT, - After, - cb_persist(cb)); - assert(req); + buf = (char*)buffer->data() + off; - req->int3 = legacy ? 1 : 0; - ev_ref(EV_DEFAULT_UC); - return Undefined(); + cb = args[5]; + if (cb->IsFunction()) { + ASYNC_CALL(read, cb, fd, buf, len, pos); } 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 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); - } + 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 new file mode 100644 index 0000000000..5a18177a4d --- /dev/null +++ b/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); +}); \ No newline at end of file diff --git a/test/simple/test-fs-read.js b/test/simple/test-fs-read.js new file mode 100644 index 0000000000..7f9bf48d4a --- /dev/null +++ b/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); +}); \ No newline at end of file