docker: ignore symlinks if already present#5
Conversation
| if(error && error.code != 'EEXIST') return callback(error) | ||
|
|
||
| fs.symlinkSync('/proc/mounts', '/etc/mtab') | ||
| var mtab = fs.lstatSync('/etc/mtab') // get stat of /etc/mtab |
There was a problem hiding this comment.
Do you mind using a try-catch instead of checking if it exists first? Besides that, you got the point ;-)
There was a problem hiding this comment.
Wouldnt it be better to check first if the symbolic link exists? And then create it.
In either way if we use docker the symbolic link wont be created, and on normal systems like qemu it creates the link.
I dont mind to try catch just fs.symlinkSync('/proc/mounts', '/etc/mtab') but for me it would be more like a bad manner.
There was a problem hiding this comment.
Wouldnt it be better to check first if the symbolic link exists? And then
create it.
I dont mind to try catch just fs.symlinkSync('/proc/mounts', '/etc/mtab')
but for me it would be more like a bad manner.
Au contraire mon ami, doing it this way you only access the filesystem one
time instead of two as would happen if you check first for the existence of
the file, and also it prevent a race condition. Remember, it's better to
say sorry than ask for permission ;-)
In either way if we use docker the symbolic link wont be created, and on
normal systems like qemu it creates the link.
Yes, that's the expected behaviour :-)
|
updated the PR |
| try { | ||
| fs.symlinkSync('/proc/mounts', '/etc/mtab') | ||
| } catch (e) { | ||
| if (e && e.code != 'EPERM') return callback(e) |
There was a problem hiding this comment.
Well... if there's an exception the e variable will always be set so there's no need to check for it's existence (left part of the conditional), but if you don't want to change it it's ok for me :-)
|
It was such a long time I forgot about this changes :-P Can you rebase them? I would gladly merge them later :-) If you want, you can be able to use async.parallel() using an object to map symlinks to targets and the async version of the symlink function since the |
|
@piranna okay after trying to understand the rebasing in git i think i got it. |
luii
changed the title
Cool! :-)
It would, but since we are using functions from the Node.js core library that by design don't use (and will not) use Promises, since we are already using |
|
@piranna you can merge it, and the for the async.parallel i make a new PR |
|
Merged and published as v0.1.6. |
2 participants
No description provided.