Re: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Tue, May 20, 2014 at 10:46 AM, Thomas Braun
wrote:
> Am 19.05.2014 22:29, schrieb Erik Faye-Lund:
>>> [...]
>>> Would we need to wrap both ends, shouldn't wrapping only reading be
>>> good enough to prevent deadlocking?
>>>
>>> compat/poll/poll.c already contains a function called IsSocketHandle
>>> that is able to tell if a HANDLE points to a socket or not.
>>
>> This very quick attempt did not work out :(
>>
>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 0335958..ec1d81f 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
>> return fd;
>> }
>>
>> +#define is_console_handle(h) (((long) (h) & 3) == 3)
>> +
>> +static int is_socket_handle(HANDLE h)
>> +{
>> +WSANETWORKEVENTS ev;
>> +
>> +if (is_console_handle(h))
>> +return 0;
>> +
>> +/*
>> + * Under Wine, it seems that getsockopt returns 0 for pipes too.
>> + * WSAEnumNetworkEvents instead distinguishes the two correctly.
>> + */
>> +ev.lNetworkEvents = 0xDEADBEEF;
>> +WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
>> +return ev.lNetworkEvents != 0xDEADBEEF;
>> +}
>> +
>> +#undef read
>> +ssize_t mingw_read(int fd, void *buf, size_t count)
>> +{
>> +int ret;
>> +HANDLE fh = (HANDLE)_get_osfhandle(fd);
>> +
>> +if (fh == INVALID_HANDLE_VALUE) {
>> +errno = EBADF;
>> +return -1;
>> +}
>> +
>> +if (!is_socket_handle(fh))
>> +return read(fd, buf, count);
>> +
>> +ret = recv((SOCKET)fh, buf, count, 0);
>> +if (ret < 0)
>> +errno = WSAGetLastError();
>> +return ret;
>> +}
>> +
>> +#undef write
>> +ssize_t mingw_write(int fd, const void *buf, size_t count)
>> +{
>> +int ret;
>> +HANDLE fh = (HANDLE)_get_osfhandle(fd);
>> +
>> +if (fh == INVALID_HANDLE_VALUE) {
>> +errno = EBADF;
>> +return -1;
>> +}
>> +
>> +if (!is_socket_handle(fh))
>> +return write(fd, buf, count);
>> +
>> +return send((SOCKET)fh, buf, count, 0);
>> +if (ret < 0)
>> +errno = WSAGetLastError();
>> +return ret;
>> +}
>> +
>> +
>> static BOOL WINAPI ctrl_ignore(DWORD type)
>> {
>> return TRUE;
>> diff --git a/compat/mingw.h b/compat/mingw.h
>> index 08b83fe..1690098 100644
>> --- a/compat/mingw.h
>> +++ b/compat/mingw.h
>> @@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
>> int mingw_open (const char *filename, int oflags, ...);
>> #define open mingw_open
>>
>> +ssize_t mingw_read(int fd, void *buf, size_t count);
>> +#define read mingw_read
>> +
>> +ssize_t mingw_write(int fd, const void *buf, size_t count);
>> +#define write mingw_write
>> +
>> int mingw_fgetc(FILE *stream);
>> #define fgetc mingw_fgetc
>
> According to [1] you also have to pass WSA_FLAG_OVERLAPPED to avoid the
> deadlock.
>
> With that change I don't get a hang anymore but a read error with
> errno 10054 aka WSAECONNRESET.
>
> [1]: https://groups.google.com/forum/#!msg/msysgit/at8D7J-h7mw/PM9w-d41cDYJ
>
Yeah, sorry, I noticed this right after sending and tested with that
as well. My results were the same :/
--
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: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
Am 19.05.2014 22:29, schrieb Erik Faye-Lund:
>> [...]
>> Would we need to wrap both ends, shouldn't wrapping only reading be
>> good enough to prevent deadlocking?
>>
>> compat/poll/poll.c already contains a function called IsSocketHandle
>> that is able to tell if a HANDLE points to a socket or not.
>
> This very quick attempt did not work out :(
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 0335958..ec1d81f 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
> return fd;
> }
>
> +#define is_console_handle(h) (((long) (h) & 3) == 3)
> +
> +static int is_socket_handle(HANDLE h)
> +{
> +WSANETWORKEVENTS ev;
> +
> +if (is_console_handle(h))
> +return 0;
> +
> +/*
> + * Under Wine, it seems that getsockopt returns 0 for pipes too.
> + * WSAEnumNetworkEvents instead distinguishes the two correctly.
> + */
> +ev.lNetworkEvents = 0xDEADBEEF;
> +WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
> +return ev.lNetworkEvents != 0xDEADBEEF;
> +}
> +
> +#undef read
> +ssize_t mingw_read(int fd, void *buf, size_t count)
> +{
> +int ret;
> +HANDLE fh = (HANDLE)_get_osfhandle(fd);
> +
> +if (fh == INVALID_HANDLE_VALUE) {
> +errno = EBADF;
> +return -1;
> +}
> +
> +if (!is_socket_handle(fh))
> +return read(fd, buf, count);
> +
> +ret = recv((SOCKET)fh, buf, count, 0);
> +if (ret < 0)
> +errno = WSAGetLastError();
> +return ret;
> +}
> +
> +#undef write
> +ssize_t mingw_write(int fd, const void *buf, size_t count)
> +{
> +int ret;
> +HANDLE fh = (HANDLE)_get_osfhandle(fd);
> +
> +if (fh == INVALID_HANDLE_VALUE) {
> +errno = EBADF;
> +return -1;
> +}
> +
> +if (!is_socket_handle(fh))
> +return write(fd, buf, count);
> +
> +return send((SOCKET)fh, buf, count, 0);
> +if (ret < 0)
> +errno = WSAGetLastError();
> +return ret;
> +}
> +
> +
> static BOOL WINAPI ctrl_ignore(DWORD type)
> {
> return TRUE;
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 08b83fe..1690098 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
> int mingw_open (const char *filename, int oflags, ...);
> #define open mingw_open
>
> +ssize_t mingw_read(int fd, void *buf, size_t count);
> +#define read mingw_read
> +
> +ssize_t mingw_write(int fd, const void *buf, size_t count);
> +#define write mingw_write
> +
> int mingw_fgetc(FILE *stream);
> #define fgetc mingw_fgetc
According to [1] you also have to pass WSA_FLAG_OVERLAPPED to avoid the
deadlock.
With that change I don't get a hang anymore but a read error with
errno 10054 aka WSAECONNRESET.
[1]: https://groups.google.com/forum/#!msg/msysgit/at8D7J-h7mw/PM9w-d41cDYJ
diff --git a/compat/mingw.c b/compat/mingw.c
index 383cafe..4a58135 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -366,6 +366,71 @@ int mingw_open (const char *filename, int oflags, ...)
return fd;
}
+#define is_console_handle(h) (((long) (h) & 3) == 3)
+
+static int is_socket_handle(HANDLE h)
+{
+WSANETWORKEVENTS ev;
+
+if (is_console_handle(h))
+return 0;
+
+/*
+ * Under Wine, it seems that getsockopt returns 0 for pipes too.
+ * WSAEnumNetworkEvents instead distinguishes the two correctly.
+ */
+ev.lNetworkEvents = 0xDEADBEEF;
+WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
+return ev.lNetworkEvents != 0xDEADBEEF;
+}
+
+#undef read
+ssize_t mingw_read(int fd, void *buf, size_t count)
+{
+int ret;
+HANDLE fh = (HANDLE)_get_osfhandle(fd);
+
+if (fh == INVALID_HANDLE_VALUE) {
+errno = EBADF;
+return -1;
+}
+
+if (!is_socket_handle(fh))
+return read(fd, buf, count);
+
+ret = recv((SOCKET)fh, buf, count, 0);
+if (ret < 0)
+{
+errno = WSAGetLastError();
+warning("errno %d",errno);
+}
+return ret;
+}
+
+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t count)
+{
+int ret;
+HANDLE fh = (HANDLE)_get_osfhandle(fd);
+
+if (fh == INVALID_HANDLE_VALUE) {
+errno = EBADF;
+return -1;
+}
+
+if (!is_socket_handle(fh))
+return write(fd, buf, count);
+
+ret = send((SOCKET)fh, buf, count, 0);
+if (ret < 0)
+{
+errno = WSAGetLastError();
+warning("errno %d",errno);
+}
+return ret;
+}
+
+
static BOOL WINAPI ctrl_ignore(DWORD type)
{
return TRUE;
@@ -1542,7 +1607,7 @@ int mingw_socket(int domain, int type, int protocol)
SOCKET s;
ensure_socket_initialization();
- s = WSASocket(domain, type, protocol, NULL, 0, 0);
+ s = WSASocket(domain, type, protocol, NULL, 0, WSA_FLAG_OVERLAPPED);
if (s == INVALID_SOCKET) {
/*
* WSAGetLastError() values are regular BSD error codes
diff --git a/compat/mingw.h b/compat/
Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Mon, May 19, 2014 at 11:15 PM, Thomas Braun wrote: > Am 19.05.2014 21:33, schrieb Jonathan Nieder: > >> Hi, >> >> Thomas Braun wrote: >> >>> pushing over the dump git protocol with a windows git client. >> >> >> I've never heard of the dump git protocol. Do you mean the git >> protocol that's used with git:// URLs? > > > You are right I mean the protocol involving git:// URLs. But unfortunately I > got it wrong as according to [1] the git:// is one of the so-called smart > protocols. That was also the source where I read that there are smart and > dump protocols. > > [1]: http://git-scm.com/book/en/Git-Internals-Transfer-Protocols > > >> [...] >>> >>> Alternative approaches considered but deemed too invasive: >>> - Rewrite read/write wrappers in mingw.c in order to distinguish between >>>a file descriptor which has a socket behind and a file descriptor >>>which has a file behind. >> >> >> I assume here "too invasive" means "too much engineering effort"? >> >> It sounds like a clean fix, not too invasive at all. But I can >> understand wanting a stopgap in the meantime. > > > No actually I meant too invasive in the sense of "requiring large rewrites > which only benefit git on windows and hurt all others". > > The two fixes I can think of either involve: > - In a read *and* write wrapper the need to check if the fd is a socket, if > yes use send/recv if no use read/write. According to Erik's comments this > should be possible. But I would deem the expected performance penalty quite > large as that will be done in every call. You clearly haven't stepped through MSVCRT's read and write implementations :P I wouldn't worry too much about this, at least not until the numbers are in. -- 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/RFC] send-pack.c: Allow to disable side-band-64k
Am 19.05.2014 21:33, schrieb Jonathan Nieder: Hi, Thomas Braun wrote: pushing over the dump git protocol with a windows git client. I've never heard of the dump git protocol. Do you mean the git protocol that's used with git:// URLs? You are right I mean the protocol involving git:// URLs. But unfortunately I got it wrong as according to [1] the git:// is one of the so-called smart protocols. That was also the source where I read that there are smart and dump protocols. [1]: http://git-scm.com/book/en/Git-Internals-Transfer-Protocols [...] Alternative approaches considered but deemed too invasive: - Rewrite read/write wrappers in mingw.c in order to distinguish between a file descriptor which has a socket behind and a file descriptor which has a file behind. I assume here "too invasive" means "too much engineering effort"? It sounds like a clean fix, not too invasive at all. But I can understand wanting a stopgap in the meantime. No actually I meant too invasive in the sense of "requiring large rewrites which only benefit git on windows and hurt all others". The two fixes I can think of either involve: - In a read *and* write wrapper the need to check if the fd is a socket, if yes use send/recv if no use read/write. According to Erik's comments this should be possible. But I would deem the expected performance penalty quite large as that will be done in every call. - Rewriting read/write to accept windows handles instead of file descriptors. Only a theoretical option IMHO. For me the goal is also to minimise the diff between git and msysgit/git. - Turning the capability side-band-64k off completely. This would remove a useful feature for users of non-affected transport protocols. Would it make sense to turn off sideband unconditionally on Windows when using the relevant protocols? Yes, if this would be also acceptable for git.git. I can check at the call site of send_pack in transport.c what protocol is in use, and then pass a new parameter use_sideband to it. Or maybe "adapt" server_capabilities in connect.c to not include side-band-64k if using git:// ? -- 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: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Mon, May 19, 2014 at 10:00 PM, Erik Faye-Lund wrote:
> On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder wrote:
>> Hi,
>>
>> Thomas Braun wrote:
>>
>>> pushing over the dump git protocol with a windows git client.
>>
>> I've never heard of the dump git protocol. Do you mean the git
>> protocol that's used with git:// URLs?
>>
>> [...]
>>> Alternative approaches considered but deemed too invasive:
>>> - Rewrite read/write wrappers in mingw.c in order to distinguish between
>>> a file descriptor which has a socket behind and a file descriptor
>>> which has a file behind.
>>
>> I assume here "too invasive" means "too much engineering effort"?
>>
>> It sounds like a clean fix, not too invasive at all. But I can
>> understand wanting a stopgap in the meantime.
>>
>
> Yeah, now that the problem seems to be understood, I don't think that
> would be too bad. I recently killed off our previous write()-wrapper
> in c9df6f4, but I see no reason why we can't add a new one.
>
> Would we need to wrap both ends, shouldn't wrapping only reading be
> good enough to prevent deadlocking?
>
> compat/poll/poll.c already contains a function called IsSocketHandle
> that is able to tell if a HANDLE points to a socket or not.
This very quick attempt did not work out :(
diff --git a/compat/mingw.c b/compat/mingw.c
index 0335958..ec1d81f 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -370,6 +370,65 @@ int mingw_open (const char *filename, int oflags, ...)
return fd;
}
+#define is_console_handle(h) (((long) (h) & 3) == 3)
+
+static int is_socket_handle(HANDLE h)
+{
+WSANETWORKEVENTS ev;
+
+if (is_console_handle(h))
+return 0;
+
+/*
+ * Under Wine, it seems that getsockopt returns 0 for pipes too.
+ * WSAEnumNetworkEvents instead distinguishes the two correctly.
+ */
+ev.lNetworkEvents = 0xDEADBEEF;
+WSAEnumNetworkEvents((SOCKET) h, NULL, &ev);
+return ev.lNetworkEvents != 0xDEADBEEF;
+}
+
+#undef read
+ssize_t mingw_read(int fd, void *buf, size_t count)
+{
+int ret;
+HANDLE fh = (HANDLE)_get_osfhandle(fd);
+
+if (fh == INVALID_HANDLE_VALUE) {
+errno = EBADF;
+return -1;
+}
+
+if (!is_socket_handle(fh))
+return read(fd, buf, count);
+
+ret = recv((SOCKET)fh, buf, count, 0);
+if (ret < 0)
+errno = WSAGetLastError();
+return ret;
+}
+
+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t count)
+{
+int ret;
+HANDLE fh = (HANDLE)_get_osfhandle(fd);
+
+if (fh == INVALID_HANDLE_VALUE) {
+errno = EBADF;
+return -1;
+}
+
+if (!is_socket_handle(fh))
+return write(fd, buf, count);
+
+return send((SOCKET)fh, buf, count, 0);
+if (ret < 0)
+errno = WSAGetLastError();
+return ret;
+}
+
+
static BOOL WINAPI ctrl_ignore(DWORD type)
{
return TRUE;
diff --git a/compat/mingw.h b/compat/mingw.h
index 08b83fe..1690098 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -177,6 +177,12 @@ int mingw_rmdir(const char *path);
int mingw_open (const char *filename, int oflags, ...);
#define open mingw_open
+ssize_t mingw_read(int fd, void *buf, size_t count);
+#define read mingw_read
+
+ssize_t mingw_write(int fd, const void *buf, size_t count);
+#define write mingw_write
+
int mingw_fgetc(FILE *stream);
#define fgetc mingw_fgetc
--
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: [msysGit] Re: [PATCH/RFC] send-pack.c: Allow to disable side-band-64k
On Mon, May 19, 2014 at 9:33 PM, Jonathan Nieder wrote: > Hi, > > Thomas Braun wrote: > >> pushing over the dump git protocol with a windows git client. > > I've never heard of the dump git protocol. Do you mean the git > protocol that's used with git:// URLs? > > [...] >> Alternative approaches considered but deemed too invasive: >> - Rewrite read/write wrappers in mingw.c in order to distinguish between >> a file descriptor which has a socket behind and a file descriptor >> which has a file behind. > > I assume here "too invasive" means "too much engineering effort"? > > It sounds like a clean fix, not too invasive at all. But I can > understand wanting a stopgap in the meantime. > Yeah, now that the problem seems to be understood, I don't think that would be too bad. I recently killed off our previous write()-wrapper in c9df6f4, but I see no reason why we can't add a new one. Would we need to wrap both ends, shouldn't wrapping only reading be good enough to prevent deadlocking? compat/poll/poll.c already contains a function called IsSocketHandle that is able to tell if a HANDLE points to a socket or not. -- 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/RFC] send-pack.c: Allow to disable side-band-64k
Hi, Thomas Braun wrote: > pushing over the dump git protocol with a windows git client. I've never heard of the dump git protocol. Do you mean the git protocol that's used with git:// URLs? [...] > Alternative approaches considered but deemed too invasive: > - Rewrite read/write wrappers in mingw.c in order to distinguish between > a file descriptor which has a socket behind and a file descriptor > which has a file behind. I assume here "too invasive" means "too much engineering effort"? It sounds like a clean fix, not too invasive at all. But I can understand wanting a stopgap in the meantime. > - Turning the capability side-band-64k off completely. This would remove a > useful > feature for users of non-affected transport protocols. Would it make sense to turn off sideband unconditionally on Windows when using the relevant protocols? I assume someone on the list wouldn't mind writing such a patch, so I don't think the engineering effort would be a problem for that. Thanks, Jonathan -- 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
[PATCH/RFC] send-pack.c: Allow to disable side-band-64k
Since commit 0c499ea60f the send-pack builtin uses the side-band-64k
capability if advertised by the server. Unfortunately this breaks
pushing over the dump git protocol with a windows git client.
The detailed reasons for this breakage are (by courtesy of Jeff
Preshing, quoted from
https://groups.google.com/d/msg/msysgit/at8D7J-h7mw/eaLujILGUWoJ):
MinGW wraps Windows sockets in CRT file descriptors in order to mimic
the functionality of POSIX sockets. This causes msvcrt.dll to treat
sockets as Installable File System (IFS) handles, calling ReadFile,
WriteFile, DuplicateHandle and CloseHandle on them. This approach works
well in simple cases on recent versions of Windows, but does not support
all usage patterns. In particular, using this approach, any attempt to
read & write concurrently on the same socket (from one or more
processes) will deadlock in a scenario where the read waits for a
response from the server which is only invoked after the write. This is
what send_pack currently attempts to do in the use_sideband codepath.
The new config option "sendpack.sideband" allows to override the
side-band-64k capability of the server.
Other transportation methods like ssh and http/https still benefit from
the sideband channel, therefore the default value of "sendpack.sideband"
is still true.
Alternative approaches considered but deemed too invasive:
- Rewrite read/write wrappers in mingw.c in order to distinguish between
a file descriptor which has a socket behind and a file descriptor
which has a file behind.
- Turning the capability side-band-64k off completely. This would remove a
useful
feature for users of non-affected transport protocols.
Signed-off-by: Thomas Braun
---
This patch, with a slightly less polished commit message, is already part of
msysgit/git see b68e386.
A lengthy discussion can be found here [1].
What do you think, is this also for you as upstream interesting?
[1]: https://github.com/msysgit/git/issues/101
Documentation/config.txt | 6 ++
send-pack.c | 14 +-
2 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..13ff657 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2435,3 +2435,9 @@ web.browser::
Specify a web browser that may be used by some commands.
Currently only linkgit:git-instaweb[1] and linkgit:git-help[1]
may use it.
+
+sendpack.sideband::
+ Allows to disable the side-band-64k capability for send-pack even
+ when it is advertised by the server. Makes it possible to work
+ around a limitation in the git for windows implementation together
+ with the dump git protocol. Defaults to true.
diff --git a/send-pack.c b/send-pack.c
index 6129b0f..aace1fc 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -12,6 +12,16 @@
#include "version.h"
#include "sha1-array.h"
+static int config_use_sideband = 1;
+
+static int send_pack_config(const char *var, const char *value, void *unused)
+{
+ if (!strcmp("sendpack.sideband", var))
+ config_use_sideband = git_config_bool(var, value);
+
+ return 0;
+}
+
static int feed_object(const unsigned char *sha1, int fd, int negative)
{
char buf[42];
@@ -209,6 +219,8 @@ int send_pack(struct send_pack_args *args,
int ret;
struct async demux;
+ git_config(send_pack_config, NULL);
+
/* Does the other end support the reporting? */
if (server_supports("report-status"))
status_report = 1;
@@ -216,7 +228,7 @@ int send_pack(struct send_pack_args *args,
allow_deleting_refs = 1;
if (server_supports("ofs-delta"))
args->use_ofs_delta = 1;
- if (server_supports("side-band-64k"))
+ if (config_use_sideband && server_supports("side-band-64k"))
use_sideband = 1;
if (server_supports("quiet"))
quiet_supported = 1;
--
1.9.1
--
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
