From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
Here's the long overdue v2 of my daemon-win32 attempt. A lot
has happened since v1. Most importantly, I abandoned using
the async API to replace fork(), and went for explicitly
spawning child process that handle the connection.
The patches are on top of the current version of Junio's
master-branch.
When talking about patch numbers here, I'm referring to the numbers
from the previous round.
Patch 1 has been adjusted to work on top of Martin's ipv6 patches.
Patch 2 has been dropped, because it was made obsolete by the
changes to patch 3.
Patch 3 has been simplified as suggested by Hannes. The problem with
reporting IPv6 addresses should still be there in theory, but I
haven't been able to trigger it. I'm having a feeling that it's better
to do a case-by-case patch of the code that reports for this one.
Patch 4 and 5 are unchanged.
Patch 6 and 7 were dropped, in favour of a new approach.
Patch 8 has been updated as suggested.
Patch 9 has been rewritten to spawn a separate child process that
serves the client.
Patch 10 is unchanged
Patch 11 has been updated as suggested.
In addition, I've added a patch that add some needed support in
our waitpid()-emulation, a (very limited) kill()-emulation, a patch
that uses real PIDs on Windows (instead of process-local kernel-handles),
a patch that changes the code to use select() instead of poll() to wait
for socket-action (due to our limited poll-emulation). And there's a
patch that makes sure connections are reported from the root-process.
In addition, I've attached an updated version of the getaddrinfo()-fix
that Martin sent me privately. I removed Hannes sign-off (as requested by
Martin, due to the update)
The branch can also be found here:
http://repo.or.cz/w/git/kusma.git daemon-win32-v2
Puuuh, I hope I didn't miss anything important.
Erik Faye-Lund (10):
inet_ntop: fix a couple of old-style decls
mingw: support waitpid with pid > 0 and WNOHANG
mingw: use real pid
mingw: add kill emulation
daemon: use explicit file descriptor
daemon: use run-command api for async serving
daemon: use full buffered mode for stderr
mingw: compile git-daemon
daemon: use select() instead of poll()
daemon: report connection from root-process
Martin Storsjö (1):
Improve the mingw getaddrinfo stub to handle more use cases
Mike Pape (3):
mingw: add network-wrappers for daemon
mingw: implement syslog
compat: add inet_pton and inet_ntop prototypes
Makefile | 10 +-
compat/inet_ntop.c | 22 ++----
compat/inet_pton.c | 8 +-
compat/mingw.c | 141 +++++++++++++++++++++++++++++--
compat/mingw.h | 80 +++++++++++++++++-
daemon.c | 236 ++++++++++++++++++++++++++++------------------------
git-compat-util.h | 9 ++
7 files changed, 361 insertions(+), 145 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
From: Mike Pape <redacted>
git-daemon requires some socket-functionality that is not yet
supported in the Windows-port. This patch adds said functionality,
and makes sure WSAStartup gets called by socket(), since it is the
first network-call in git-daemon. In addition, a check is added to
prevent WSAStartup (and WSACleanup, though atexit) from being
called more than once, since git-daemon calls both socket() and
gethostbyname().
Signed-off-by: Mike Pape <redacted>
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/mingw.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
compat/mingw.h | 16 ++++++++++++++++
2 files changed, 58 insertions(+), 1 deletions(-)
@@ -1125,6 +1128,44 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)returnconnect(s,sa,sz);}+#undef bind+intmingw_bind(intsockfd,structsockaddr*sa,size_tsz)+{+SOCKETs=(SOCKET)_get_osfhandle(sockfd);+returnbind(s,sa,sz);+}++#undef setsockopt+intmingw_setsockopt(intsockfd,intlvl,intoptname,void*optval,intoptlen)+{+SOCKETs=(SOCKET)_get_osfhandle(sockfd);+returnsetsockopt(s,lvl,optname,(constchar*)optval,optlen);+}++#undef listen+intmingw_listen(intsockfd,intbacklog)+{+SOCKETs=(SOCKET)_get_osfhandle(sockfd);+returnlisten(s,backlog);+}++#undef accept+intmingw_accept(intsockfd1,structsockaddr*sa,socklen_t*sz)+{+intsockfd2;++SOCKETs1=(SOCKET)_get_osfhandle(sockfd1);+SOCKETs2=accept(s1,sa,sz);++/* convert into a file descriptor */+if((sockfd2=_open_osfhandle(s2,O_RDWR|O_BINARY))<0){+closesocket(s2);+returnerror("unable to make a socket file descriptor: %s",+strerror(errno));+}+returnsockfd2;+}+#undef renameintmingw_rename(constchar*pold,constchar*pnew){
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
From: Mike Pape <redacted>
Syslog does not usually exist on Windows, so we implement our own
using Window's ReportEvent mechanism.
Signed-off-by: Mike Pape <redacted>
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/mingw.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
compat/mingw.h | 15 +++++++++++++++
daemon.c | 2 --
git-compat-util.h | 1 +
4 files changed, 65 insertions(+), 2 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
From: Mike Pape <redacted>
Windows doesn't have inet_pton and inet_ntop, so
add prototypes in git-compat-util.h for them.
At the same time include git-compat-util.h in
the sources for these functions, so they use the
network-wrappers from there on Windows.
Signed-off-by: Mike Pape <redacted>
Signed-off-by: Erik Faye-Lund <redacted>
---
Makefile | 2 ++
compat/inet_ntop.c | 6 +++---
compat/inet_pton.c | 8 +++++---
git-compat-util.h | 8 ++++++++
4 files changed, 18 insertions(+), 6 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
The Windows port so far used process handles as PID. However,
this does not work consistently with getpid.
Change the code to use the real PID, and use OpenProcess to
get a process-handle.
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/mingw.c | 2 +-
compat/mingw.h | 35 +++++++++++++++++++++++++++++++----
2 files changed, 32 insertions(+), 5 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
This is a quite limited kill-emulation; it can only handle
SIGTERM on positive pids. However, it's enough for git-daemon.
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/mingw.c | 19 +++++++++++++++++++
compat/mingw.h | 3 +++
2 files changed, 22 insertions(+), 0 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
This patch adds support to specify an explicit file
descriotor for communication with the client, instead
of using stdin/stdout.
This will be useful for the Windows port, because it
will use threads instead of fork() to serve multiple
clients, making it impossible to reuse stdin/stdout.
Signed-off-by: Erik Faye-Lund <redacted>
---
daemon.c | 43 +++++++++++++++++++++----------------------
1 files changed, 21 insertions(+), 22 deletions(-)
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
fork() is only available on POSIX, so to support git-daemon
on Windows we have to use something else. Conveniently
enough, we have an API for async operation already.
Signed-off-by: Erik Faye-Lund <redacted>
---
daemon.c | 87 +++++++++++++++++++++++++++++++------------------------------
1 files changed, 44 insertions(+), 43 deletions(-)
@@ -665,20 +659,15 @@ static void handle(int incoming, struct sockaddr *addr, int addrlen)}}-if((pid=fork())){-close(incoming);-if(pid<0){-logerror("Couldn't fork %s",strerror(errno));-return;-}--add_child(pid,addr,addrlen);-return;-}+cld.argv=(constchar**)cld_argv;+cld.in=incoming;+cld.out=dup(incoming);-fd[0]=incoming;-fd[1]=dup(incoming);-exit(execute(fd,addr));+if(start_command(&cld))+logerror("unable to fork");+else+add_child(&cld,addr,addrlen);+close(incoming);}staticvoidchild_handler(intsigno)
@@ -934,7 +923,7 @@ int main(int argc, char **argv){intlisten_port=0;char*listen_addr=NULL;-intinetd_mode=0;+intserve_mode=0,inetd_mode=0;constchar*pid_file=NULL,*user_name=NULL,*group_name=NULL;intdetach=0;structpasswd*pass=NULL;
@@ -960,7 +949,12 @@ int main(int argc, char **argv)continue;}}+if(!strcmp(arg,"--serve")){+serve_mode=1;+continue;+}if(!strcmp(arg,"--inetd")){+serve_mode=1;inetd_mode=1;log_syslog=1;continue;
@@ -1104,13 +1098,13 @@ int main(int argc, char **argv)die("base-path '%s' does not exist or is not a directory",base_path);-if(inetd_mode){+if(serve_mode){intfd[2]={0,1};structsockaddr_storagess;structsockaddr*peer=(structsockaddr*)&ss;socklen_tslen=sizeof(ss);-if(!freopen("/dev/null","w",stderr))+if(inetd_mode&&!freopen("/dev/null","w",stderr))die_errno("failed to redirect stderr to /dev/null");if(getpeername(0,peer,&slen))
@@ -1129,5 +1123,12 @@ int main(int argc, char **argv)if(pid_file)store_pid(pid_file);+/* prepare argv for serving-processes */+cld_argv=xmalloc(sizeof(char*)*(argc+2));+for(i=0;i<argc;++i)+cld_argv[i]=argv[i];+cld_argv[argc]="--serve";+cld_argv[argc+1]=NULL;+returnserve(listen_addr,listen_port,pass,gid);}
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
Windows doesn't support line buffered mode for file
streams, so let's just use full buffered mode with
a big buffer ("4096 should be enough for everyone")
and add explicit flushing.
Signed-off-by: Erik Faye-Lund <redacted>
---
daemon.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
@@ -1062,7 +1064,7 @@ int main(int argc, char **argv)set_die_routine(daemon_die);}else/* avoid splitting a message in the middle */-setvbuf(stderr,NULL,_IOLBF,0);+setvbuf(stderr,NULL,_IOFBF,4096);if(inetd_mode&&(group_name||user_name))die("--user and --group are incompatible with --inetd");
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
--user and --detach are disabled on Windows due to lack of
fork(), setuid(), setgid(), setsid() and initgroups().
Signed-off-by: Erik Faye-Lund <redacted>
---
Makefile | 8 +++-----
compat/mingw.h | 1 +
daemon.c | 19 ++++++++++++++-----
3 files changed, 18 insertions(+), 10 deletions(-)
@@ -390,6 +390,7 @@ EXTRA_PROGRAMS =# ... and all the rest that could be moved out of bindir to gitexecdirPROGRAMS+=$(EXTRA_PROGRAMS)+PROGRAMS+=git-daemon$XPROGRAMS+=git-fast-import$XPROGRAMS+=git-hash-object$XPROGRAMS+=git-imap-send$X
@@ -847,7 +847,7 @@ static int service_loop(int socknum, int *socklist)for(i=0;i<socknum;i++){if(pfd[i].revents&POLLIN){structsockaddr_storagess;-unsignedintsslen=sizeof(ss);+socklen_tsslen=sizeof(ss);intincoming=accept(pfd[i].fd,(structsockaddr*)&ss,&sslen);if(incoming<0){switch(errno){
@@ -893,6 +894,9 @@ static void daemonize(void)close(1);close(2);sanitize_stdfds();+#else+die("--detach is not supported on Windows");+#endif}staticvoidstore_pid(constchar*path)
@@ -913,10 +917,12 @@ static int serve(char *listen_addr, int listen_port, struct passwd *pass, gid_tdie("unable to allocate any listen sockets on host %s port %u",listen_addr,listen_port);+#ifndef WIN32if(pass&&gid&&(initgroups(pass->pw_name,gid)||setgid(gid)||setuid(pass->pw_uid)))die("cannot drop privileges");+#endifreturnservice_loop(socknum,socklist);}
@@ -929,7 +935,6 @@ int main(int argc, char **argv)constchar*pid_file=NULL,*user_name=NULL,*group_name=NULL;intdetach=0;structpasswd*pass=NULL;-structgroup*group;gid_tgid=0;inti;
@@ -1078,6 +1083,7 @@ int main(int argc, char **argv)die("--group supplied without --user");if(user_name){+#ifndef WIN32pass=getpwnam(user_name);if(!pass)die("user not found - %s",user_name);
@@ -1085,12 +1091,15 @@ int main(int argc, char **argv)if(!group_name)gid=pass->pw_gid;else{-group=getgrnam(group_name);+structgroup*group=getgrnam(group_name);if(!group)die("group not found - %s",group_name);gid=group->gr_gid;}+#else+die("--user is not supported on Windows");+#endif}if(strict_paths&&(!ok_paths||!*ok_paths))
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
From: Martin Storsjö <redacted>
Allow the node parameter to be null, which is used for getting
the default bind address.
Also allow the hints parameter to be null, to improve standard
conformance of the stub implementation a little.
Signed-off-by: Martin Storsjo <redacted>
---
compat/mingw.c | 28 +++++++++++++++++++++-------
1 files changed, 21 insertions(+), 7 deletions(-)
@@ -956,14 +959,25 @@ static int WSAAPI getaddrinfo_stub(const char *node, const char *service,break;}ai->ai_addrlen=sizeof(structsockaddr_in);-ai->ai_canonname=strdup(h->h_name);+if(hints&&(hints->ai_flags&AI_CANONNAME))+ai->ai_canonname=h?strdup(h->h_name):NULL;+else+ai->ai_canonname=NULL;sin=xmalloc(ai->ai_addrlen);memset(sin,0,ai->ai_addrlen);sin->sin_family=AF_INET;+/* Note: getaddrinfo is supposed to allow service to be a string,+*whichshouldbelookedupusinggetservbyname.Thisis+*currentlynotimplemented*/if(service)sin->sin_port=htons(atoi(service));-sin->sin_addr=*(structin_addr*)h->h_addr;+if(h)+sin->sin_addr=*(structin_addr*)h->h_addr;+elseif(hints&&(hints->ai_flags&AI_PASSIVE))+sin->sin_addr.s_addr=INADDR_ANY;+else+sin->sin_addr.s_addr=INADDR_LOOPBACK;ai->ai_addr=(structsockaddr*)sin;ai->ai_next=0;return0;
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
Windows doesn't have poll(), and the poll-emulation in
compat/mingw.c doesn't support checking multiple sockets.
Signed-off-by: Erik Faye-Lund <redacted>
---
compat/mingw.h | 7 +++++++
daemon.c | 27 ++++++++++++---------------
2 files changed, 19 insertions(+), 15 deletions(-)
@@ -818,26 +818,23 @@ static int socksetup(char *listen_addr, int listen_port, int **socklist_p)staticintservice_loop(intsocknum,int*socklist){-structpollfd*pfd;-inti;--pfd=xcalloc(socknum,sizeof(structpollfd));--for(i=0;i<socknum;i++){-pfd[i].fd=socklist[i];-pfd[i].events=POLLIN;-}-signal(SIGCHLD,child_handler);for(;;){-inti;+inti,maxfd=0;+fd_setfds;check_dead_children();-if(poll(pfd,socknum,-1)<0){+FD_ZERO(&fds);+for(i=0;i<socknum;i++){+FD_SET(socklist[i],&fds);+maxfd=socklist[i]>maxfd?socklist[i]:maxfd;+}++if(select(maxfd+1,&fds,NULL,NULL,NULL)<0){if(errno!=EINTR){-logerror("Poll failed, resuming: %s",+logerror("select() failed, resuming: %s",strerror(errno));sleep(1);}
@@ -845,10 +842,10 @@ static int service_loop(int socknum, int *socklist)}for(i=0;i<socknum;i++){-if(pfd[i].revents&POLLIN){+if(FD_ISSET(socklist[i],&fds)){structsockaddr_storagess;socklen_tsslen=sizeof(ss);-intincoming=accept(pfd[i].fd,(structsockaddr*)&ss,&sslen);+intincoming=accept(socklist[i],(structsockaddr*)&ss,&sslen);if(incoming<0){switch(errno){caseEAGAIN:
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
Report incoming connections from the process that
accept() the connection instead of the handling
process.
This enables "Connection from"-reporting on
Windows, where getpeername(0, ...) consistently
fails.
Signed-off-by: Erik Faye-Lund <redacted>
---
daemon.c | 70 +++++++++++++++++++++++++++++++++++--------------------------
1 files changed, 40 insertions(+), 30 deletions(-)
@@ -661,14 +659,21 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)}}+addrstr=get_addrstr(&port,addr);+strcat(envbuf,addrstr);++cld.env=(constchar**)env;cld.argv=(constchar**)cld_argv;cld.in=incoming;cld.out=dup(incoming);if(start_command(&cld))logerror("unable to fork");-else+else{+loginfo("[%"PRIuMAX"] Connection from %s:%d",+(uintmax_t)cld.pid,addrstr,port);add_child(&cld,addr,addrlen);+}close(incoming);}
@@ -1115,8 +1120,13 @@ int main(int argc, char **argv)if(inetd_mode&&!freopen("/dev/null","w",stderr))die_errno("failed to redirect stderr to /dev/null");-if(getpeername(0,peer,&slen))-peer=NULL;+if(!getpeername(0,peer,&slen)){+intport=-1;+char*addrstr=get_addrstr(&port,peer);+setenv("REMOTE_ADDR",addrstr,1);+loginfo("[%"PRIuMAX"] Connection from %s:%d",+(uintmax_t)getpid(),addrstr,port);+}returnexecute(fd,peer);}
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
A very nicely done series. Thank you very much!
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
Here's the long overdue v2 of my daemon-win32 attempt. A lot
has happened since v1. Most importantly, I abandoned using
the async API to replace fork(), and went for explicitly
spawning child process that handle the connection.
IOW, you run git-daemon recursively in inetd mode (almost). Let's see what
people say about this approach.
-- Hannes
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
This patch adds support to specify an explicit file
descriotor for communication with the client, instead
of using stdin/stdout.
This will be useful for the Windows port, because it
will use threads instead of fork() to serve multiple
clients, making it impossible to reuse stdin/stdout.
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
fork() is only available on POSIX, so to support git-daemon
on Windows we have to use something else. Conveniently
enough, we have an API for async operation already.
I had a huh?-moment when I read this statement. This patch does not use what
we call 'async', but start_command().
-- Hannes
I'm worried about the internals that you have to use here. Isn't it possible
save the original macro text and use it in the new definition, like (this is
for exposition only):
#define ORIG_FD_SET(fd, set) FD_SET(fd, set)
#undef FD_SET
#define FD_SET(fd, set) ORIG_FD_SET(_get_osfhandle(fd), set)
Another approach would be to extend the poll emulation such that it uses
select if all FDs to wait for are sockets, and I think this would be the case
in this application.
-- Hannes
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
On Fri, Jan 15, 2010 at 11:27 PM, Johannes Sixt [off-list ref] wrote:
A very nicely done series. Thank you very much!
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
quoted
Here's the long overdue v2 of my daemon-win32 attempt. A lot
has happened since v1. Most importantly, I abandoned using
the async API to replace fork(), and went for explicitly
spawning child process that handle the connection.
IOW, you run git-daemon recursively in inetd mode (almost). Let's see what
people say about this approach.
-- Hannes
Yes. Or, a subset of the inetd-mode.
--
Erik "kusma" Faye-Lund
You are not using the pi.hProcess anymore, so you must close it.
No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).
--
Erik "kusma" Faye-Lund
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
On Fri, Jan 15, 2010 at 11:57 PM, Janos Laube [off-list ref] wrote:
quoted
+static HANDLE ms_eventlog;
+
+void openlog(const char *ident, int logopt, int facility)
+{
+ if (ms_eventlog)
+ return;
+ ms_eventlog = RegisterEventSourceA(NULL, ident);
+}
maybe make ms_eventlog thread local?
for example:
static __thread HANDLE ms_eventlog;
this would break compilation with msvc tho.
janos
Since the code that use it isn't multi-threaded, I fail to see the
point. In fact even if it were, I'm not sure I see the big point...
especially since the "__thread"-keyword isn't used (AFAICT) at all in
the git source code so far.
--
Erik "kusma" Faye-Lund
I'm worried about the internals that you have to use here. Isn't it possible
save the original macro text and use it in the new definition, like (this is
for exposition only):
#define ORIG_FD_SET(fd, set) FD_SET(fd, set)
#undef FD_SET
#define FD_SET(fd, set) ORIG_FD_SET(_get_osfhandle(fd), set)
Redefining it is indeed fishy - I guess I should also have noted that
I even stripped the code down slightly (compared to the original).
I'm no preprocessor wizard, but I'll give it a stab.
Another approach would be to extend the poll emulation such that it uses
select if all FDs to wait for are sockets, and I think this would be the case
in this application.
The problem with that is differentiating between pipes and sockets.
GetFileType() returns FILE_TYPE_PIPE for sockets (ugh). I did find
some code in gnulib that used WSAEnumNetworkEvents() to differentiate
between them, but I find this quite hacky.
--
Erik "kusma" Faye-Lund
Since the code that use it isn't multi-threaded, I fail to see the
point. In fact even if it were, I'm not sure I see the big point...
especially since the "__thread"-keyword isn't used (AFAICT) at all in
the git source code so far.
that's why i have put a question mark behind my sentence. it was just
an idea :-). it would allow different threads to be an own event
source. but yes, i wasn't sure how much git makes use of threads. if
it doesn't, it does not make much sense at the moment, indeed.
janos
I'm worried about the internals that you have to use here. Isn't it possible
save the original macro text and use it in the new definition, like (this is
for exposition only):
#define ORIG_FD_SET(fd, set) FD_SET(fd, set)
#undef FD_SET
#define FD_SET(fd, set) ORIG_FD_SET(_get_osfhandle(fd), set)
Redefining it is indeed fishy - I guess I should also have noted that
I even stripped the code down slightly (compared to the original).
I'm no preprocessor wizard, but I'll give it a stab.
You are not using the pi.hProcess anymore, so you must close it.
No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).
Previously, this handle was closed by _cwait() (it was the "pid"), so we
didn't leak it.
I somehow thought that you need the process ID instead of the handle for
TerminateProcess, but now I see that this is not the case (it takes the
handle). So I don't see the point of this change anymore. You say the process
handle "does not work consistently with getpid", but I don't know what you
mean. Please explain.
-- Hannes
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
On Samstag, 16. Januar 2010, Erik Faye-Lund wrote:
The problem with that is differentiating between pipes and sockets.
GetFileType() returns FILE_TYPE_PIPE for sockets (ugh). I did find
some code in gnulib that used WSAEnumNetworkEvents() to differentiate
between them, but I find this quite hacky.
Wouldn't it be possible to call getsockopt(), and if it returns ENOTSOCK
(WSAENOTSOCK), then it is a pipe?
-- Hannes
You are not using the pi.hProcess anymore, so you must close it.
No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).
Previously, this handle was closed by _cwait() (it was the "pid"), so we
didn't leak it.
Oh, I see. My planned route with this (before I looked for where the
handle was closed), was to maintain some sort of list of each started
PID and their handle, and lookup in that list instead of using
OpenProcess. I guess that would solve the problem here, but it feels a
bit nasty. Not as nasty as introducing a leak, though.
I somehow thought that you need the process ID instead of the handle for
TerminateProcess, but now I see that this is not the case (it takes the
handle). So I don't see the point of this change anymore. You say the process
handle "does not work consistently with getpid", but I don't know what you
mean. Please explain.
getpid() returns the real PID, and is used for prefixing each logged
message by git-daemon. However, the root process reports whenever a
new process is started or has terminated using the PID returned by
mingw_spawnve(), and this handle does not match up with the PID that
getpid() reports. Thus it becomes impossible to tell which reported
error belongs to which client.
--
Erik "kusma" Faye-Lund
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
On Sat, Jan 16, 2010 at 9:08 AM, Johannes Sixt [off-list ref] wrote:
On Samstag, 16. Januar 2010, Erik Faye-Lund wrote:
quoted
The problem with that is differentiating between pipes and sockets.
GetFileType() returns FILE_TYPE_PIPE for sockets (ugh). I did find
some code in gnulib that used WSAEnumNetworkEvents() to differentiate
between them, but I find this quite hacky.
Wouldn't it be possible to call getsockopt(), and if it returns ENOTSOCK
(WSAENOTSOCK), then it is a pipe?
-- Hannes
I read reports that this didn't work in Wine. Not that I care that
much about Wine.
--
Erik "kusma" Faye-Lund
Ah, yes, how obvious ;) You are going to do the same with FD_ISSET as well,
aren't you?
-- Hannes
Do I really need to? There's already a single function for that, with
no "ugly hidden internals" there; __WSAFDIsSet() is documented in
MSDN. I mean, Sure... I could... I just don't see the point.
--
Erik "kusma" Faye-Lund
Ah, yes, how obvious ;) You are going to do the same with FD_ISSET as
well, aren't you?
Do I really need to? There's already a single function for that, with
no "ugly hidden internals" there; __WSAFDIsSet() is documented in
MSDN. I mean, Sure... I could... I just don't see the point.
__WSAFDIsSet is "ugly hidden internals" and we should not rely on it when we
can use the official FD_ISSET for our own FD_ISSET.
-- Hannes
Ah, yes, how obvious ;) You are going to do the same with FD_ISSET as
well, aren't you?
Do I really need to? There's already a single function for that, with
no "ugly hidden internals" there; __WSAFDIsSet() is documented in
MSDN. I mean, Sure... I could... I just don't see the point.
__WSAFDIsSet is "ugly hidden internals" and we should not rely on it when we
can use the official FD_ISSET for our own FD_ISSET.
-- Hannes
...but __WSAFDIsSet() seems to be every bit as official on Windows as
FD_ISSET() (documented in msdn, without any notes not to use it), so I
still don't really see the point.
However, I don't personally care one way or the other, so just doing
it is probably less work than convincing you that I don't have to ;)
--
Erik "kusma" Faye-Lund
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:02
On Samstag, 16. Januar 2010, Erik Faye-Lund wrote:
...but __WSAFDIsSet() seems to be every bit as official on Windows as
FD_ISSET() (documented in msdn, without any notes not to use it), so I
still don't really see the point.
I didn't know nor check whether it is documented, but assumed from the '__'
that it must be internal. Being documented makes a big difference. I'm fine
with either solution.
-- Hannes
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
On Sat, Jan 16, 2010 at 1:36 PM, Johannes Sixt [off-list ref] wrote:
On Samstag, 16. Januar 2010, Erik Faye-Lund wrote:
quoted
...but __WSAFDIsSet() seems to be every bit as official on Windows as
FD_ISSET() (documented in msdn, without any notes not to use it), so I
still don't really see the point.
I didn't know nor check whether it is documented, but assumed from the '__'
that it must be internal. Being documented makes a big difference. I'm fine
with either solution.
OK, in that case, I'll leave it as it is. The current code is slightly
more tested (I've been using it for some weeks), and I'm slightly lazy
;)
But I'll update FD_SET...
--
Erik "kusma" Faye-Lund
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:02
On Fri, Jan 15, 2010 at 11:36 PM, Johannes Sixt [off-list ref] wrote:
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
quoted
This patch adds support to specify an explicit file
descriotor for communication with the client, instead
of using stdin/stdout.
This will be useful for the Windows port, because it
will use threads instead of fork() to serve multiple
clients, making it impossible to reuse stdin/stdout.
This statement is a bit outdated.
-- Hannes
Heh, yeah. I apparently missed that this patch isn't needed at all any more.
Thanks for noticing something was off :)
--
Erik "kusma" Faye-Lund
You are not using the pi.hProcess anymore, so you must close it.
No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).
Previously, this handle was closed by _cwait() (it was the "pid"), so we
didn't leak it.
Oh, I see. My planned route with this (before I looked for where the
handle was closed), was to maintain some sort of list of each started
PID and their handle, and lookup in that list instead of using
OpenProcess. I guess that would solve the problem here, but it feels a
bit nasty. Not as nasty as introducing a leak, though.
What I had in mind was something along these lines:
From: Johannes Sixt <hidden> Date: 2016-06-15 22:48:03
On Montag, 18. Januar 2010, Erik Faye-Lund wrote:
On Sat, Jan 16, 2010 at 10:12 AM, Erik Faye-Lund wrote:
quoted
On Sat, Jan 16, 2010 at 9:03 AM, Johannes Sixt [off-list ref] wrote:
quoted
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
quoted
No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).
Previously, this handle was closed by _cwait() (it was the "pid"), so we
didn't leak it.
Oh, I see. My planned route with this (before I looked for where the
handle was closed), was to maintain some sort of list of each started
PID and their handle, and lookup in that list instead of using
OpenProcess. I guess that would solve the problem here, but it feels a
bit nasty. Not as nasty as introducing a leak, though.
What I had in mind was something along these lines:
Given that that the process ID is the user-visible (and system-wide unique)
identifier of a process, this looks like the only reasonable way to go. Your
implementation looks good as well.
+ /* store process handle */
/*
* The process ID is the human-readable identifier of the process
* that we want to present in log and error messages. The handle
* is not useful for this purpose. But we cannot close it, either,
* because it is not possible to turn a process ID into a process
* handle after the process terminated.
* Keep the handle in a list for waitpid.
*/
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:48:03
On Tue, Jan 19, 2010 at 7:19 PM, Johannes Sixt [off-list ref] wrote:
On Montag, 18. Januar 2010, Erik Faye-Lund wrote:
quoted
On Sat, Jan 16, 2010 at 10:12 AM, Erik Faye-Lund wrote:
quoted
On Sat, Jan 16, 2010 at 9:03 AM, Johannes Sixt [off-list ref] wrote:
quoted
On Freitag, 15. Januar 2010, Erik Faye-Lund wrote:
quoted
No. If I do, the pid becomes invalid after the process is finished,
and waitpid won't work. I couldn't find anywhere were we actually were
closing the handle, even after it was finished. So I don't think we
leak any more than we already did (for non-daemon purposes).
Previously, this handle was closed by _cwait() (it was the "pid"), so we
didn't leak it.
Oh, I see. My planned route with this (before I looked for where the
handle was closed), was to maintain some sort of list of each started
PID and their handle, and lookup in that list instead of using
OpenProcess. I guess that would solve the problem here, but it feels a
bit nasty. Not as nasty as introducing a leak, though.
What I had in mind was something along these lines:
Given that that the process ID is the user-visible (and system-wide unique)
identifier of a process, this looks like the only reasonable way to go. Your
implementation looks good as well.
quoted
+ /* store process handle */
/*
* The process ID is the human-readable identifier of the process
* that we want to present in log and error messages. The handle
* is not useful for this purpose. But we cannot close it, either,
* because it is not possible to turn a process ID into a process
* handle after the process terminated.
* Keep the handle in a list for waitpid.
*/