baudehlo / node-fs-ext

Extras missing from node's fs module
MIT License
101 stars 45 forks source link

Update seek() for large file support (>32 bit position values) #3

Closed tshinnic closed 13 years ago

tshinnic commented 13 years ago

First it was "wait, this ain't right, what about large file support?"

    seek_data->offset = args[1]->Int32Value();

and then "returned offsets aren't right either!"

   off_t i = lseek(seek_data->fd, seek_data->offset, seek_data->oper);
     req->result = i;

because

   ssize_t result;  /* result of syscall, e.g. result = read (... */

(ssize_t is not as big as off_t, which might really be __off64_t)

Then I went off to look at how write()/read() did position values, and they were wrong in their definition of GET_OFFSET.

Then I looked all over the node source and library source and found references/definitions which showed another way of fetching the values for positions was needed.

Then I looked around in the v8 sources and found IntegerValue().

Then I looked around and found two different threads on nodejs where people were complaining about the lack of support for files over 2GB.

On the verge of rolling my own fix, I then trawled through the pull requests for node.js and found one addressing all this that was only 3 days old! ( https://github.com/joyent/node/pull/1199 )

Read the discussion there and the proposed code. Seems quite excessive care for 'integer-ness', and exceptions to boot! But if bnoordhuis and ry like it, yes?

Anyway, I've adopted/adapted that code to fit the seek() code. As the other comments and threads say, this provides over 48 bits, perhaps as much as 53 bits. Into petabyte territory is better than mere gigabytes.

Oh, and then I had to figure out how to return >32 bit integer values! return scope.Close(Number::New(offs_result));

Description in commit:

Realized that the code was neither receiving nor returning values larger than 32 bits. Much investigation ended up at pull request https://github.com/joyent/node/pull/1199 which discussed and illustrated acceptable code for large integer arguments.

Testing found that returned values were being truncated, and so changed to using Number::New() to allow larger than 32 bit integer return values.

Tests for seek() updated to check for non-integer values and for position values larger than 32 bits.

baudehlo commented 13 years ago

Wow, you're a fucking rock star :)

I had thought about this at the time but dismissed it as "I'll deal with this later - broken code is better than no code".

Nice one!