fs_{read,write}: make offset argument optional and default it to -1

The magic -1 was not very user friendly and wasn't documented, and making it an optional argument makes the most sense to me. This is fully backwards compatible, and will also error on cases where the offset could be used as a callback, but the user probably didn't intend it to be the callback, like:

    uv.fs_read(fd, 32, function() end, function() end)

which will give the error

    bad argument luvit#3 to 'fs_read' (number expected, got function)

docs.md

@@ -2234,18 +2234,22 @@ Equivalent to `open(2)`. Access `flags` may be an integer or one of: `"r"`,
 opened in binary mode. Because of this, the `O_BINARY` and `O_TEXT` flags are
 not supported.
 
-### `uv.fs_read(fd, size, offset, [callback])`
+### `uv.fs_read(fd, size, [offset, [callback]])`
 
 **Parameters:**
 - `fd`: `integer`
 - `size`: `integer`
-- `offset`: `integer`
+- `offset`: `integer` or `nil`
 - `callback`: `callable` (async version) or `nil` (sync version)
   - `err`: `nil` or `string`
   - `data`: `string` or `nil`
 
 Equivalent to `preadv(2)`. Returns any data. An empty string indicates EOF.
 
+If `offset` is nil or omitted, it will default to `-1`, which indicates 'use and update the current file offset.'
+
+**Note:** When `offset` is >= 0, the current file offset will not be updated by the read.
+
 **Returns (sync version):** `string` or `fail`
 
 **Returns (async version):** `uv_fs_t userdata`
@@ -2264,18 +2268,22 @@ Equivalent to `unlink(2)`.
 
 **Returns (async version):** `uv_fs_t userdata`
 
-### `uv.fs_write(fd, data, offset, [callback])`
+### `uv.fs_write(fd, data, [offset, [callback]])`
 
 **Parameters:**
 - `fd`: `integer`
 - `data`: `buffer`
-- `offset`: `integer`
+- `offset`: `integer` or `nil`
 - `callback`: `callable` (async version) or `nil` (sync version)
   - `err`: `nil` or `string`
   - `bytes`: `integer` or `nil`
 
 Equivalent to `pwritev(2)`. Returns the number of bytes written.
 
+If `offset` is nil or omitted, it will default to `-1`, which indicates 'use and update the current file offset.'
+
+**Note:** When `offset` is >= 0, the current file offset will not be updated by the write.
+
 **Returns (sync version):** `integer` or `fail`
 
 **Returns (async version):** `uv_fs_t userdata`

src/fs.c

@@ -450,15 +450,24 @@ static int luv_fs_read(lua_State* L) {
   luv_ctx_t* ctx = luv_context(L);
   uv_file file = luaL_checkinteger(L, 1);
   int64_t len = luaL_checkinteger(L, 2);
-  int64_t offset = luaL_checkinteger(L, 3);
-  uv_buf_t buf;
+  // -1 offset means "the current file offset is used and updated"
+  int64_t offset = -1;
   int ref;
-  uv_fs_t* req;
+  // both offset and callback are optional
+  if (luv_is_callable(L, 3) && lua_isnoneornil(L, 4)) {
+    ref = luv_check_continuation(L, 3);
+  }
+  else {
+    offset = luaL_optinteger(L, 3, offset);
+    ref = luv_check_continuation(L, 4);
+  }
   char* data = (char*)malloc(len);
-  if (!data) return luaL_error(L, "Failure to allocate buffer");
-  buf = uv_buf_init(data, len);
-  ref = luv_check_continuation(L, 4);
-  req = (uv_fs_t*)lua_newuserdata(L, sizeof(*req));
+  if (!data) {
+    luaL_unref(L, LUA_REGISTRYINDEX, ref);
+    return luaL_error(L, "Failure to allocate buffer");
+  }
+  uv_buf_t buf = uv_buf_init(data, len);
+  uv_fs_t* req = (uv_fs_t*)lua_newuserdata(L, sizeof(*req));
   req->data = luv_setup_req(L, ctx, ref);
   // TODO: find out why we can't just use req->ptr for the base
   ((luv_req_t*)req->data)->data = buf.base;
@@ -477,8 +486,17 @@ static int luv_fs_unlink(lua_State* L) {
 static int luv_fs_write(lua_State* L) {
   luv_ctx_t* ctx = luv_context(L);
   uv_file file = luaL_checkinteger(L, 1);
-  int64_t offset = luaL_checkinteger(L, 3);
-  int ref = luv_check_continuation(L, 4);
+  // -1 offset means "the current file offset is used and updated"
+  int64_t offset = -1;
+  int ref;
+  // both offset and callback are optional
+  if (luv_is_callable(L, 3) && lua_isnoneornil(L, 4)) {
+    ref = luv_check_continuation(L, 3);
+  }
+  else {
+    offset = luaL_optinteger(L, 3, offset);
+    ref = luv_check_continuation(L, 4);
+  }
   uv_fs_t* req = (uv_fs_t*)lua_newuserdata(L, sizeof(*req));
   req->data = luv_setup_req(L, ctx, ref);
   size_t count;

tests/test-fs.lua

@@ -10,6 +10,25 @@ return require('lib/tap')(function (test)
     assert(uv.fs_close(fd))
   end)
 
+  test("read a file sync in chunks", function (print, p, expect, uv)
+    local fd = assert(uv.fs_open('README.md', 'r', tonumber('644', 8)))
+    local stat = assert(uv.fs_fstat(fd))
+    local chunks = {}
+    local numchunks = 8
+    local chunksize = math.ceil(stat.size/numchunks)
+    while true do
+      local chunk, err = uv.fs_read(fd, chunksize)
+      assert(not err, err)
+      if #chunk == 0 then
+        break
+      end
+      table.insert(chunks, chunk)
+    end
+    assert(#chunks == numchunks)
+    assert(#table.concat(chunks) == stat.size)
+    assert(uv.fs_close(fd))
+  end)
+
   test("read a file async", function (print, p, expect, uv)
     uv.fs_open('README.md', 'r', tonumber('644', 8), expect(function (err, fd)
       assert(not err, err)
@@ -32,10 +51,13 @@ return require('lib/tap')(function (test)
   test("fs.write", function (print, p, expect, uv)
     local path = "_test_"
     local fd = assert(uv.fs_open(path, "w", 438))
-    uv.fs_write(fd, "Hello World\n", -1)
-    uv.fs_write(fd, {"with\n", "more\n", "lines\n"}, -1)
-    uv.fs_close(fd)
-    uv.fs_unlink(path)
+    -- a mix of async and sync
+    uv.fs_write(fd, "Hello World\n", expect(function(err, n)
+      assert(not err, err)
+      assert(uv.fs_write(fd, {"with\n", "more\n", "lines\n"}))
+      assert(uv.fs_close(fd))
+      assert(uv.fs_unlink(path))
+    end))
   end)
 
   -- collect garbage after uv.fs_write but before the write callback
@@ -47,7 +69,7 @@ return require('lib/tap')(function (test)
     do
       -- the number here gets coerced into a string
       local t = {"with", 600, "lines"}
-      uv.fs_write(fd, t, -1, function()
+      uv.fs_write(fd, t, function()
         local expectedContents = table.concat(t)
         local stat = assert(uv.fs_fstat(fd))
         assert(stat.size == #expectedContents)