fs,doc,test: open reserved characters under win32

[XadillaX]

Explain the behavior of fs.open() under win32 that file path contains
some characters and add some test cases for them.

• < (less than)
• > (greater than)
• : (colon)
• " (double quote)
• / (forward slash)
• \ (backslash)
• | (vertical bar or pipe)
• ? (question mark)
• * (asterisk)

Refs: #13868
Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
Refs: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx

Checklist

• make -j4 test (UNIX)
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

fs, doc, test

[mscdex]

I'm not sure this extra closure is needed. Perhaps it could just be process.nextTick(req.oncomplete, err).

[XadillaX]

@mscdex sometimes req.oncomplete depends on req, eg.

  var context = new ReadFileContext(callback, options.encoding);
  context.isUserFd = isFd(path); // file descriptor ownership
  var req = new FSReqWrap();
  req.context = context;
  req.oncomplete = readFileAfterOpen;

...

function readFileAfterOpen(err, fd) {
  var context = this.context;

  ...
}

[mscdex]

I see. Perhaps we could at least factor out the callback then, like:

process.nextTick(onWinOpenErrorNT, req, err);

// ...

function onWinOpenErrorNT(req, err) {
  req.oncomplete(err);
}

[mscdex]

I wonder if having two separate implementations, one for posix and one for Windows, would help any performance-wise? I think it definitely would on node v6.x, which still uses Crankshaft and would possibly allow the posix version to be inlineable.

[XadillaX]

@mscdex Now only one call stack is added, and an if statement added. I think it won't slow down too much.

[mscdex]

If we're going to start explicitly checking for these, perhaps we should be checking for all of the reserved characters, either here in JS or at the libuv layer (via a flag of some kind if not enforced all the time for Windows).

EDIT: I just saw your comment about the other reserved characters. I'm wondering if we actually test for those on Windows nodes in CI...

[silverwind]

Did you test other invalid characters and/or filenames (like com, prn)? Windows has quite peculiar rules on which filenames are considered valid, ideally I think we should test them all:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#Naming_Conventions

[XadillaX]

Did you test other invalid characters and/or filenames (like com, prn)? Windows has quite peculiar rules on which filenames are considered valid, ideally I think we should test them all:

https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx#Naming_Conventions

Not yet.

I will try it tomorrow, now it's 1:11 a.m. at my location and I'm going to sleep.

[XadillaX]

@mscdex @silverwind Sorry, I'm going to take a vacation for one week and now I don't have a Windows computer at my hand. So maybe I will continue to work for this PR after one week.

[cjihrig]

Maybe the stringToFlags() calls should be pushed into openWrap() too.

[cjihrig]

Style nit: single line if statements shouldn't have curly braces.

[cjihrig]

I think the errors in this file are sorted by error code.

[cjihrig]

I don't think you have to add this for new files.

[cjihrig]

Can you use common.skip() here.

[cjihrig]

Can you add common.mustCall() to the readFile() and writeFile() callbacks in this file.

[cjihrig]

Please use ^ and $ to match the full error message.

[silverwind]

I agree with @mscdex, if we do this checking, it's better done in libuv.

Of course, it's also possible to delegate the task of checking if a filename is valid for a platform to the user, for example through modules like https://github.com/sindresorhus/valid-filename.

[XadillaX]

@silverwind To be honest, I think libuv's PR progress is much slower than Node.js'.

I will continue to work for other reserved names by implement a checking function next week after my vacation.

[refack]

I will continue to work for other reserved names by implement a checking function next week after my vacation.

Enjoy 🍹

[bzoz]

This file:smth is an NTFS feature, using > etc in filenames is already correctly handled by Node. Please see my comment in the original issue..

[bzoz]

Oh, forgot to add that I'm -1 on this.

[refack]

Oh, forgot to add that I'm -1 on this.

@bzoz It has an element of surprise (alternative streams I personally keep forgetting about). Should we at least document this? Or maybe add special syntax for?

[bzoz]

IMHO we should document this. Special syntax would be surprising for everyone that already knows this feature. It would also add maintenance burden.

[XadillaX]

@bzoz so what I should do is just to document it?

[bzoz]

I guess so.

The streams feature of NTFS is surprising, we can mention it in the docs and add a link to MSDN: https://msdn.microsoft.com/en-us/library/windows/desktop/bb540537.aspx

[XadillaX]

I'll update this issue to document this feature after I'm back.

[XadillaX]

@cjihrig, @refack, @bzoz, I've updated this PR to explain the behavior.