Jeff King [off-list ref] writes:
It would probably look like the patch below, though it really feels like
the right solution is to fix the cygwin bug.
Thanks for a quick analysis and fix, folks.
We would need to assume certain things that the platform would give its
users are reliable, and system calls are fundamental part; otherwise we
would end up tying our hands behind our back saying "we cannot use this
and that as these are unreliable in certain places" and litter our
codebase with full of ifdefs and workarounds.
If this were merely of a breakage in a test release or a nightly build, I
would be happier to see us ignore this, but the problematic one is widely
in the wild, a workaround would be necessary, and more importantly, if it
is harder for a casual end-user to tell if the platform is affected (I am
assuming that most releases of Cygwin is without this bug, and most users
who build git themselves wouldn't bother reading README or Makefile even
if we said a MAX_WRITE_SIZE definition is necessary only for this and that
version), I would rather see a change that covers the problem a bit more
widely than necessary.
How prevalent is the problematic cygwin1.dll 1.7.8? Also for how long did
this bug exist, in other words, if we were to make a table of problematic
versions, would we have only just a handful entries in it? Also can we at
runtime find out what version we are running?
The reason I am asking these questions is because I think, assuming that
this would affect many unsuspecting Cygwin users, the best fix would be to
add a hook in the compat/ layer that decides if MAX_WRITE_SIZE workaround
is necessary at runtime, and do something like this:
ssize_t xwrite(int fd, const void *buf, size_t len)
{
ssize_t nr;
static size_t max_write_size = platform_max_write_size();
if (max_write_size && max_write_size < len)
len = max_write_size;
...
}
And then we would have in git-compat-util.h something like:
#define platform_max_write_size() 0
on sane platforms, so that the fix will be optimized away by the compiler.
By the way, does the same version of Cygwin have similar issue on the read
side?
By the way, does the same version of Cygwin have similar issue on the read
side?
Actually I've written a small test program on the problematic cygwin
machine that seems to show that the EAGAIN is somehow related to pipes
(e.g. stdout & stuff) and does not occur when fd refers to a file.
I will test this for read() as well but I don't know enough cygwin
internals to tell what other versions may be affected.
It might even be related to 32/64 bit or different Windows versions
since I didn't test more constellations.
-Robert
By the way, does the same version of Cygwin have similar issue on the read
side?
Here some quick results:
read(fd, buffer, 90000000)
Returns total file size for a file and 65536 (errno=0) for pipes (cat
file | readtest). When repeating the read, the whole file is read until
read() returns 0. No problem here on any cygwin I tested.
write(fd, buffer, 90000000)
Returns 90000000 for a file.
Returns 90000000 for redirection (writetest > outfile)
Returns 90000000 for pipes (writetest | cat > outfile) on sane cygwins
and -1 / EAGAIN on the machine with the original problem.
On Fri, Mar 04, 2011 at 09:16:30AM -0800, Junio C Hamano wrote:
How prevalent is the problematic cygwin1.dll 1.7.8? Also for how long did
this bug exist, in other words, if we were to make a table of problematic
versions, would we have only just a handful entries in it? Also can we at
runtime find out what version we are running?
The reason I am asking these questions is because I think, assuming that
this would affect many unsuspecting Cygwin users, the best fix would be to
add a hook in the compat/ layer that decides if MAX_WRITE_SIZE workaround
is necessary at runtime, and do something like this:
ssize_t xwrite(int fd, const void *buf, size_t len)
{
ssize_t nr;
static size_t max_write_size = platform_max_write_size();
if (max_write_size && max_write_size < len)
len = max_write_size;
...
}
How are we doing the runtime test for platform max write?
If I read the original bug report correctly, the problem was that write
would actually write some bytes _and_ return -1, which is terrible. We
can detect "seems to be returning -1 over and over", but we can't handle
a misbehavior like writing and claiming not to have done so.
So I think the test needs to be "is our version of cygwin in the broken
list" and not "let's try a few different writes and see what works".
But it is still not clear to me how many versions have this bug. I think
the next stop is to show the cygwin developers a clear test-case and see
whether it's already fixed, and which versions show the behavior. They
should be able to get that information much more easily than us. I
really don't want to get involved in bisecting bugs in cygwin (according
to cygwin.com, it's kept in CVS. Blech).
Robert, can you try (or have you already tried) submitting a bug report
to Cygwin?
-Peff