Re: [PATCH] Set errno to EEXIST if mkdir returns EACCES or EPERM

Alex Riesen <raa.lkml@gmail.com> writes:

Thanks for all the background information. Although it is very tempting for me to adopt your patch as an easy way out, I would feel *dirty* if I did so.

Eric Blake is right in his argument in that thread. Our code should not depend on the Linux EEXIST behaviour. The reason Cygwin folks want to be as close to Linux is to work around application bugs like this. Which is a valid concern for them, but it does not mean the application has license to depend on Linux behaviour.

We, as an application, should take a different attitude -- we should fix things to make things portable, not work things around, if both are equally easily doable. I do not object to using a function that has a semantics Linux mkdir() gives us, but calling that mkdir() does not feel quite right.

Also the wrapper implies that in our future use of mkdir() we cannot tell the difference between EEXIST and other errors if we later wanted to.

So let's look at our existing uses first:

apply.c:1577: if (mkdir(buf, 0777) < 0) { apply.c-1578- if (errno != EEXIST) apply.c-1579- break; apply.c-1580- } apply.c-1581- }

This is "we want to see buf directory exist and we would create one if there isn't". Instead of checking errno, we could stat as your patch does.

entry.c:15: if (mkdir(buf, 0777)) { entry.c-16- if (errno == EEXIST) { entry.c-17- struct stat st; entry.c:18: if (len > state->base_dir_len && state->force && !unlink(buf) && !mkdir(buf, 0777)) entry.c-19- continue; entry.c-20- if (!stat(buf, &st) && S_ISDIR(st.st_mode)) entry.c-21- continue; /* ok */ entry.c-22- }

This is a bit more involved. "we want to have buf directory and we would create it, and if force is given to have something under that directory, we would even unlink the nondirectory that sits there".

I asked "git-grep" where else we use mkdir(), and all other users want the semantics of the apply.c use quoted above.

So I'd rather see us create a generic helper function like this:

	int make_directory(const char *path, int force)
        {
		struct stat st;
		int mkdir_errno;
		if (!mkdir(path, 0777))
			return 0;
                mkdir_errno = errno;
		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
                	return 0;
                if (!force) {
		bad:
                	errno = mkdir_errno;
                        return -1;
                }
                if (!unlink(path, &st) && !mkdir(path, 0777))
                	return 0;
                /* we might have failed to unlink an existing symlink
		 * which happens to point at an existing directory; that
                 * directory is not what we want here.
                 */
		if (!lstat(path, &st) && S_ISDIR(st.st_mode))
                	return 0;
		goto bad;
        }

and have current callers of mkdir() use it, regardless of the platform. It may not worth saving mkdir_errno, though.

Then everybody but entry.c one would say force=0, and entry.c one passes force appropriately using the condition it uses in its current if() statement.