From 0803962860ae4cd2aa7355cbcfff3432e430e496 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johannes=20W=C3=BCller?= Date: Sat, 3 Oct 2015 02:06:42 +0200 Subject: [PATCH] fs: add file descriptor support to *File() funcs These changes affect the following functions and their synchronous counterparts: * fs.readFile() * fs.writeFile() * fs.appendFile() If the first parameter is a uint32, it is treated as a file descriptor. In all other cases, the original implementation is used to ensure backwards compatibility. File descriptor ownership is never taken from the user. The documentation was adjusted to reflect these API changes. A note was added to make the user aware of file descriptor ownership and the conditions under which a file descriptor can be used by each of these functions. Tests were extended to test for file descriptor parameters under the conditions noted in the relevant documentation. PR-URL: https://github.com/nodejs/node/pull/3163 Reviewed-By: Trevor Norris --- doc/api/fs.markdown | 36 +++++++--- lib/fs.js | 86 ++++++++++++++++++----- test/parallel/test-fs-append-file-sync.js | 14 ++++ test/parallel/test-fs-append-file.js | 33 ++++++++- test/parallel/test-fs-readfile-fd.js | 47 +++++++++++++ test/parallel/test-fs-write-file-sync.js | 12 ++++ test/parallel/test-fs-write-file.js | 31 +++++++- 7 files changed, 230 insertions(+), 29 deletions(-) create mode 100644 test/parallel/test-fs-readfile-fd.js diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown index e535ce64e2..b46c94984f 100644 --- a/doc/api/fs.markdown +++ b/doc/api/fs.markdown @@ -468,9 +468,9 @@ The callback is given the three arguments, `(err, bytesRead, buffer)`. Synchronous version of `fs.read`. Returns the number of `bytesRead`. -## fs.readFile(filename[, options], callback) +## fs.readFile(file[, options], callback) -* `filename` {String} +* `file` {String | Integer} filename or file descriptor * `options` {Object | String} * `encoding` {String | Null} default = `null` * `flag` {String} default = `'r'` @@ -492,18 +492,20 @@ If `options` is a string, then it specifies the encoding. Example: fs.readFile('/etc/passwd', 'utf8', callback); +Any specified file descriptor has to support reading. -## fs.readFileSync(filename[, options]) +_Note: Specified file descriptors will not be closed automatically._ -Synchronous version of `fs.readFile`. Returns the contents of the `filename`. +## fs.readFileSync(file[, options]) + +Synchronous version of `fs.readFile`. Returns the contents of the `file`. If the `encoding` option is specified then this function returns a string. Otherwise it returns a buffer. +## fs.writeFile(file, data[, options], callback) -## fs.writeFile(filename, data[, options], callback) - -* `filename` {String} +* `file` {String | Integer} filename or file descriptor * `data` {String | Buffer} * `options` {Object | String} * `encoding` {String | Null} default = `'utf8'` @@ -528,13 +530,21 @@ If `options` is a string, then it specifies the encoding. Example: fs.writeFile('message.txt', 'Hello Node.js', 'utf8', callback); -## fs.writeFileSync(filename, data[, options]) +Any specified file descriptor has to support writing. + +Note that it is unsafe to use `fs.writeFile` multiple times on the same file +without waiting for the callback. For this scenario, +`fs.createWriteStream` is strongly recommended. + +_Note: Specified file descriptors will not be closed automatically._ + +## fs.writeFileSync(file, data[, options]) The synchronous version of `fs.writeFile`. Returns `undefined`. -## fs.appendFile(filename, data[, options], callback) +## fs.appendFile(file, data[, options], callback) -* `filename` {String} +* `file` {String | Integer} filename or file descriptor * `data` {String | Buffer} * `options` {Object | String} * `encoding` {String | Null} default = `'utf8'` @@ -556,7 +566,11 @@ If `options` is a string, then it specifies the encoding. Example: fs.appendFile('message.txt', 'data to append', 'utf8', callback); -## fs.appendFileSync(filename, data[, options]) +Any specified file descriptor has to have been opened for appending. + +_Note: Specified file descriptors will not be closed automatically._ + +## fs.appendFileSync(file, data[, options]) The synchronous version of `fs.appendFile`. Returns `undefined`. diff --git a/lib/fs.js b/lib/fs.js index fef4f7ba50..76873be14a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -101,6 +101,10 @@ function nullCheck(path, callback) { return true; } +function isFd(path) { + return (path >>> 0) === path; +} + // Static method to set the stats properties on a Stats object. fs.Stats = function( dev, @@ -243,10 +247,18 @@ fs.readFile = function(path, options, callback_) { return; var context = new ReadFileContext(callback, encoding); + context.isUserFd = isFd(path); // file descriptor ownership var req = new FSReqWrap(); req.context = context; req.oncomplete = readFileAfterOpen; + if (context.isUserFd) { + process.nextTick(function() { + req.oncomplete(null, path); + }); + return; + } + binding.open(pathModule._makeLong(path), stringToFlags(flag), 0o666, @@ -257,6 +269,7 @@ const kReadFileBufferLength = 8 * 1024; function ReadFileContext(callback, encoding) { this.fd = undefined; + this.isUserFd = undefined; this.size = undefined; this.callback = callback; this.buffers = null; @@ -293,6 +306,14 @@ ReadFileContext.prototype.close = function(err) { req.oncomplete = readFileAfterClose; req.context = this; this.err = err; + + if (this.isUserFd) { + process.nextTick(function() { + req.oncomplete(null); + }); + return; + } + binding.close(this.fd, req); }; @@ -394,7 +415,8 @@ fs.readFileSync = function(path, options) { assertEncoding(encoding); var flag = options.flag || 'r'; - var fd = fs.openSync(path, flag, 0o666); + var isUserFd = isFd(path); // file descriptor ownership + var fd = isUserFd ? path : fs.openSync(path, flag, 0o666); var st; var size; @@ -404,7 +426,7 @@ fs.readFileSync = function(path, options) { size = st.isFile() ? st.size : 0; threw = false; } finally { - if (threw) fs.closeSync(fd); + if (threw && !isUserFd) fs.closeSync(fd); } var pos = 0; @@ -419,7 +441,7 @@ fs.readFileSync = function(path, options) { buffer = new Buffer(size); threw = false; } finally { - if (threw) fs.closeSync(fd); + if (threw && !isUserFd) fs.closeSync(fd); } } @@ -442,14 +464,15 @@ fs.readFileSync = function(path, options) { } threw = false; } finally { - if (threw) fs.closeSync(fd); + if (threw && !isUserFd) fs.closeSync(fd); } pos += bytesRead; done = (bytesRead === 0) || (size !== 0 && pos >= size); } - fs.closeSync(fd); + if (!isUserFd) + fs.closeSync(fd); if (size === 0) { // data was collected into the buffers list. @@ -1096,25 +1119,33 @@ fs.futimesSync = function(fd, atime, mtime) { binding.futimes(fd, atime, mtime); }; -function writeAll(fd, buffer, offset, length, position, callback_) { +function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) { var callback = maybeCallback(arguments[arguments.length - 1]); // write(fd, buffer, offset, length, position, callback) fs.write(fd, buffer, offset, length, position, function(writeErr, written) { if (writeErr) { - fs.close(fd, function() { + if (isUserFd) { if (callback) callback(writeErr); - }); + } else { + fs.close(fd, function() { + if (callback) callback(writeErr); + }); + } } else { if (written === length) { - fs.close(fd, callback); + if (isUserFd) { + if (callback) callback(null); + } else { + fs.close(fd, callback); + } } else { offset += written; length -= written; if (position !== null) { position += written; } - writeAll(fd, buffer, offset, length, position, callback); + writeAll(fd, isUserFd, buffer, offset, length, position, callback); } } }); @@ -1134,16 +1165,27 @@ fs.writeFile = function(path, data, options, callback_) { assertEncoding(options.encoding); var flag = options.flag || 'w'; + + if (isFd(path)) { + writeFd(path, true); + return; + } + fs.open(path, flag, options.mode, function(openErr, fd) { if (openErr) { if (callback) callback(openErr); } else { - var buffer = (data instanceof Buffer) ? data : new Buffer('' + data, - options.encoding || 'utf8'); - var position = /a/.test(flag) ? null : 0; - writeAll(fd, buffer, 0, buffer.length, position, callback); + writeFd(fd, false); } }); + + function writeFd(fd, isUserFd) { + var buffer = (data instanceof Buffer) ? data : new Buffer('' + data, + options.encoding || 'utf8'); + var position = /a/.test(flag) ? null : 0; + + writeAll(fd, isUserFd, buffer, 0, buffer.length, position, callback); + } }; fs.writeFileSync = function(path, data, options) { @@ -1158,7 +1200,9 @@ fs.writeFileSync = function(path, data, options) { assertEncoding(options.encoding); var flag = options.flag || 'w'; - var fd = fs.openSync(path, flag, options.mode); + var isUserFd = isFd(path); // file descriptor ownership + var fd = isUserFd ? path : fs.openSync(path, flag, options.mode); + if (!(data instanceof Buffer)) { data = new Buffer('' + data, options.encoding || 'utf8'); } @@ -1175,7 +1219,7 @@ fs.writeFileSync = function(path, data, options) { } } } finally { - fs.closeSync(fd); + if (!isUserFd) fs.closeSync(fd); } }; @@ -1192,6 +1236,11 @@ fs.appendFile = function(path, data, options, callback_) { if (!options.flag) options = util._extend({ flag: 'a' }, options); + + // force append behavior when using a supplied file descriptor + if (isFd(path)) + options.flag = 'a'; + fs.writeFile(path, data, options, callback); }; @@ -1203,9 +1252,14 @@ fs.appendFileSync = function(path, data, options) { } else if (typeof options !== 'object') { throwOptionsError(options); } + if (!options.flag) options = util._extend({ flag: 'a' }, options); + // force append behavior when using a supplied file descriptor + if (isFd(path)) + options.flag = 'a'; + fs.writeFileSync(path, data, options); }; diff --git a/test/parallel/test-fs-append-file-sync.js b/test/parallel/test-fs-append-file-sync.js index 42e0790e1f..4c5ae870ec 100644 --- a/test/parallel/test-fs-append-file-sync.js +++ b/test/parallel/test-fs-append-file-sync.js @@ -66,6 +66,19 @@ var fileData4 = fs.readFileSync(filename4); assert.equal(Buffer.byteLength('' + num) + currentFileData.length, fileData4.length); +// test that appendFile accepts file descriptors +var filename5 = join(common.tmpDir, 'append-sync5.txt'); +fs.writeFileSync(filename5, currentFileData); + +var filename5fd = fs.openSync(filename5, 'a+', 0o600); +fs.appendFileSync(filename5fd, data); +fs.closeSync(filename5fd); + +var fileData5 = fs.readFileSync(filename5); + +assert.equal(Buffer.byteLength(data) + currentFileData.length, + fileData5.length); + //exit logic for cleanup process.on('exit', function() { @@ -73,4 +86,5 @@ process.on('exit', function() { fs.unlinkSync(filename2); fs.unlinkSync(filename3); fs.unlinkSync(filename4); + fs.unlinkSync(filename5); }); diff --git a/test/parallel/test-fs-append-file.js b/test/parallel/test-fs-append-file.js index b20323bd14..01742aa6f8 100644 --- a/test/parallel/test-fs-append-file.js +++ b/test/parallel/test-fs-append-file.js @@ -92,11 +92,42 @@ fs.appendFile(filename4, n, { mode: m }, function(e) { }); }); +// test that appendFile accepts file descriptors +var filename5 = join(common.tmpDir, 'append5.txt'); +fs.writeFileSync(filename5, currentFileData); + +fs.open(filename5, 'a+', function(e, fd) { + if (e) throw e; + + ncallbacks++; + + fs.appendFile(fd, s, function(e) { + if (e) throw e; + + ncallbacks++; + + fs.close(fd, function(e) { + if (e) throw e; + + ncallbacks++; + + fs.readFile(filename5, function(e, buffer) { + if (e) throw e; + + ncallbacks++; + assert.equal(Buffer.byteLength(s) + currentFileData.length, + buffer.length); + }); + }); + }); +}); + process.on('exit', function() { - assert.equal(8, ncallbacks); + assert.equal(12, ncallbacks); fs.unlinkSync(filename); fs.unlinkSync(filename2); fs.unlinkSync(filename3); fs.unlinkSync(filename4); + fs.unlinkSync(filename5); }); diff --git a/test/parallel/test-fs-readfile-fd.js b/test/parallel/test-fs-readfile-fd.js new file mode 100644 index 0000000000..fc63d480ab --- /dev/null +++ b/test/parallel/test-fs-readfile-fd.js @@ -0,0 +1,47 @@ +'use strict'; +var common = require('../common'); +var assert = require('assert'); + +var path = require('path'), + fs = require('fs'), + fn = path.join(common.fixturesDir, 'empty.txt'); + +tempFd(function(fd, close) { + fs.readFile(fd, function(err, data) { + assert.ok(data); + close(); + }); +}); + +tempFd(function(fd, close) { + fs.readFile(fd, 'utf8', function(err, data) { + assert.strictEqual('', data); + close(); + }); +}); + +tempFdSync(function(fd) { + assert.ok(fs.readFileSync(fd)); +}); + +tempFdSync(function(fd) { + assert.strictEqual('', fs.readFileSync(fd, 'utf8')); +}); + +function tempFd(callback) { + fs.open(fn, 'r', function(err, fd) { + if (err) throw err; + + callback(fd, function() { + fs.close(fd, function(err) { + if (err) throw err; + }); + }); + }); +} + +function tempFdSync(callback) { + var fd = fs.openSync(fn, 'r'); + callback(fd); + fs.closeSync(fd); +} diff --git a/test/parallel/test-fs-write-file-sync.js b/test/parallel/test-fs-write-file-sync.js index 37373404da..72c0a2b19b 100644 --- a/test/parallel/test-fs-write-file-sync.js +++ b/test/parallel/test-fs-write-file-sync.js @@ -47,6 +47,18 @@ assert.equal('abc', content); assert.equal(mode, fs.statSync(file2).mode & mode); +// Test writeFileSync with file descriptor +var file3 = path.join(common.tmpDir, 'testWriteFileSyncFd.txt'); + +var fd = fs.openSync(file3, 'w+', mode); +fs.writeFileSync(fd, '123'); +fs.closeSync(fd); + +content = fs.readFileSync(file3, {encoding: 'utf8'}); +assert.equal('123', content); + +assert.equal(mode, fs.statSync(file3).mode & 0o777); + // Verify that all opened files were closed. assert.equal(0, openCount); diff --git a/test/parallel/test-fs-write-file.js b/test/parallel/test-fs-write-file.js index 9ff3b3f39a..a29e841ea2 100644 --- a/test/parallel/test-fs-write-file.js +++ b/test/parallel/test-fs-write-file.js @@ -69,11 +69,40 @@ fs.writeFile(filename3, n, { mode: m }, function(e) { }); }); +// test that writeFile accepts file descriptors +var filename4 = join(common.tmpDir, 'test4.txt'); +var buf = new Buffer(s, 'utf8'); + +fs.open(filename4, 'w+', function(e, fd) { + if (e) throw e; + + ncallbacks++; + + fs.writeFile(fd, s, function(e) { + if (e) throw e; + + ncallbacks++; + + fs.close(fd, function(e) { + if (e) throw e; + + ncallbacks++; + + fs.readFile(filename4, function(e, buffer) { + if (e) throw e; + + ncallbacks++; + assert.equal(Buffer.byteLength(s), buffer.length); + }); + }); + }); +}); process.on('exit', function() { - assert.equal(6, ncallbacks); + assert.equal(10, ncallbacks); fs.unlinkSync(filename); fs.unlinkSync(filename2); fs.unlinkSync(filename3); + fs.unlinkSync(filename4); });