Skip to content

Commit

Permalink
Implemented Syrus' Volume fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@torch2424
torch2424 committed Oct 15, 2019
1 parent 56fe3a0 commit aed9a61
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 1 deletion.
8 changes: 8 additions & 0 deletions src/__tests__/volume.test.ts
View file
Expand Up @@ -3,6 +3,7 @@ import { Link, Node } from '../node';
import Stats from '../Stats';
import Dirent from '../Dirent';
import { Volume, filenameToSteps, StatWatcher } from '../volume';
import { constants } from '../constants';
import hasBigInt from './hasBigInt';

describe('volume', () => {
Expand Down Expand Up @@ -282,6 +283,13 @@ describe('volume', () => {
expect(typeof fd).toBe('number');
expect(fd).toBeGreaterThan(0);
});
it('Create new directory at root (/)', () => {
vol.mkdirSync('/abc');
const fd = vol.openSync('/abc', constants.O_DIRECTORY);
// expect(vol.root.getChild('test.txt')).toBeInstanceOf(Link);
expect(typeof fd).toBe('number');
expect(fd).toBeGreaterThan(0);
});
it('Error on file not found', () => {
try {
vol.openSync('/non-existing-file.txt', 'r');
Expand Down
8 changes: 7 additions & 1 deletion src/volume.ts
View file
Expand Up @@ -904,7 +904,13 @@ export class Volume {
if (!realLink) throwError(ENOENT, 'open', link.getPath());

const node = realLink.getNode();
if (node.isDirectory() && flagsNum !== FLAGS.r) throwError(EISDIR, 'open', link.getPath());
if (node.isDirectory()) {
const isRead = flagsNum === FLAGS.r;
const isDir = (flagsNum & constants.O_DIRECTORY) !== 0;
if (!isRead && !isDir) {
throwError(EISDIR, 'open', link.getPath());
}
}

// Check node permissions
if (!(flagsNum & O_WRONLY)) {
Expand Down

3 comments on commit aed9a61

@corwin-of-amber
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @torch2424! I found this commit because I was also using memfs in my code and this deviation from upstream came up.
Is this a bug in memfs that can be suggested upstream? Or would this issue better be addressed by an adapter in wasmfs's path_open?
I kinda want to use memfs 3.x but then it's incompatible with your fork, which breaks everything.

@torch2424
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello! @corwin-of-amber Sorry for the late reply, we've been working really hard on a new feature! 😄

So I'm not entirely sure! The reaosn being, orgiinally this code was on a branch from @syrusakbary and I copied it over to have a single PR for the feature.

Also, we are doing a lot of very specific logic for Wamer-J. Thus, if we investigate the problem to be something that memfs can also benefit from, let's contribute upstream. Otherwise, we can do it in wasmfs, or in our memfs fork.

Let me know what you think 😄 👍

@torch2424
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait! I just saw: streamich#495

Thank you soooooo much for implementing this upstream! That is soooo cool! 😄 👍 🎉 I also appreciate the shoutout. Super awesome to see that!

Will continue the discussion on the latest thread 😄

Please sign in to comment.