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

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

> ... and the directory already exists. I.E. Cygwin is such
> a case: mkdir fail for mounts which reference directly
> to windows mounts ("drives").
>
> ---
>
> The discussion, which ended up with this patch can be read
> here: http://www.cygwin.com/ml/cygwin/2006-01/msg01276.html
>
> BTW, there is this:
> http://www.cygwin.com/ml/cygwin/2006-01/msg01380.html
> So this patch will probably be not needed soon.

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.

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html