Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A couple of fixes; same as moby/moby#35618
1. MkdirAllNewAs: error out if dir exists as file
os.MkdirAll()
function returns "not a directory" error in case adirectory to be created already exists but is not a directory
(e.g. a file). MkdirAllNewAs function do not replicate the behavior.
This is a bug since it is expected to ensure the required directory
exists and is indeed a directory, and return an error otherwise.
2. idtools: fix MkdirAll usage
This subtle bug keeps lurking in because error checking for
Mkdir()
and
MkdirAll()
is slightly different wrtEEXIST
/IsExist
:for
Mkdir()
,IsExist
error should (usually) be ignored(unless you want to make sure directory was not there before)
as it means "the destination directory was already there";
for
MkdirAll()
,IsExist
error should NEVER be ignored.This commit removes ignoring the IsExist error, as it should not
be ignored.
For more details, a quote from my opencontainers/runc#162:
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.
Quoting MkdirAll documentation:
This means two things:
If a directory to be created already exists, no error is
returned.
If the error returned is IsExist (EEXIST), it means there exists
a non-directory with the same name as MkdirAll need to use for
directory. Example: we want to MkdirAll("a/b"), but file "a"
(or "a/b") already exists, so MkdirAll fails.
The above is a theory, based on quoted documentation and my UNIX
knowledge.
ENOTDIR in most of cases described in Test device numbers #2, with the exception when
there is a race between MkdirAll and someone else creating the
last component of MkdirAll argument as a file. In this very case
MkdirAll() will indeed return EEXIST.
Because of #1, IsExist check after MkdirAll is not needed.
Because of #2 and #3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.
Note this error is all over the tree, I guess due to copy-paste,
or trying to follow the same usage pattern as for Mkdir(),
or some not quite correct examples on the Internet.