Conversation
a4d557c to
57053a2
Compare
|
@thaJeztah looks like golangci-lint is not too happy with the changes |
4d6ae4d to
7ee622a
Compare
9ef7b0f to
405c4bb
Compare
405c4bb to
2890ea4
Compare
e87e767 to
70f6df5
Compare
70f6df5 to
b4a0c39
Compare
The Windows implementation of mkdirAs was originally introduced as a minimal wrapper around os.MkdirAll because ownership is not supported on Windows and the uid/gid parameters were only needed to keep the daemon starting. As a result, MkdirAndChown would also create missing parent directories, unlike the Unix implementation. Use os.Mkdir when mkAll is false to preserve the documented semantics, while retaining the simplified Windows implementation for ownership and permissions. Originally introduced in [moby/moby@bfe252b78184][1], which added a Windows-specific implementation to work around limitations on that platform; > A recent change to use pkg\idtools causes a chown to be on the > startup path of the daemon. Chown is not supported on Windows, > hence the daemon would not start. [1]: moby/moby@bfe252b Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
b4a0c39 to
e02fdc4
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Address the non-atomic directory creation race before approval.
Review details
Suppressed comments (1)
user/idtools_windows.go:25
- The initial
Statand thisMkdirare not atomic: if another process createspathafter the stat reports it missing,os.MkdirreturnsEEXISTeven thoughpathis now a directory. That violatesMkdirAndChown's documented nil-on-existing-directory behavior and is a regression fromMkdirAll's race handling. AttemptMkdirfirst and re-stat its error to accept a directory (while retaining theENOTDIRresult for files).
return os.Mkdir(path, 0)
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Had this initially, but then changed it to use the same order as on Linux; we can look at the linux code if we want to do a second check on errors, but that can be done separately. |
The Windows implementation of mkdirAs was originally introduced as a minimal wrapper around os.MkdirAll because ownership is not supported on Windows and the uid/gid parameters were only needed to keep the daemon starting.
As a result, MkdirAndChown would also create missing parent directories, unlike the Unix implementation.
Use os.Mkdir when mkAll is false to preserve the documented semantics, while retaining the simplified Windows implementation for ownership and permissions.
Originally introduced in moby/moby@bfe252b78184, which added a Windows-specific implementation to work around limitations on that platform;