Re: [PATCH 2/2] A loose object is not corrupt if it cannot be read due to EMFILE

From

Erik Faye-Lund <kusmabite@gmail.com>

Date

Nov 18, 2010, 17:21 UTC

Message-ID

<AANLkTikns0Rm5znC7FOa72qZXKpQzazKiYsPe-oDNMAk@mail.gmail.com>

In-Reply-To

<4CE55794.7050201@viscovery.net>

On Thu, Nov 18, 2010 at 5:43 PM, Johannes Sixt <j.sixt@viscovery.net> wrote:

> Am 11/18/2010 15:19, schrieb Erik Faye-Lund:
>> What happens, is that read_object returns NULL, but errno is 0.
>> Further, it looks to me like read_object can only return NULL through
>> the unpack_sha1_file (problem with the compressed data) or
>> read_packed_sha1 (find_pack_entry() failure) code-paths.
>>
>> errno is set to ENOENT by open_sha1_file (through map_sha1_file)
>> before any possible error-points. I guess this makes the "errno = 0"
>> redundant, but I think it improves readability of the code. I'm
>> guessing that errno gets overwritten by some other call, losing the
>> ENOENT. Perhaps some unintended side-effect of one of the
>> compat/mingw.[ch]-wrappers?
>
> The problem is in opendir() called via prepare_packed_git_one() via
> prepare_packed_git(). It resets errno to 0 on success.
>

---8<---

diff --git a/compat/mingw.h b/compat/mingw.h
index 35d9813..b5f16fa 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -133,6 +133,29 @@ static inline int mingw_unlink(const char *pathname)
 }
 #define unlink mingw_unlink

+static inline DIR *mingw_opendir(const char *path)
+{
+	DIR *ret;
+	int saved_errno = errno;
+
+	if (!(ret = opendir(path)))
+		return NULL;
+
+	errno = saved_errno;
+	return ret;
+}
+#define opendir mingw_opendir
+
+static inline int mingw_closedir(DIR *dir)
+{
+	int saved_errno = errno;
+	if (closedir(dir) == -1)
+		return -1;
+	errno = saved_errno;
+	return 0;
+}
+#define closedir mingw_closedir
+
 #define WNOHANG 1
 pid_t waitpid(pid_t pid, int *status, unsigned options);

---8<---

I didn't patch readdir(), since we already have our own
implementation, but perhaps the code should work together with
NO_MINGW_REPLACE_READDIR also... Can somebody remind me why we have an
opt-out for the readdir()-replacement? Debugging?

> You can test this easily by inserting test_done after the 3rd test of
> t5530 and run it with --debug; in the trash-directory you can run
>
>  ../../git-pack-objects --revs --all --stdout >/dev/null </dev/null
>
> and observe the different failure modes on Windows and Linux.
>

Thanks.

> This makes me question whether the approach of Junio's fix is sane. It
> depends on errno being set *way* before it is checked and after *a*lot* of
> potentially failing system and library calls have been called. Which
> function is it that is expected to fail with ENOENT? git_open_noatime()?
>

I was wondering about the same thing, I don't think this approach is
very easy to follow. But either way I think we should make sure
opendir/closedir/readdir doesn't mess with errno.