On 11/04/2016 07:17 AM, Tom Lane wrote:
- if (err != EEXIST && !itsdir(name)) { + if (err != EEXIST || !itsdir(name)) {
I see that you modified the function's header comment to acknowledge that it doesn't consider a collision with a non-directory to be an error, but that doesn't make any sense to me.
For example, that change could cause zic to cry wolf when it attempts to make an already-existing directory whose parent is unwritable. In this case mkdir can fail with errno == EACCES (because when there are multiple reasons to fail POSIX does not specify which errno value to use). This would cause "err != EXIST" to yield 1, which would generate a bogus diagnostic. Although it would be correct to replace the code with just 'if (!itsdir(name)) {', this would slow zic down in the common case where mkdir fails with errno == EEXIST due to an existing directory.
All the call sites would prefer the invariant to be "on return, there is a directory there"; they are all going to fail if there isn't. That should be OK, since when they fail they should diagnose their failures, with error messages that are good enough. Plus, any such failures can happen only when some other process is simultaneously doing something weird like replacing directories with non-directories. I don't think we need to worry about exactly which diagnostic is generated in such situations, so long as the problem is diagnosed somehow.
I installed the attached patch to help comment this a bit better.