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
