Implement a cross-platform synchronous API on top of the Unix back-end (bug 707679) and the Windows back-end (bug 707681). This API should work on only on chrome workers. Followup bug 729057 will deal with turning this synchronous API for workers in an asynchronous API for the main thread.

Comment on attachment 618660 [details] [diff] [review] Windows front-end A couple things. + write: function write(buffer, nbytes) { + this._ensureOpen(); + WinFile.WriteFile(this._fd, buffer, nbytes, gBytesWrittenPtr); + return gBytesWritten.value; + }, Why no error handling. Is WinFile.WriteFile throwing an exception via some declareFFI-created-wrapper? As I said before, I'm against changing behavior of wrapped functions in any way(beyond cdatafinalizer safety valve). Why is there worker/main-thread communication in declareFFI?

Comment on attachment 618660 [details] [diff] [review] Windows front-end One of the reasons I asked to start with minimal functionality is because adding more stuff just increases your chances making a mistake + File.truncate = function truncate(path, options) { + return generic_open(path, false, Const.TRUNCATE_EXISTING, options); + }; ^ wrong. Above is completely busted in presence of links(yes windows has those tool) and is much less efficient. The correct way to do this is to seek + SetFileSize ala http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.cpp#69 Please take out of all of APIs that are no strictly required from the higher level api(harder to make mistakes when doing mere wrappers).

Comment on attachment 618661 [details] [diff] [review] Unix front-end + setPositionFromEnd: function setPositionFromEnd(pos) { + this._ensureOpen(); + return UnixFile.lseek(this._fd, pos, Const.SEEK_END); + }, + I do not see utility in above & other setPosition*. it does nothing beyond obscuring the SEEK_END seek r-ing because this code needs to be rewritten to take out exception handling from low level functions

Comment on attachment 618662 [details] [diff] [review] Companion testsuite + try { + test_init(); + test_open_existing_file(); + test_open_non_existing_file(); + test_pump_existing_file(); + test_copy_existing_file(); + test_move_file(); + test_append_to_file(); + test_truncate_file(); + } catch (x) { + log("Catching error: " + x); + log("Stack: " + x.stack); + ok(false, x.toString() + "\n" + x.stack); + } Why not just let things fail?

