Simplify and fix os.MkdirAll() usage #422
Conversation
TL;DR: check for IsExist(err) after a failed MkdirAll() is both
redundant and wrong -- so two reasons to remove it.
Quoting MkdirAll documentation:
> MkdirAll creates a directory named path, along with any necessary
> parents, and returns nil, or else returns an error. If path
> is already a directory, MkdirAll does nothing and returns nil.
This means two things:
1. If a directory to be created already exists, no error is
returned.
2. 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.
3. In practice, though, current MkdirAll implementation [1] returns
ENOTDIR in most of cases described in moby#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 moby#1, IsExist check after MkdirAll is not needed.
Because of moby#2 and moby#3, ignoring IsExist error is just plain wrong,
as directory we require is not created. It's cleaner to report
the error now.
[1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go
Signed-off-by: Kir Kolyshkin <[email protected]>
|
Here's an example |
|
@kolyshkin Thanks for the patch. The existing code ignores the error ONLY for the case of EEXIST. If, as per your example, the call |
|
@mavenugo The thing is, it is wrong to ignore EEXIST error, too. In its current implementation (https://golang.org/src/os/path.go#L12), MkdirAll() can actually return EEXIST in a case of the following race (which I admit I was too lazy to explain in details, but here you go). First, MkdirAll calls stat() and gets ENOENT, so it goes on to check and create all the parents. At this time, someone else creates a file under the same name as the last MkdirAll component. Finally, MkdirAll calls Mkdir to create that last component, Mkdir returns EEXIST which MkdirAll passes to the caller. Having said that, the race should be pretty rate, this is why I named my commit "Simplify and fix" not "Fix and simplify". This extra check is redundant in the first place. As of OS dependent, I admit I have UNIX/Linux in mind and don't know much about other platforms, but MkdirAll is definitely not OS-specific. Although Mkdir (which it calls) is OS-specific, it should be wrong to ignore its errors on any platform. |
|
@kolyshkin Thanks for the explanation. As you understand, Ignoring the EExist is a lazy way to make sure we dont fail if the directory is already available. Hence, if we decide to return the EExist error, then it is going to impact the rest of the code which has some assumptions on |
No. Please re-read my patch description. Let me quote the relevant part:
Also, I went ahead and checked that the current implementation of MkdirAll() is in sync with the above quoted documentation (i.e. it doesn't return EEXIST error if the directory is there), and that it is also OS-independent. So, once again, the check for EEXIST after MkdirAll() is at the very least redundant. |
|
@kolyshkin yep. I missed that point. Thanks. LGTM. |
|
BTW my similar pull requests to Docker and RunC are merged: |
|
Thanks @kolyshkin for the explanation. LGTM |
Simplify and fix os.MkdirAll() usage
Please review; I think it's of moderate importance as ignoring error from MkdirAll can lead to some other error later and it could be hard to track the cause down to failed MkdirAll.