Re: [PATCH 05/20] rename_tmp_log(): use raceproof_create_file()
Michael Haggerty writes: > Now, we have to consider the opposite case, namely that we are calling a > non-buggy implementation of `rename()`, and we artificially change > ENOTDIR to EISDIR. Can that cause any bad effects? > > I don't think so, because the case where a non-buggy implementation can > yield ENOTDIR is a case, the consequent call to > `remove_dir_recursively()` would fail with ENOTDIR too, and > `raceproof_create_file()` would give up immediately. > > So I think everything is OK, though I admit that it is not especially > elegant. OK, thanks for a clear analysis. I knew the original and the update were meant to (and would) behave the same, but the workaround logic in the original looked iffy. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/20] rename_tmp_log(): use raceproof_create_file()
On 02/17/2016 09:53 PM, Junio C Hamano wrote:
> Michael Haggerty writes:
>
>> Besides shortening the code, this saves an unnecessary call to
>> safe_create_leading_directories_const() in almost all cases.
>>
>> Signed-off-by: Michael Haggerty
>> ---
>> refs/files-backend.c | 76
>> ++--
>> 1 file changed, 32 insertions(+), 44 deletions(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index a549942..e5f964c 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -2400,55 +2400,43 @@ out:
>> */
>> #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
>>
>> +static int rename_tmp_log_callback(const char *path, void *cb)
>> +{
>> +int *true_errno = cb;
>> +
>> +if (rename(git_path(TMP_RENAMED_LOG), path)) {
>> +/*
>> + * rename(a, b) when b is an existing directory ought
>> + * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
>> + * Sheesh. Record the true errno for error reporting,
>> + * but report EISDIR to raceproof_create_file() so
>> + * that it knows to retry.
>> + */
>> +*true_errno = errno;
>> +if (errno==ENOTDIR)
>> +errno = EISDIR;
>
> Style: SP on both sides of a binary operator.
Thanks; will fix.
> More importantly, is ENOTDIR expected only on a buggy platform?
Here I was just mimicking the old behavior, which I think was correct,
but let's check more carefully...
> [ENOTDIR]
> A component of either path prefix names an existing file that is
> neither a directory nor a symbolic link to a directory; or the old
> argument names a directory and the new argument names a
> non-directory file; or the old argument contains at least one non-
> character and ends with one or more trailing
> characters and the last pathname component names an existing file
> that is neither a directory nor a symbolic link to a directory; or
> the old argument names an existing non-directory file and the new
> argument names a nonexistent file, contains at least one non-
> character, and ends with one or more trailing
> characters; or the new argument names an existing non-directory
> file, contains at least one non- character, and ends with
> one or more trailing characters.
>
> i.e. when a leading component of "path" or TMP_RENAMED_LOG is an
> existing non-directory, we could get ENOTDIR on a valid system.
>
> If another instance of Git created a file A/B when this process is
> trying to rename the temporary thing to its final location A/B/C,
> isn't that the errno we would see here?
>
> [EISDIR]
> The new argument points to a directory and the old argument
> points to a file that is not a directory.
>
> Puzzled...
We just created TMP_RENAMED_LOG ourselves, so I don't think we need to
expect errors from that argument. (Though I don't recall that there is
any locking to prevent two `git branch -m` processes from clobbering
each others' temporary files. Oh well; renaming branches is relatively
rare and probably interactive, so I'll declare that potential problem to
be out of scope for this patch series.)
So let's consider the cases where we can get ENOTDIR for `path`:
> A component of either path prefix names an existing file that is
> neither a directory nor a symbolic link to a directory.
This can certainly happen for `path`, but it is not a case that can be
rescued by raceproof_create_file().
> or the old argument names a directory [...]
This is not the case.
> or the old argument contains [...]
Also not interesting.
> or the old argument names an existing non-directory file and the new
> argument names a nonexistent file, contains at least one non-
> character, and ends with one or more trailing
> characters
The new argument doesn't end with trailing characters, so this
can't happen.
> or the new argument names an existing non-directory
> file, contains at least one non- character, and ends with
> one or more trailing characters
Ditto.
So while it is true that a non-buggy implementation can give ENOTDIR, it
is for a case that we can't rescue. So if it weren't for the buggy
implementation, we could just leave ENOTDIR un-handled.
Now, we have to consider the opposite case, namely that we are calling a
non-buggy implementation of `rename()`, and we artificially change
ENOTDIR to EISDIR. Can that cause any bad effects?
I don't think so, because the case where a non-buggy implementation can
yield ENOTDIR is a case, the consequent call to
`remove_dir_recursively()` would fail with ENOTDIR too, and
`raceproof_create_file()` would give up immediately.
So I think everything is OK, though I admit that it is not especially
elegant. We could limit ourselves to doing the workaround only on
Solaris 5.8, but that seems like a lot of effort for not much benefit.
Or we could drop the workaround; after all, Solaris
Re: [PATCH 05/20] rename_tmp_log(): use raceproof_create_file()
Michael Haggerty writes:
> Besides shortening the code, this saves an unnecessary call to
> safe_create_leading_directories_const() in almost all cases.
>
> Signed-off-by: Michael Haggerty
> ---
> refs/files-backend.c | 76
> ++--
> 1 file changed, 32 insertions(+), 44 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a549942..e5f964c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2400,55 +2400,43 @@ out:
> */
> #define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
>
> +static int rename_tmp_log_callback(const char *path, void *cb)
> +{
> + int *true_errno = cb;
> +
> + if (rename(git_path(TMP_RENAMED_LOG), path)) {
> + /*
> + * rename(a, b) when b is an existing directory ought
> + * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
> + * Sheesh. Record the true errno for error reporting,
> + * but report EISDIR to raceproof_create_file() so
> + * that it knows to retry.
> + */
> + *true_errno = errno;
> + if (errno==ENOTDIR)
> + errno = EISDIR;
Style: SP on both sides of a binary operator.
More importantly, is ENOTDIR expected only on a buggy platform?
[ENOTDIR]
A component of either path prefix names an existing file that is
neither a directory nor a symbolic link to a directory; or the old
argument names a directory and the new argument names a
non-directory file; or the old argument contains at least one non-
character and ends with one or more trailing
characters and the last pathname component names an existing file
that is neither a directory nor a symbolic link to a directory; or
the old argument names an existing non-directory file and the new
argument names a nonexistent file, contains at least one non-
character, and ends with one or more trailing
characters; or the new argument names an existing non-directory
file, contains at least one non- character, and ends with
one or more trailing characters.
i.e. when a leading component of "path" or TMP_RENAMED_LOG is an
existing non-directory, we could get ENOTDIR on a valid system.
If another instance of Git created a file A/B when this process is
trying to rename the temporary thing to its final location A/B/C,
isn't that the errno we would see here?
[EISDIR]
The new argument points to a directory and the old argument
points to a file that is not a directory.
Puzzled...
> + return -1;
> + } else {
> + return 0;
> + }
> +}
> +
> static int rename_tmp_log(const char *newrefname)
> {
> - int attempts_remaining = 4;
> - struct strbuf path = STRBUF_INIT;
> - int ret = -1;
> + char *path = git_pathdup("logs/%s", newrefname);
> + int true_errno;
> + int ret;
>
> - retry:
> - strbuf_reset(&path);
> - strbuf_git_path(&path, "logs/%s", newrefname);
> - switch (safe_create_leading_directories_const(path.buf)) {
> - case SCLD_OK:
> - break; /* success */
> - case SCLD_VANISHED:
> - if (--attempts_remaining > 0)
> - goto retry;
> - /* fall through */
> - default:
> - error("unable to create directory for %s", newrefname);
> - goto out;
> - }
> -
> - if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
> - if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining >
> 0) {
> - /*
> - * rename(a, b) when b is an existing
> - * directory ought to result in ISDIR, but
> - * Solaris 5.8 gives ENOTDIR. Sheesh.
> - */
> - if (remove_empty_directories(&path)) {
> - error("Directory not empty: logs/%s",
> newrefname);
> - goto out;
> - }
> - goto retry;
> - } else if (errno == ENOENT && --attempts_remaining > 0) {
> - /*
> - * Maybe another process just deleted one of
> - * the directories in the path to newrefname.
> - * Try again from the beginning.
> - */
> - goto retry;
> - } else {
> + ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
> + if (ret) {
> + if (true_errno==EISDIR)
> + error("Directory not empty: %s", path);
> + else
> error("unable to move logfile "TMP_RENAMED_LOG" to
> logs/%s: %s",
> - newrefname, strerror(errno));
> - goto out;
> - }
> + newrefname, strerror(true_errno));
> }
> - ret =