(In reply to Taras Glek (:taras) from comment #5) > Comment on attachment 618660 [details] [diff] [review] > Windows front-end > > A couple things. > + write: function write(buffer, nbytes) { > + this._ensureOpen(); > + WinFile.WriteFile(this._fd, buffer, nbytes, gBytesWrittenPtr); > + return gBytesWritten.value; > + }, > Why no error handling. Is WinFile.WriteFile throwing an exception via some > declareFFI-created-wrapper? As I said before, I'm against changing behavior > of wrapped functions in any way(beyond cdatafinalizer safety valve). Indeed, I have incorporated error-checking in the Type passed to declareFFI. |Type.zero_or_nothing| informs |declareFFI| that 0 is an error and that the function returns an unspecified value otherwise. > Why is there worker/main-thread communication in declareFFI? Er, what?

(In reply to Taras Glek (:taras) from comment #6) > Comment on attachment 618660 [details] [diff] [review] > Windows front-end > > One of the reasons I asked to start with minimal functionality is because > adding more stuff just increases your chances making a mistake > + File.truncate = function truncate(path, options) { > + return generic_open(path, false, Const.TRUNCATE_EXISTING, options); > + }; > ^ wrong. Above is completely busted in presence of links(yes windows has > those tool) and is much less efficient. Gasp. So much for the Windows API documentation. > The correct way to do this is to seek + SetFileSize ala > http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/FileUtils.cpp#69 So three system calls instead of one is faster? Ok, I will fix that. > Please take out of all of APIs that are no strictly required from the higher > level api(harder to make mistakes when doing mere wrappers). Well, as I mentioned yesterday, I still have no scenario and no clear idea which APIs are "strictly required". The two bugs mentioned by Dietrich when I asked for his use cases seem to require only "read whole file asynchronously as String" and "copy file asynchronously", so if you want, I can just throw away my synchronous front-end and just provide these two functions. (In reply to Taras Glek (:taras) from comment #7) > I do not see utility in above & other setPosition*. it does nothing beyond > obscuring the SEEK_END seek Well, the idea is simply to have the same function name for Windows and Unix, which is the whole point of this synchronous front-end. Using libc constants SEEK_END et al. on a API that also works under Windows would be rather bad style. > r-ing because this code needs to be rewritten to take out exception handling > from low level functions (In reply to Taras Glek (:taras) from comment #8) > Comment on attachment 618662 [details] [diff] [review] > Companion testsuite [...] > Why not just let things fail? The idea is to get better stack reporting. I do not remember whether this is actually required, though.

> ^ wrong. Above is completely busted in presence of links(yes windows has > those tool) and is much less efficient. We continue to care about links? I did work in the nsifile stuff to support shortcuts - but was under the impression it was used very little (if not at all)

I mean shortcuts...

Shifted the exceptions from the back-end to the front-end, slightly changed the shape of the File object.

Exceptions moved from back-end to front-end, truncate function rewritten as asked. This code has not been tested yet, though.

(In reply to Doug Turner (:dougt) from comment #12) > I mean shortcuts... i meant windows hardlinks :)

Comment on attachment 618658 [details] [diff] [review] Front-end glue I'm leave this kind of stuff to dougt

Comment on attachment 619067 [details] [diff] [review] Unix front-end + close: function close() { + if (this._fd) { + let fd = this._fd; + this._fd = null; + delete this.fd; + Object.defineProperty(this, "fd", {get: File.prototype._nofd}); + return this._closeResult = WinFile.close(fd); + } WinFile, really? I think rather than having setPositionFromEnd..etc, provide a single .seek and take an argument for START, CURRENT, END or whatever you want to call them. for code like + let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR); + if (result == -1) { + throw new File.Error(); + } + return result; instead of cut'n'paste do return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where checkError would do the error checking boilerplate I'm not sure what Unix-style open is, your array of opens is more confusing than helpful. As I said before:instead of implementing everything, only implement functions as needed. Too much stuff here

(In reply to Taras Glek (:taras) from comment #18) > Comment on attachment 619067 [details] [diff] [review] > Unix front-end > > + close: function close() { > + if (this._fd) { > + let fd = this._fd; > + this._fd = null; > + delete this.fd; > + Object.defineProperty(this, "fd", {get: File.prototype._nofd}); > + return this._closeResult = WinFile.close(fd); > + } > > WinFile, really? Oops. Forgot to |qref| before uploading. > I think rather than having setPositionFromEnd..etc, provide a single .seek > and take an argument for START, CURRENT, END or whatever you want to call > them. Ok. I marginally prefer my solution, as it avoids having to add yet another set of constants, but that's not big deal. So, do you want me to put the constants in |OS.File|? In a subobject |OS.File.Constants|? > for code like > + let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR); > + if (result == -1) { > + throw new File.Error(); > + } > + return result; > > instead of cut'n'paste do > return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where > checkError would do the error checking boilerplate ok > I'm not sure what Unix-style open is, your array of opens is more confusing > than helpful. Well, that was the result of a design discussion with Dietrich, Paul O'Shannessy and Drew Willcoxon on the topic of OS.File API a few months ago, plus a more recent discussion with Jason Orendorff on the topic of OS-specific APIs. One of the main ideas is to avoid "one size doesn't quite fit anybody" APIs such as the file opening API of nspr/xpcom that require Unix-specific options even on Windows. Rather, we offer a set of APIs that cover the most common cases that can be made portable without performance penalty (that's "open", "truncate", "truncateC", ...), plus OS-specific APIs (prefixed by "unix" or "win"), to handle the myriad of cases in which no portable solution can be envisioned. Requires more documentation, of course. > As I said before:instead of implementing everything, only implement > functions as needed. > > Too much stuff here I fully agree with the general idea. As I mentioned, for the moment, I cannot do this until I have a clear idea of what is needed, so I am experimenting with nice API design, in the hope of obtaining something that I can show to potential users and get some feedback. It was my understanding that this was also something that you wanted. As I said before, if you could point me towards what is needed, that would be great. As I mentioned, for the moment, the only scenarios I have seen that do not involve a full API, both provided by Dietrich, are: 1/ read a full file to a string; 2/ copy a file. As I told you, scenario 1 is not really possible in the current state of js-ctypes, but I plan to work on patches to make it possible (bug 552551). So, if you prefer, as I wrote above, I can throw away all that code for the moment (or move it to another bug) and just provide file copy. What do you think, Taras?

Attaching a new version. ChangeLog: - factored the error-handling mechanism; - File.prototype.close now raises an error if |close| returns -1; - plenty more documentation and clarifications; - this time, the qref has been done :) - added several missing error checks; - now, all implementations of |copy| and |move| implement option |noOverwrite|. Now, Taras, as I mentioned, if you can just tell me which features you want in priority, I will be able to divide the patch in smaller chunks adding only these features.

(In reply to David Rajchenbach Teller [:Yoric] from comment #19) > (In reply to Taras Glek (:taras) from comment #18) > > Comment on attachment 619067 [details] [diff] [review] > > Unix front-end > > > > + close: function close() { > > + if (this._fd) { > > + let fd = this._fd; > > + this._fd = null; > > + delete this.fd; > > + Object.defineProperty(this, "fd", {get: File.prototype._nofd}); > > + return this._closeResult = WinFile.close(fd); > > + } > > > > WinFile, really? > > Oops. Forgot to |qref| before uploading. > > > I think rather than having setPositionFromEnd..etc, provide a single .seek > > and take an argument for START, CURRENT, END or whatever you want to call > > them. > > Ok. I marginally prefer my solution, as it avoids having to add yet another > set of constants, but that's not big deal. > > So, do you want me to put the constants in |OS.File|? In a subobject > |OS.File.Constants|? > > > > for code like > > + let result = UnixFile.lseek(this.fd, pos, Const.SEEK_CUR); > > + if (result == -1) { > > + throw new File.Error(); > > + } > > + return result; > > > > instead of cut'n'paste do > > return checkError(UnixFile.lseek(this.fd, pos, Const.SEEK_CUR)) where > > checkError would do the error checking boilerplate > > ok I still see explicit if(!...) thow Error code in new patches. > > > I'm not sure what Unix-style open is, your array of opens is more confusing > > than helpful. > > Well, that was the result of a design discussion with Dietrich, Paul > O'Shannessy and Drew Willcoxon on the topic of OS.File API a few months ago, > plus a more recent discussion with Jason Orendorff on the topic of > OS-specific APIs. > > One of the main ideas is to avoid "one size doesn't quite fit anybody" APIs > such as the file opening API of nspr/xpcom that require Unix-specific > options even on Windows. Rather, we offer a set of APIs that cover the most > common cases that can be made portable without performance penalty (that's > "open", "truncate", "truncateC", ...), plus OS-specific APIs (prefixed by > "unix" or "win"), to handle the myriad of cases in which no portable > solution can be envisioned. > > Requires more documentation, of course. I think this way insanity lies. I think we should do what common apis(python, fopen) do and specify file mode as a string. permissions can be an optional argument. I will strongly r- code with a bunch of similar looking open/create/etc functions. Having lots of really similar, but slightly different functions will increase the maintenance burden. > > > As I said before:instead of implementing everything, only implement > > functions as needed. > > > > Too much stuff here > > I fully agree with the general idea. As I mentioned, for the moment, I > cannot do this until I have a clear idea of what is needed, so I am > experimenting with nice API design, in the hope of obtaining something that > I can show to potential users and get some feedback. It was my understanding > that this was also something that you wanted. > > As I said before, if you could point me towards what is needed, that would > be great. As I mentioned, for the moment, the only scenarios I have seen > that do not involve a full API, both provided by Dietrich, are: > 1/ read a full file to a string; > 2/ copy a file. > > As I told you, scenario 1 is not really possible in the current state of > js-ctypes, but I plan to work on patches to make it possible (bug 552551). > > So, if you prefer, as I wrote above, I can throw away all that code for the > moment (or move it to another bug) and just provide file copy. > > What do you think, Taras? I don't understand why you can't do 1 atm.

Comment on attachment 619068 [details] [diff] [review] Windows front-end as i mentioned, too many variants of same function, error handling needs to be factored out.

Comment on attachment 619520 [details] [diff] [review] Unix front-end catch_negative -> throw_on_negative As I mentioned elsewhere seek/read/open/etc functions need to be unified. It's it's up to you where you put the seek, etc constants. If something is unclear, please chat with me before posting a revised patch.

(In reply to Taras Glek (:taras) from comment #21) > > What do you think, Taras? > > I don't understand why you can't do 1 atm. Let's discuss this on irc. Essentially, two reasons: - no support for encodings other than utf-8; - no support for improperly encoded strings (i.e. typically what you get from one |read| operation).

(In reply to Taras Glek (:taras) from comment #23) > Comment on attachment 619520 [details] [diff] [review] > Unix front-end > > catch_negative -> throw_on_negative You are right, that's clearer. > I think this way insanity lies. I think we should do what common apis(python, > fopen) do and specify file mode as a string. permissions can be an optional > argument. [...] Permissions are not an issue. Well, besides the fact that they are very much not portable between Unix and Windows, which I solve with the |options| object. > As I mentioned elsewhere seek/read/open/etc functions need to be unified. > It's it's up to you where you put the seek, etc constants. [...] > I will strongly r- code with a bunch of similar looking open/create/etc > functions. Having lots of really similar, but slightly different functions will > increase the maintenance burden. Ok. We are trading a little robustness in the API for a little robustness in the implementation, but this remains reasonable. > If something is unclear, please chat with me before posting a revised patch. Just to clarify: I posted the revised patch just to have a more readable base for discussion.

Comment on attachment 624015 [details] [diff] [review] Windows front-end + if (options && options.from) { That's wrong when from is 0. .hasOwnProperty('from') or typeof options.from =='int' + throw_on_zero( + WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null) + ); + this.setPosition(pos); + } else { + throw_on_zero( + WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null) + ); This is a weird api...read() should always set the file position after what was read in. same with writeI)...especially write write has same problems I'm still not sure what the point of winOpen is. + case undefined: + case 'r': add // fall through when falling through a case on purpose there is no default handler when you pass in a mode that cant be parsed..it should throw an exception. I also doubt that passing modes as a string is the right thing to do(even if i suggested it). This is not C, you can pass an array of [Const.* modes] and skip the parsing step. + WinFile.chdir(path); //FIXME: Not implemented ???? GetCurrentDirectory loop seems crazy, should expose MAX_PATH and use that + 1 for file length Also, allocating on with a +1 buffer increase is crazy inefficient. I don't see the point of the pump function...even if i were convinced that we need pumping I would say that buffer allocation should be the responsibility of the user(allows more buffer reuse and feature less fragile state)

Comment on attachment 624014 [details] [diff] [review] Unix front-end Lets get one platform r+ed and then we can r+ the other...I prefer to review windows to start with.

Comment on attachment 624013 [details] [diff] [review] Companion testsuite This is useful for reference, but i don't need to review this until we have r+ed the api.

(In reply to Taras Glek (:taras) from comment #33) > Comment on attachment 624015 [details] [diff] [review] > Windows front-end > > + if (options && options.from) { > That's wrong when from is 0. .hasOwnProperty('from') or typeof options.from > =='int' Right. > + throw_on_zero( > + WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null) > + ); > + this.setPosition(pos); > + } else { > + throw_on_zero( > + WinFile.ReadFile(this.fd, buffer, nbytes, gBytesReadPtr, null) > + ); > > This is a weird api...read() should always set the file position after what > was read in. > > > same with writeI)...especially write > write has same problems I was attempting to provide |pread|/|pwrite|-like functionality. If this is useless, I can drop that. > I'm still not sure what the point of winOpen is. Well, the idea is to let users access a lower-level API, by providing a function that can be used exactly as the Windows API. Typical use would look like let file; if (OS.Win) { file = OS.Win.winOpen(/*my exact args, I know what I am doing*/) } else if (OS.Unix) { file = OS.Unix.unixOpen(/*my exact args, I know what I am doing*/) } // ... Now, since I have added options winShare, winSecurity, etc., we can decide that this feature is redundant. > > + case undefined: > + case 'r': > > add // fall through > when falling through a case on purpose > > there is no default handler when you pass in a mode that cant be parsed..it > should throw an exception. Good catch. In the Unix version, it throws a |TypeError|, but I obviously forgot to put this in the Windows version. > I also doubt that passing modes as a string is the right thing to do(even if > i suggested it). This is not C, you can pass an array of [Const.* modes] and > skip the parsing step. Mmmhh... Ok. Not the actual |Const| modes (these wouldn't be portable), but I can do something. Note that this will end up much more verbose: OS.File.open("foo", [OS.File.open.APPEND, OS.File.open.RW, OS.File.open.CREATE]) or something such. This will not really simplify the parsing step (we still need argument validation), but it might be easier on users. We might also, more or less equivalently, introduce something along the lines of OS.File.open("foo", {append: true, rw: true, create: true}) A little easier on the eyes, but it is also easier to introduce a typo. > > + WinFile.chdir(path); //FIXME: Not implemented ???? Thanks. > > GetCurrentDirectory loop seems crazy, should expose MAX_PATH and use that + > 1 for file length Unfortunately, under Windows, MAX_PATH is *not* the maximal length for a path. The maximal length for a path is "approximately" (sic) 32768 (doc is fuzzy, but it seems that this can be 32768 + the name of the drive or share), while the value of MAX_PATH, iirc, is 4095. I do not think that we want to allocate 32kb each time we request a path, so we have to try and be smarter. > Also, allocating on with a +1 buffer increase is crazy inefficient. Good thing I am not doing it, then :) I am allocating with the value returned by |GetCurrentDirectory|, which is the required size of the buffer. Rewriting the algorithm to make this clearer. > I don't see the point of the pump function...even if i were convinced that > we need pumping I would say that buffer allocation should be the > responsibility of the user(allows more buffer reuse and feature less fragile > state) In that case, let's remove it. The Linux version needs it to implement copy (using splice), but we can probably do without on other platforms.

Attaching an updated patch. Removed winOpen and pump, fixed option.from in read/write, added some documentation on read/write, changed handling of modes in open, clarified get current directory.

Comment on attachment 624323 [details] [diff] [review] Windows front-end + + let gBytesRead = new ctypes.int32_t(-1); + let gBytesReadPtr = gBytesRead.address(); + let gBytesWritten = new ctypes.int32_t(-1); + let gBytesWrittenPtr = gBytesWritten.address(); these should be 'private' with _ prefix, right? I don't see a reason to expose these. > > I was attempting to provide |pread|/|pwrite|-like functionality. If this is > useless, I can drop that. > drop that > > I'm still not sure what the point of winOpen is. > > Well, the idea is to let users access a lower-level API, by providing a > function that can be used exactly as the Windows API. > Usually you let apps use a native open..and provide functionality to import the fd..we can add that later when we actually need it > We might also, more or less equivalently, introduce something along the > lines of > > OS.File.open("foo", {append: true, rw: true, create: true}) > > A little easier on the eyes, but it is also easier to introduce a typo. > Lets do that, lets also add validation such that if a bonus/unexpected options argument is passed, we throw an exception. We should do the options validation in a followup pump didn't get removed

(In reply to Taras Glek (:taras) from comment #38) > Comment on attachment 624323 [details] [diff] [review] > Windows front-end > > + > + let gBytesRead = new ctypes.int32_t(-1); > + let gBytesReadPtr = gBytesRead.address(); > + let gBytesWritten = new ctypes.int32_t(-1); > + let gBytesWrittenPtr = gBytesWritten.address(); > > these should be 'private' with _ prefix, right? I don't see a reason to > expose these. They are actually private, just as closure-local variables, rather than as per-file fields. I am taking advantage of the fact that two concurrent read/write cannot take place concurrently to save (a little) memory and (a little) time. > > I was attempting to provide |pread|/|pwrite|-like functionality. If this is > > useless, I can drop that. > > > > drop that Done. > > > I'm still not sure what the point of winOpen is. > > > > Well, the idea is to let users access a lower-level API, by providing a > > function that can be used exactly as the Windows API. > > > > Usually you let apps use a native open..and provide functionality to import > the fd..we can add that later when we actually need it Done. > > We might also, more or less equivalently, introduce something along the > > lines of > > > > OS.File.open("foo", {append: true, rw: true, create: true}) > > > > A little easier on the eyes, but it is also easier to introduce a typo. > > > > Lets do that, lets also add validation such that if a bonus/unexpected > options argument is passed, we throw an exception. We should do the options > validation in a followup Let me attach a version that uses an object, with some option validation. > pump didn't get removed Oops. Done.

Thanks for the quick review. Here is a new (untested) version.

Comment on attachment 624495 [details] [diff] [review] Windows front-end This seems ok

I have updated the Windows and Unix front-ends to put them at the same level of features. Along the way, I have found a few issues in the Windows front-end, so if you want a re-review, here is the update: - more documentation; - removed some debugging code; - added two |because*| getters to determine error reasons; - a few |let| => |const|; - added a mode flag |existing| for |open|, to open the file only if it exists; - reworked the code that handles mode for |open| into something a little less ad-hoc; - handled "truncate an existing file", as per your earlier recommendations.

Comment on attachment 626449 [details] [diff] [review] Windows front-end + case "existing": + existing = true; + default: missing break What's with because in the property names? Seems like they would be just as good without, no?

Comment on attachment 626450 [details] [diff] [review] Unix front-end + case "existing": + existing = true; + default: + throw new TypeError("Mode " + key + " not understood"); + } + } Same bug as in windows frontend. This leads me to believe that you should factor out the open argument parsing and share it.(post a followup patch to the windows code so dont have to rereview the whole thing) I image it would take mode and return an object with members for + let read = false; + let write = false; + let trunc = false; + let create = false; + let existing = false; is the pump fallback useful on any platform in current code? splice can fail even if it is available: EINVAL Target file system doesn't support splicing; target file is opened in append mode; neither of the descriptors refers to a pipe; or offset given for nonseekable device. We need to work on filesystems too lazy to implement splice(fallback on sendfile?) Also I'm ok with landing code that only works within a single (decent) filesystem and them applying fallbacks as a followup(ie for move, copy, splice)

(In reply to Taras Glek (:taras) from comment #55) > Comment on attachment 626449 [details] [diff] [review] > Windows front-end > > + case "existing": > + existing = true; > + default: > > > missing break Thanks. > What's with because in the property names? Seems like they would be just as > good without, no? Sorry, I don't understand the question.

(In reply to Taras Glek (:taras) from comment #56) > Comment on attachment 626450 [details] [diff] [review] > Unix front-end > > + case "existing": > + existing = true; > + default: > + throw new TypeError("Mode " + key + " not understood"); > + } > + } > > Same bug as in windows frontend. This leads me to believe that you should > factor out the open argument parsing and share it.(post a followup patch to > the windows code so dont have to rereview the whole thing) > > > I image it would take mode and return an object with members for > + let read = false; > + let write = false; > + let trunc = false; > + let create = false; > + let existing = false; > Ok, I'll find a way to factor this out. > is the pump fallback useful on any platform in current code? I do not think it is necessary for any tier one platform. I included this because I am pretty sure that there are some Unices that implement neither |splice| nor |copyfile|, and I wanted to have a fallback. > splice can fail even if it is available: > EINVAL Target file system doesn't support splicing; target file is opened > in append mode; neither of the descriptors refers to a pipe; or offset given > for nonseekable device. > > > We need to work on filesystems too lazy to implement splice(fallback on > sendfile?) Should work on Linux, but I'm not 100% confident that it will work on other OSes. For instance, on BSD, I believe that |sendfile| is only for sockets. > Also I'm ok with landing code that only works within a single (decent) > filesystem and them applying fallbacks as a followup(ie for move, copy, > splice) Ok.

Is this what you had in mind?

Comment on attachment 626449 [details] [diff] [review] Windows front-end I'll ask dougt to review this once we have a final version.

Comment on attachment 626749 [details] [diff] [review] Windows front-end, continued I would like to get rid of discrete variables in this. + let read = false; + let write = false; + let trunc = false; + let create = false; + let existing = false; -> let ret = { read:false, write:false... } + case "truncate": + trunc = true; + write = true; -> ret.trunk -> true ... return ret let { + read, write, trunc, create, existing + } = OS.Shared._aux.normalizeOpenMode(options); let mode = OS.Shared._aux.normalizeOpenMode(options); but otherwise looks good

Here we go. I will fold the patches a little differently once I have your approval, Taras.

Applying requested changes to Unix front-end, too. Again, I will do some re-folding somewhere along the way.

This function proved useful both for part 2 of the Unix front-end and for the ongoing work on thumbnails.

Comment on attachment 628286 [details] [diff] [review] Scope-based resource control 10:56 <@taras> and i think instead of having magic cleanup function 10:56 <@taras> and passing resources 10:56 <@taras> you should just pass in 2 functions 10:56 <@taras> cleanup, action 10:57 <@taras> otherwise too much magic is happening in cleanup(), afaik 10:57 < Yoric> Well, I have the feeling that the common scenario is either: 10:57 < Yoric> - closing one file; 10:57 < Yoric> - or closing many files. 10:57 <@taras> so you make a function called try_finally 10:57 <@taras> and try_finally_fd 10:57 <@taras> for the common scenario 10:57 < Yoric> ok 10:57 <@taras> and try_finally_fsls for the other one 10:58 <@taras> err 10:58 <@taras> try_finally_fdls it's not obvious what using does from name. I propose try_finally

Unix front-end, consolidated in one patch.

Shared code, extracted from its patch.

Windows front-end, consolidated in one patch.

Comment on attachment 626452 [details] [diff] [review] Front-end glue And a simpler review to rest a little bit after the complex stuff :)

Comment on attachment 628734 [details] [diff] [review] Windows front-end Review of attachment 628734 [details] [diff] [review]: ----------------------------------------------------------------- on mac and unix, paths use '/' as the file node seperator. on windows, it is '\'. Suppose I need to use this API, and say -- want to delete a file that exists at $HOME/doomtext. How do we do that in an cross platform way? ::: toolkit/components/osfile/osfile_win_front.jsm @@ +42,5 @@ > + // and we use the following global mutable values: > + let gBytesRead = new ctypes.int32_t(-1); > + let gBytesReadPtr = gBytesRead.address(); > + let gBytesWritten = new ctypes.int32_t(-1); > + let gBytesWrittenPtr = gBytesWritten.address(); these are no more global than WinFile declared above, right? These are basically instance member variables. You probably should drop the 'g' prefix. Maybe use 'm' instead? @@ +56,5 @@ > + */ > + let File = function File(fd) { > + this._fd = fd; > + }; > + File.prototype = { add newline whitespace before File.prototype. @@ +69,5 @@ > + // Placeholder getter, used to replace |get fd| once > + // the file is closed. > + _nofd: function nofd(operation) { > + operation = operation || > + this._nofd.caller.name || This is different than the unix one. You added this._nofd.caller.name here. Which is correct? @@ +88,5 @@ > + close: function close() { > + if (this._fd) { > + let fd = this._fd; > + this._fd = null; > + delete this.fd; why the explict delete here? @@ +118,5 @@ > + * the end of the file has been reached. > + * @throws {OS.File.Error} In case of I/O error. > + */ > + read: function read(buffer, nbytes, options) { > + // |gBytesReadPtr| is a pointer to |gBytesRead|. useless comment. @@ +175,5 @@ > + // In this implementation, > + // OS.File.POS_START == OS.Constants.Win.FILE_BEGIN > + // OS.File.POS_CURRENT == OS.Constants.Win.FILE_CURRENT > + // OS.File.POS_END == OS.Constants.Win.FILE_END > + whence = (whence == undefined)?Const.FILE_BEGIN:whence; spaces between operators @@ +322,5 @@ > + > + let share = options.winShare || DEFAULT_SHARE; > + let security = options.winSecurity || null; > + let flags = options.winFlags || DEFAULT_FLAGS; > + let template = options.winTemplate?options.winTemplate._fd:null; same - add spaces around ? and :

Comment on attachment 628727 [details] [diff] [review] Unix front-end Review of attachment 628727 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/osfile_unix_front.jsm @@ +397,5 @@ > + }; > + } else { > + // If the OS does not implement file copying for us, we need to > + // implement it ourselves. For this purpose, we need to define > + // a pumping function. What platform would this code run on? What platforms do not have copyfile(3)?

Fixing a few typoes.

Comment on attachment 630983 [details] [diff] [review] Windows front-end Review of attachment 630983 [details] [diff] [review]: ----------------------------------------------------------------- r with those changes. ::: toolkit/components/osfile/Makefile.in @@ +26,5 @@ > $(NSINSTALL) $(srcdir)/osfile_shared.jsm $(FINAL_TARGET)/modules/osfile > $(NSINSTALL) $(srcdir)/osfile_unix_back.jsm $(FINAL_TARGET)/modules/osfile > $(NSINSTALL) $(srcdir)/osfile_unix_front.jsm $(FINAL_TARGET)/modules/osfile > $(NSINSTALL) $(srcdir)/osfile_win_back.jsm $(FINAL_TARGET)/modules/osfile > + $(NSINSTALL) $(srcdir)/osfile_win_front.jsm $(FINAL_TARGET)/modules/osfile \ No newline at end of file \ No newline at end of file ::: toolkit/components/osfile/osfile_win_front.jsm @@ +333,5 @@ > + ||(!("winAccess" in options) && "winDisposition" in options)) { > + throw new TypeError("OS.File.open requires either both options " + > + "winAccess and winDisposition or neither"); > + } else { > + mode = OS.Shared._aux.normalizeOpenMode(mode); Declare 'mode' above with the rest.

Comment on attachment 630986 [details] [diff] [review] Unix front-end I am unsure where UnixFile.copyfile isn't going to be defined. Why do we need this code?

(In reply to Doug Turner (:dougt) from comment #75) > Comment on attachment 630986 [details] [diff] [review] > Unix front-end > > I am unsure where UnixFile.copyfile isn't going to be defined. Why do we > need this code? UnixFile.copyfile is only defined under BSD/MacOS X. For all other platforms, we need to reimplement it.

(In reply to Doug Turner (:dougt) from comment #74) > Comment on attachment 630983 [details] [diff] [review] > Windows front-end > > Review of attachment 630983 [details] [diff] [review]: > ----------------------------------------------------------------- > > r with those changes. > > ::: toolkit/components/osfile/Makefile.in > > \ No newline at end of file Done. > > ::: toolkit/components/osfile/osfile_win_front.jsm > @@ +333,5 @@ > > + ||(!("winAccess" in options) && "winDisposition" in options)) { > > + throw new TypeError("OS.File.open requires either both options " + > > + "winAccess and winDisposition or neither"); > > + } else { > > + mode = OS.Shared._aux.normalizeOpenMode(mode); > > Declare 'mode' above with the rest. Are you sure? I am just normalizing an argument. Declaring it might make code a little messier.

Ahaha, everything is reviewed. I am tracking down an intermittent orange. I will land this once I have found it.

Will land.

https://hg.mozilla.org/integration/fx-team/rev/3cde4e8722eb https://hg.mozilla.org/integration/fx-team/rev/9e4b5966a2e8 https://hg.mozilla.org/integration/fx-team/rev/03f35867f598 https://hg.mozilla.org/integration/fx-team/rev/4f0670e0828a https://hg.mozilla.org/integration/fx-team/rev/06ae8c3e0898 https://hg.mozilla.org/integration/fx-team/rev/4adb8c310f9d https://hg.mozilla.org/integration/fx-team/rev/4ef5ee480ddd

https://hg.mozilla.org/mozilla-central/rev/3cde4e8722eb https://hg.mozilla.org/mozilla-central/rev/9e4b5966a2e8 https://hg.mozilla.org/mozilla-central/rev/03f35867f598 https://hg.mozilla.org/mozilla-central/rev/4f0670e0828a https://hg.mozilla.org/mozilla-central/rev/06ae8c3e0898 https://hg.mozilla.org/mozilla-central/rev/4adb8c310f9d https://hg.mozilla.org/mozilla-central/rev/4ef5ee480ddd

re https://hg.mozilla.org/mozilla-central/rev/9e4b5966a2e8, i'm afraid the copyfile API is OSX only. There's no copyfile.h on OpenBSD, and after a bit of googling there doesnt seem to be one on FreeBSD either. I'm pretty sure it should be included only in the XP_MACOSX case..

from http://www.manpagez.com/man/3/copyfile/ HISTORY The copyfile() API was introduced in Mac OS X 10.5.

Only include copyfile.h on mac, tests passes fine here now : /usr/obj/m-c/ $TEST_PATH=toolkit/components/osfile/tests/mochi/ gmake mochitest-chrome ... 201 INFO Passed: 195 202 INFO Failed: 0 ...

Thanks for the quick patch, gaston.

http://hg.mozilla.org/integration/mozilla-inbound/rev/b9981690ede0 (and indeed, i've put the wrong bug # in the commit message... DOH!)

https://hg.mozilla.org/mozilla-central/rev/b9981690ede0 Landry, I don't suppose you could put a note in your calendar to add a reference pointing to here in bug 767876 once it exists? Would just help hg blame :-)

(In reply to Ed Morley [:edmorley] from comment #99) > https://hg.mozilla.org/mozilla-central/rev/b9981690ede0 > > Landry, I don't suppose you could put a note in your calendar to add a > reference pointing to here in bug 767876 once it exists? Would just help hg > blame :-) I'd have if i could.. but the now-existing bug 767876 is 'access denied' for everyone apparently :(