Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support fs.open() flags being a number vs. string #747

Open
tinchoz49 opened this issue Mar 7, 2019 · 5 comments
Open

Support fs.open() flags being a number vs. string #747

tinchoz49 opened this issue Mar 7, 2019 · 5 comments
Labels

Comments

@tinchoz49
Copy link

Hi everyone!

I'm trying to do this and I'm getting flags is not valid:

fs.open("/file.txt", fs.constants.O_RDWR | fs.constants.O_CREAT, function(err, fd) {
  if(err) throw err;
  fs.fstat(fd, function(err, stats) {
    if(err) throw err;
    fs.close(fd);
  });
});
@humphd
Copy link
Contributor

humphd commented Mar 7, 2019

Hey @tinchoz49, this is a bug, thanks for filing. We currently assume flags are in string form ('w+' is closest to what you'd want), and don't yet support passing flags as a bitwise number. We should.

Fixing this would involve making our code do what node does:

A simple fix would be to alter our O_FLAGS object to use numeric keys vs. strings, and convert strings to numbers like node does.

@humphd humphd changed the title flags is not valid Support fs.open() flags being a number vs. string Mar 7, 2019
@humphd humphd added the bug label Mar 7, 2019
@humphd
Copy link
Contributor

humphd commented Mar 8, 2019

@rscotchmer not sure what you're looking to do for your 0.3 bugs, but this one might be interesting to you. I could mentor you through it, if you have interest. No pressure if you have other things you want to try next.

@CDNRae
Copy link
Contributor

CDNRae commented Mar 8, 2019

@humphd I'd love to work on this! Thanks for notifying me!

@humphd
Copy link
Contributor

humphd commented Mar 8, 2019

@rscotchmer great, here are some more thoughts.

We currently use strings for flags, and we should switch to using numbers. If a string is passed to us, we should convert it into a number. See the code I linked to above from node, which does what we want.

Next, we do a bunch of checks all over like this: https://github.com/filerjs/filer/blob/master/src/filesystem/implementation.js#L622. Instead of doing this, we'd want to convert that to use the flags number, and bitwise & (and) it with the constant we want to check against. The constants we want are defined in https://github.com/filerjs/filer/blob/master/src/constants.js#L88.

We could start by converting the code in the first comment above into a test we add to the fs.open.spec.js test file, and add more as we go. I have no doubt you'll find other bugs while fixing this, since we probably don't do the right thing in all cases with flags vs. what node does.

Let me know what questions you have, or further help I can give. I'm happy to guide you as you deem necessary.

@tinchoz49
Copy link
Author

Hey everyone, thanks to start working on this!

@CDNRae CDNRae mentioned this issue Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants