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

On Thu, Nov 18, 2010 at 5:43 PM, Johannes Sixt [off-list ref] wrote:

Am 11/18/2010 15:19, schrieb Erik Faye-Lund:

quoted

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.

Hmm, this is not one of our wrappings, but a bug in the mingw-runtine,
right? Perhaps we should report it? I see that "errno = 0;" is the
first line of code of _topendir() in the mingw-runtime (see
http://cygwin.com/cgi-bin/cvsweb.cgi/src/winsup/mingw/mingwex/dirent.c?rev=1.9&cvsroot=src),
and from the looks of it it should simply be removed. The same goes
for closedir() and readdir().

That being said, we should probably work around this problem with
opendir() ourselves. This patch fixes the problem for me:

---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.