From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-01-12 15:32:33
This series introduces a multi-threaded IPC mechanism called "Simple IPC".
This is a library-layer feature to make it easy to create very long running
daemon/service applications and for unrelated Git commands to communicate
with them. Communication uses pkt-line messaging over a Windows named pipe
or Unix domain socket.
On the server side, Simple IPC implements a (platform-specific) connection
listener and worker thread-pool to accept and handle a series of client
connections. The server functionality is completely hidden behind the
ipc_server_run() and ipc_server_run_async() APIs. The daemon/service
application only needs to define an application-specific callback to handle
client requests.
Note that Simple IPC is completely unrelated to the long running process
feature (described in sub-process.h) where the lifetime of a "sub-process"
child is bound to that of the invoking parent process and communication
occurs over the child's stdin/stdout.
Simple IPC will serve as a basis for a future builtin FSMonitor daemon
feature.
Jeff Hostetler (7):
pkt-line: use stack rather than static buffer in packet_write_gently()
simple-ipc: design documentation for new IPC mechanism
simple-ipc: add win32 implementation
unix-socket: create gentle version of unix_stream_listen()
unix-socket: add no-chdir option to unix_stream_listen_gently()
simple-ipc: add t/helper/test-simple-ipc and t0052
simple-ipc: add Unix domain socket implementation
Johannes Schindelin (3):
pkt-line: (optionally) libify the packet readers
pkt-line: optionally skip the flush packet in
write_packetized_from_buf()
pkt-line: accept additional options in read_packetized_to_strbuf()
Documentation/technical/api-simple-ipc.txt | 31 +
Makefile | 8 +
compat/simple-ipc/ipc-shared.c | 28 +
compat/simple-ipc/ipc-unix-socket.c | 1093 ++++++++++++++++++++
compat/simple-ipc/ipc-win32.c | 723 +++++++++++++
config.mak.uname | 2 +
contrib/buildsystems/CMakeLists.txt | 6 +
convert.c | 4 +-
pkt-line.c | 30 +-
pkt-line.h | 13 +-
simple-ipc.h | 221 ++++
t/helper/test-simple-ipc.c | 485 +++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0052-simple-ipc.sh | 129 +++
unix-socket.c | 58 +-
unix-socket.h | 9 +
17 files changed, 2828 insertions(+), 14 deletions(-)
create mode 100644 Documentation/technical/api-simple-ipc.txt
create mode 100644 compat/simple-ipc/ipc-shared.c
create mode 100644 compat/simple-ipc/ipc-unix-socket.c
create mode 100644 compat/simple-ipc/ipc-win32.c
create mode 100644 simple-ipc.h
create mode 100644 t/helper/test-simple-ipc.c
create mode 100755 t/t0052-simple-ipc.sh
base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/766
--
gitgitgadget
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-01-12 15:32:33
From: Jeff Hostetler <redacted>
Teach packet_write_gently() to use a stack buffer rather than a static
buffer when composing the packet line message. This helps get us ready
for threaded operations.
Signed-off-by: Jeff Hostetler <redacted>
---
pkt-line.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Jeff King <hidden> Date: 2021-01-13 13:29:55
On Tue, Jan 12, 2021 at 03:31:23PM +0000, Jeff Hostetler via GitGitGadget wrote:
Teach packet_write_gently() to use a stack buffer rather than a static
buffer when composing the packet line message. This helps get us ready
for threaded operations.
Sounds like a good goal, but...
static int packet_write_gently(const int fd_out, const char *buf, size_t size)
{
- static char packet_write_buffer[LARGE_PACKET_MAX];
+ char packet_write_buffer[LARGE_PACKET_MAX];
size_t packet_size;
64k is awfully big for the stack, especially if you are thinking about
having threads. I know we've run into issues around that size before
(though I don't offhand recall whether there was any recursion
involved).
We might need to use thread-local storage here. Heap would also
obviously work, but I don't think we'd want a new allocation per write
(or maybe it wouldn't matter; we're making a syscall, so a malloc() may
not be that big a deal in terms of performance).
-Peff
From: Jeff Hostetler <hidden> Date: 2021-01-26 01:53:44
On 1/13/21 8:29 AM, Jeff King wrote:
On Tue, Jan 12, 2021 at 03:31:23PM +0000, Jeff Hostetler via GitGitGadget wrote:
quoted
Teach packet_write_gently() to use a stack buffer rather than a static
buffer when composing the packet line message. This helps get us ready
for threaded operations.
Sounds like a good goal, but...
quoted
static int packet_write_gently(const int fd_out, const char *buf, size_t size)
{
- static char packet_write_buffer[LARGE_PACKET_MAX];
+ char packet_write_buffer[LARGE_PACKET_MAX];
size_t packet_size;
64k is awfully big for the stack, especially if you are thinking about
having threads. I know we've run into issues around that size before
(though I don't offhand recall whether there was any recursion
involved).
We might need to use thread-local storage here. Heap would also
obviously work, but I don't think we'd want a new allocation per write
(or maybe it wouldn't matter; we're making a syscall, so a malloc() may
not be that big a deal in terms of performance).
-Peff
Good point.
I'll look at the callers and see if I can do something safer.
Jeeff
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-01-12 15:32:33
From: Johannes Schindelin <redacted>
The `read_packetized_to_strbuf()` function reads packets into a strbuf
until a flush packet has been received. So far, it has only one caller:
`apply_multi_file_filter()` in `convert.c`. This caller really only
needs the `PACKET_READ_GENTLE_ON_EOF` option to be passed to
`packet_read()` (which makes sense in the scenario where packets should
be read until a flush packet is received).
We are about to introduce a caller that wants to pass other options
through to `packet_read()`, so let's extend the function signature
accordingly.
Signed-off-by: Johannes Schindelin <redacted>
---
convert.c | 2 +-
pkt-line.c | 4 ++--
pkt-line.h | 6 +++++-
3 files changed, 8 insertions(+), 4 deletions(-)
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-01-12 15:32:33
From: Johannes Schindelin <redacted>
So far, the (possibly indirect) callers of `get_packet_data()` can ask
that function to return an error instead of `die()`ing upon end-of-file.
However, random read errors will still cause the process to die.
So let's introduce an explicit option to tell the packet reader
machinery to please be nice and only return an error.
This change prepares pkt-line for use by long-running daemon processes.
Such processes should be able to serve multiple concurrent clients and
and survive random IO errors. If there is an error on one connection,
a daemon should be able to drop that connection and continue serving
existing and future connections.
This ability will be used by a Git-aware "Internal FSMonitor" feature
in a later patch series.
Signed-off-by: Johannes Schindelin <redacted>
---
pkt-line.c | 19 +++++++++++++++++--
pkt-line.h | 4 ++++
2 files changed, 21 insertions(+), 2 deletions(-)
@@ -298,8 +298,11 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,*src_size-=ret;}else{ret=read_in_full(fd,dst,size);-if(ret<0)+if(ret<0){+if(options&PACKET_READ_NEVER_DIE)+returnerror_errno(_("read error"));die_errno(_("read error"));+}}/* And complain if we didn't get enough bytes to satisfy the read. */
@@ -307,6 +310,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,if(options&PACKET_READ_GENTLE_ON_EOF)return-1;+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("the remote end hung up unexpectedly"));die(_("the remote end hung up unexpectedly"));}
@@ -335,6 +340,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,len=packet_length(linelen);if(len<0){+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("protocol error: bad line length "+"character: %.4s"),linelen);die(_("protocol error: bad line length character: %.4s"),linelen);}elseif(!len){packet_trace("0000",4,0);
@@ -349,12 +357,19 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,*pktlen=0;returnPACKET_READ_RESPONSE_END;}elseif(len<4){+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("protocol error: bad line length %d"),+len);die(_("protocol error: bad line length %d"),len);}len-=4;-if((unsigned)len>=size)+if((unsigned)len>=size){+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("protocol error: bad line length %d"),+len);die(_("protocol error: bad line length %d"),len);+}if(get_packet_data(fd,src_buffer,src_len,buffer,len,options)<0){*pktlen=-1;
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-01-12 15:32:33
From: Jeff Hostetler <redacted>
Brief design documentation for new IPC mechanism allowing
foreground Git client to talk with an existing daemon process
at a known location using a named pipe or unix domain socket.
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Jeff Hostetler <redacted>
---
Documentation/technical/api-simple-ipc.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/technical/api-simple-ipc.txt
@@ -0,0 +1,31 @@+simple-ipc API+==============++The simple-ipc API is used to send an IPC message and response between+a (presumably) foreground Git client process to a background server or+daemon process. The server process must already be running. Multiple+client processes can simultaneously communicate with the server+process.++Communication occurs over a named pipe on Windows and a Unix domain+socket on other platforms. Clients and the server rendezvous at a+previously agreed-to application-specific pathname (which is outside+the scope of this design).++This IPC mechanism differs from the existing `sub-process.c` model+(Documentation/technical/long-running-process-protocol.txt) and used+by applications like Git-LFS because the server is assumed to be very+long running system service. In contrast, a "sub-process model process"+is started with the foreground process and exits when the foreground+process terminates. How the server is started is also outside the+scope of the IPC mechanism.++The IPC protocol consists of a single request message from the client and+an optional request message from the server. For simplicity, pkt-line+routines are used to hide chunking and buffering concerns. Each side+terminates their message with a flush packet.+(Documentation/technical/protocol-common.txt)++The actual format of the client and server messages is application+specific. The IPC layer transmits and receives an opaque buffer without+any concern for the content within.
On Tue, Jan 12 2021, Jeff Hostetler via GitGitGadget wrote:
quoted hunk
From: Jeff Hostetler <redacted>
Brief design documentation for new IPC mechanism allowing
foreground Git client to talk with an existing daemon process
at a known location using a named pipe or unix domain socket.
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Jeff Hostetler <redacted>
---
Documentation/technical/api-simple-ipc.txt | 31 ++++++++++++++++++++++
1 file changed, 31 insertions(+)
create mode 100644 Documentation/technical/api-simple-ipc.txt
@@ -0,0 +1,31 @@+simple-ipc API+==============++The simple-ipc API is used to send an IPC message and response between+a (presumably) foreground Git client process to a background server or+daemon process. The server process must already be running. Multiple+client processes can simultaneously communicate with the server+process.++Communication occurs over a named pipe on Windows and a Unix domain+socket on other platforms. Clients and the server rendezvous at a+previously agreed-to application-specific pathname (which is outside+the scope of this design).++This IPC mechanism differs from the existing `sub-process.c` model+(Documentation/technical/long-running-process-protocol.txt) and used+by applications like Git-LFS because the server is assumed to be very
s/to be very long running/to be a long running/, or at least "s/to be
very/to be a very/.
+long running system service. In contrast, a "sub-process model process"
+is started with the foreground process and exits when the foreground
+process terminates. How the server is started is also outside the
+scope of the IPC mechanism.
+
+The IPC protocol consists of a single request message from the client and
+an optional request message from the server. For simplicity, pkt-line
+routines are used to hide chunking and buffering concerns. Each side
+terminates their message with a flush packet.
+(Documentation/technical/protocol-common.txt)
+
+The actual format of the client and server messages is application
+specific. The IPC layer transmits and receives an opaque buffer without
+any concern for the content within.
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-01-12 15:32:33
From: Johannes Schindelin <redacted>
This function currently has only one caller: `apply_multi_file_filter()`
in `convert.c`. That caller wants a flush packet to be written after
writing the payload.
However, we are about to introduce a user that wants to write many
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
Signed-off-by: Johannes Schindelin <redacted>
---
convert.c | 2 +-
pkt-line.c | 5 +++--
pkt-line.h | 3 ++-
3 files changed, 6 insertions(+), 4 deletions(-)
@@ -261,7 +261,8 @@ int write_packetized_from_fd(int fd_in, int fd_out)returnerr;}-intwrite_packetized_from_buf(constchar*src_in,size_tlen,intfd_out)+intwrite_packetized_from_buf(constchar*src_in,size_tlen,intfd_out,+intflush_at_end){interr=0;size_tbytes_written=0;
@@ -277,7 +278,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write);bytes_written+=bytes_to_write;}-if(!err)+if(!err&&flush_at_end)err=packet_flush_gently(fd_out);returnerr;}
@@ -0,0 +1,1093 @@+#include"cache.h"+#include"simple-ipc.h"+#include"strbuf.h"+#include"pkt-line.h"+#include"thread-utils.h"+#include"unix-socket.h"++#ifdef NO_UNIX_SOCKETS+#error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets+#endif++enumipc_active_stateipc_get_active_state(constchar*path)+{+enumipc_active_statestate=IPC_STATE__OTHER_ERROR;+structipc_client_connect_optionsoptions+=IPC_CLIENT_CONNECT_OPTIONS_INIT;+structstatst;+intfd_test=-1;++options.wait_if_busy=0;+options.wait_if_not_found=0;++if(lstat(path,&st)==-1){+switch(errno){+caseENOENT:+caseENOTDIR:+returnIPC_STATE__NOT_LISTENING;+default:+returnIPC_STATE__INVALID_PATH;+}+}++/* also complain if a plain file is in the way */+if((st.st_mode&S_IFMT)!=S_IFSOCK)+returnIPC_STATE__INVALID_PATH;++/*+*JustbecausethefilesystemhasaS_IFSOCKtypeinode+*at`path`,doesn'tmeanitthatthereisaserverlistening.+*Pingittobesure.+*/+state=ipc_client_try_connect(path,&options,&fd_test);+close(fd_test);++returnstate;+}++/*+*Thisvaluewaschosenatrandom.+*/+#define WAIT_STEP_MS (50)++/*+*Trytoconnecttotheserver.Iftheserverisjuststartingupor+*isverybusy,wemaynotgetaconnectionthefirsttime.+*/+staticenumipc_active_stateconnect_to_server(+constchar*path,+inttimeout_ms,+conststructipc_client_connect_options*options,+int*pfd)+{+intwait_ms=50;+intk;++*pfd=-1;++for(k=0;k<timeout_ms;k+=wait_ms){+intfd=unix_stream_connect(path);++if(fd!=-1){+*pfd=fd;+returnIPC_STATE__LISTENING;+}++if(errno==ENOENT){+if(!options->wait_if_not_found)+returnIPC_STATE__PATH_NOT_FOUND;++gotosleep_and_try_again;+}++if(errno==ETIMEDOUT){+if(!options->wait_if_busy)+returnIPC_STATE__NOT_LISTENING;++gotosleep_and_try_again;+}++if(errno==ECONNREFUSED){+if(!options->wait_if_busy)+returnIPC_STATE__NOT_LISTENING;++gotosleep_and_try_again;+}++returnIPC_STATE__OTHER_ERROR;++sleep_and_try_again:+sleep_millisec(wait_ms);+}++returnIPC_STATE__NOT_LISTENING;+}++/*+*Arandomlychosentimeoutvalue.+*/+#define MY_CONNECTION_TIMEOUT_MS (1000)++enumipc_active_stateipc_client_try_connect(+constchar*path,+conststructipc_client_connect_options*options,+int*pfd)+{+enumipc_active_statestate=IPC_STATE__OTHER_ERROR;++*pfd=-1;++trace2_region_enter("ipc-client","try-connect",NULL);+trace2_data_string("ipc-client",NULL,"try-connect/path",path);++state=connect_to_server(path,MY_CONNECTION_TIMEOUT_MS,+options,pfd);++trace2_data_intmax("ipc-client",NULL,"try-connect/state",+(intmax_t)state);+trace2_region_leave("ipc-client","try-connect",NULL);+returnstate;+}++intipc_client_send_command_to_fd(intfd,constchar*message,+structstrbuf*answer)+{+intret=0;++strbuf_setlen(answer,0);++trace2_region_enter("ipc-client","send-command",NULL);++if(write_packetized_from_buf(message,strlen(message),fd,1)<0){+ret=error(_("could not send IPC command"));+gotodone;+}++if(read_packetized_to_strbuf(fd,answer,PACKET_READ_NEVER_DIE)<0){+ret=error(_("could not read IPC response"));+gotodone;+}++done:+trace2_region_leave("ipc-client","send-command",NULL);+returnret;+}++intipc_client_send_command(constchar*path,+conststructipc_client_connect_options*options,+constchar*message,structstrbuf*answer)+{+intfd;+intret=-1;+enumipc_active_statestate;++state=ipc_client_try_connect(path,options,&fd);++if(state!=IPC_STATE__LISTENING)+returnret;++ret=ipc_client_send_command_to_fd(fd,message,answer);+close(fd);+returnret;+}++staticintset_socket_blocking_flag(intfd,intmake_nonblocking)+{+intflags;++flags=fcntl(fd,F_GETFL,NULL);++if(flags<0)+return-1;++if(make_nonblocking)+flags|=O_NONBLOCK;+else+flags&=~O_NONBLOCK;++returnfcntl(fd,F_SETFL,flags);+}++/*+*Magicnumbersusedtoannotatecallbackinstancedata.+*Theseareusedtohelpguardagainstaccidentallypassingthe+*wronginstancedataacrossmultiplelevelsofcallbacks(which+*iseasytodoifthereare`void*`arguments).+*/+enummagic{+MAGIC_SERVER_REPLY_DATA,+MAGIC_WORKER_THREAD_DATA,+MAGIC_ACCEPT_THREAD_DATA,+MAGIC_SERVER_DATA,+};++structipc_server_reply_data{+enummagicmagic;+intfd;+structipc_worker_thread_data*worker_thread_data;+};++structipc_worker_thread_data{+enummagicmagic;+structipc_worker_thread_data*next_thread;+structipc_server_data*server_data;+pthread_tpthread_id;+};++structipc_accept_thread_data{+enummagicmagic;+structipc_server_data*server_data;+intfd_listen;+ino_tinode_listen;+intfd_send_shutdown;+intfd_wait_shutdown;+pthread_tpthread_id;+};++/*+*Withunix-sockets,theconceptual"ipc-server"isimplementedasasingle+*controller"accept-thread"threadandapoolof"worker-thread"threads.+*Theformerdoestheusual`accept()`loopanddispatchesconnections+*toanidleworkerthread.Theworkerthreadswaitinanidleloopfor+*anewconnection,communicatewiththeclientandrelaydatato/from+*the`application_cb`andthenwaitforanotherconnectionfromthe+*serverthread.Thisavoidstheoverheadofconstantlycreatingand+*destroyingthreads.+*/+structipc_server_data{+enummagicmagic;+ipc_server_application_cb*application_cb;+void*application_data;+structstrbufbuf_path;++structipc_accept_thread_data*accept_thread;+structipc_worker_thread_data*worker_thread_list;++pthread_mutex_twork_available_mutex;+pthread_cond_twork_available_cond;++/*+*Acceptedbutnotyetprocessedclientconnectionsarekept+*inacircularbufferFIFO.Thequeueisemptywhenthe+*positionsareequal.+*/+int*fifo_fds;+intqueue_size;+intback_pos;+intfront_pos;++intshutdown_requested;+intis_stopped;+};++/*+*Removeandreturntheoldestqueuedconnection.+*+*Returns-1ifempty.+*/+staticintfifo_dequeue(structipc_server_data*server_data)+{+/* ASSERT holding mutex */++intfd;++if(server_data->back_pos==server_data->front_pos)+return-1;++fd=server_data->fifo_fds[server_data->front_pos];+server_data->fifo_fds[server_data->front_pos]=-1;++server_data->front_pos++;+if(server_data->front_pos==server_data->queue_size)+server_data->front_pos=0;++returnfd;+}++/*+*Pushanewfdontothebackofthequeue.+*+*Dropitandreturn-1ifqueueisalreadyfull.+*/+staticintfifo_enqueue(structipc_server_data*server_data,intfd)+{+/* ASSERT holding mutex */++intnext_back_pos;++next_back_pos=server_data->back_pos+1;+if(next_back_pos==server_data->queue_size)+next_back_pos=0;++if(next_back_pos==server_data->front_pos){+/* Queue is full. Just drop it. */+close(fd);+return-1;+}++server_data->fifo_fds[server_data->back_pos]=fd;+server_data->back_pos=next_back_pos;++returnfd;+}++/*+*WaitforaconnectiontobequeuedtotheFIFOandreturnit.+*+*Returns-1ifsomeonehasalreadyrequestedashutdown.+*/+staticintworker_thread__wait_for_connection(+structipc_worker_thread_data*worker_thread_data)+{+/* ASSERT NOT holding mutex */++structipc_server_data*server_data=worker_thread_data->server_data;+intfd=-1;++pthread_mutex_lock(&server_data->work_available_mutex);+for(;;){+if(server_data->shutdown_requested)+break;++fd=fifo_dequeue(server_data);+if(fd>=0)+break;++pthread_cond_wait(&server_data->work_available_cond,+&server_data->work_available_mutex);+}+pthread_mutex_unlock(&server_data->work_available_mutex);++returnfd;+}++/*+*Forwarddeclareourreplycallbackfunctionsothatanycompiler+*errorsarereportedwhenweactuallydefinethefunction(inaddition+*toanyerrorsreportedwhenwetrytopassthiscallbackfunctionas+*aparameterinafunctioncall).Theformerareeasiertounderstand.+*/+staticipc_server_reply_cbdo_io_reply_callback;++/*+*Relayapplication'sresponsemessagetotheclientprocess.+*(Wedonotflushatthispointbecauseweallowthecaller+*tochunkdatatotheclientthruus.)+*/+staticintdo_io_reply_callback(structipc_server_reply_data*reply_data,+constchar*response,size_tresponse_len)+{+if(reply_data->magic!=MAGIC_SERVER_REPLY_DATA)+BUG("reply_cb called with wrong instance data");++returnwrite_packetized_from_buf(response,response_len,+reply_data->fd,0);+}++/* A randomly chosen value. */+#define MY_WAIT_POLL_TIMEOUT_MS (10)++/*+*Iftheclienthangsupwithoutsendinganydataonthewire,just+*quietlyclosethesocketandignorethisclient.+*+*ThisworkerthreadiscommittedtoreadingtheIPCrequestdata+*fromtheclientattheotherendofthisfd.Waithereforthe+*clienttoactuallyputsomethingonthewire--becauseifthe+*clientjustdoesaping(connectandhangupwithoutsendingany+*data),ouruseofthepkt-linereadroutineswillspewanerror+*message.+*+*Return-1iftheclienthungup.+*Return0ifdata(possiblyincomplete)isready.+*/+staticintworker_thread__wait_for_io_start(+structipc_worker_thread_data*worker_thread_data,+intfd)+{+structipc_server_data*server_data=worker_thread_data->server_data;+structpollfdpollfd[1];+intresult;++for(;;){+pollfd[0].fd=fd;+pollfd[0].events=POLLIN;++result=poll(pollfd,1,MY_WAIT_POLL_TIMEOUT_MS);+if(result<0){+if(errno==EINTR)+continue;+gotocleanup;+}++if(result==0){+/* a timeout */++intin_shutdown;++pthread_mutex_lock(&server_data->work_available_mutex);+in_shutdown=server_data->shutdown_requested;+pthread_mutex_unlock(&server_data->work_available_mutex);++/*+*Ifashutdownisalreadyinprogressandthis+*clienthasnotstartedtalkingyet,justdropit.+*/+if(in_shutdown)+gotocleanup;+continue;+}++if(pollfd[0].revents&POLLHUP)+gotocleanup;++if(pollfd[0].revents&POLLIN)+return0;++gotocleanup;+}++cleanup:+close(fd);+return-1;+}++/*+*Receivetherequest/commandfromtheclientandpassittothe+*registeredrequest-callback.Therequest-callbackwillcompose+*aresponseandcallourreply-callbacktosendittotheclient.+*/+staticintworker_thread__do_io(+structipc_worker_thread_data*worker_thread_data,+intfd)+{+/* ASSERT NOT holding lock */++structstrbufbuf=STRBUF_INIT;+structipc_server_reply_datareply_data;+intret=0;++reply_data.magic=MAGIC_SERVER_REPLY_DATA;+reply_data.worker_thread_data=worker_thread_data;++reply_data.fd=fd;++ret=read_packetized_to_strbuf(reply_data.fd,&buf,+PACKET_READ_NEVER_DIE);+if(ret>=0){+ret=worker_thread_data->server_data->application_cb(+worker_thread_data->server_data->application_data,+buf.buf,do_io_reply_callback,&reply_data);++packet_flush_gently(reply_data.fd);+}+else{+/*+*Theclientprobablydisconnected/shutdownbeforeit+*couldsendawell-formedmessage.Ignoreit.+*/+}++strbuf_release(&buf);+close(reply_data.fd);++returnret;+}++/*+*BlockSIGPIPEonthecurrentthread(sothatwegetEPIPEfrom+*write()ratherthananactualsignal).+*+*Notethatusingsigchain_push()and_pop()tocontrolSIGPIPE+*aroundourIOcallsisnotthreadsafe:+*[]Itusesaglobalstackofhandlerframes.+*[]ItusesALLOC_GROW()toresizeit.+*[]Finally,accordingtothe`signal(2)`man-page:+*"The effects of `signal()` in a multithreaded process are unspecified."+*/+staticvoidthread_block_sigpipe(sigset_t*old_set)+{+sigset_tnew_set;++sigemptyset(&new_set);+sigaddset(&new_set,SIGPIPE);++sigemptyset(old_set);+pthread_sigmask(SIG_BLOCK,&new_set,old_set);+}++/*+*ThreadprocforanIPCworkerthread.Ithandlesaseriesof+*connectionsfromclients.Itpullsthenextfdfromthequeue+*processesit,andthenwaitsforthenextclient.+*+*BlockSIGPIPEinthisworkerthreadforthelifeofthethread.+*Thisavoidsstray(andsometimesdelayed)SIGPIPEsignalscaused+*byclienterrorsand/orwhenweareunderextremelyheavyIOload.+*+*ThismeansthattheapplicationcallbackwillhaveSIGPIPEblocked.+*Thecallbackshouldnotchangeit.+*/+staticvoid*worker_thread_proc(void*_worker_thread_data)+{+structipc_worker_thread_data*worker_thread_data=_worker_thread_data;+structipc_server_data*server_data=worker_thread_data->server_data;+sigset_told_set;+intfd,io;+intret;++trace2_thread_start("ipc-worker");++thread_block_sigpipe(&old_set);++for(;;){+fd=worker_thread__wait_for_connection(worker_thread_data);+if(fd==-1)+break;/* in shutdown */++io=worker_thread__wait_for_io_start(worker_thread_data,fd);+if(io==-1)+continue;/* client hung up without sending anything */++ret=worker_thread__do_io(worker_thread_data,fd);++if(ret==SIMPLE_IPC_QUIT){+trace2_data_string("ipc-worker",NULL,"queue_stop_async",+"application_quit");+/* The application told us to shutdown. */+ipc_server_stop_async(server_data);+break;+}+}++trace2_thread_exit();+returnNULL;+}++/*+*Return1ifsomeonedeletedorstoletheon-disksocketfromus.+*/+staticintsocket_was_stolen(structipc_accept_thread_data*accept_thread_data)+{+structstatst;++if(lstat(accept_thread_data->server_data->buf_path.buf,&st)==-1)+return1;++if(st.st_ino!=accept_thread_data->inode_listen)+return1;++return0;+}++/* A randomly chosen value. */+#define MY_ACCEPT_POLL_TIMEOUT_MS (60 * 1000)++/*+*Acceptanewclientconnectiononoursocket.Thisusesnon-blocking+*IOsothatwecanalsowaitforshutdownrequestsonoursocket-pair+*withoutactuallyspinningonafasttimeout.+*/+staticintaccept_thread__wait_for_connection(+structipc_accept_thread_data*accept_thread_data)+{+structpollfdpollfd[2];+intresult;++for(;;){+pollfd[0].fd=accept_thread_data->fd_wait_shutdown;+pollfd[0].events=POLLIN;++pollfd[1].fd=accept_thread_data->fd_listen;+pollfd[1].events=POLLIN;++result=poll(pollfd,2,MY_ACCEPT_POLL_TIMEOUT_MS);+if(result<0){+if(errno==EINTR)+continue;+returnresult;+}++if(result==0){+/* a timeout */++/*+*Ifsomeonedeletesorforce-createsanewunix+*domainsocketatoutpath,allfutureclients+*willberoutedelsewhereandwesilentlystarve.+*Ifthathappens,justqueueashutdown.+*/+if(socket_was_stolen(+accept_thread_data)){+trace2_data_string("ipc-accept",NULL,+"queue_stop_async",+"socket_stolen");+ipc_server_stop_async(+accept_thread_data->server_data);+}+continue;+}++if(pollfd[0].revents&POLLIN){+/* shutdown message queued to socketpair */+return-1;+}++if(pollfd[1].revents&POLLIN){+/* a connection is available on fd_listen */++intclient_fd=accept(accept_thread_data->fd_listen,+NULL,NULL);+if(client_fd>=0)+returnclient_fd;++/*+*Anerrorhereisunlikely--itprobably+*indicatesthattheconnectingprocesshas+*alreadydroppedtheconnection.+*/+continue;+}++BUG("unandled poll result errno=%d r[0]=%d r[1]=%d",+errno,pollfd[0].revents,pollfd[1].revents);+}+}++/*+*ThreadprocfortheIPCserver"accept thread".Thiswaitsfor+*anincomingsocketconnection,appendsittothequeueofavailable+*connections,andnotifiesaworkerthreadtoprocessit.+*+*BlockSIGPIPEinthisthreadforthelifeofthethread.This+*avoidsanystraySIGPIPEsignalswhenclosingpipefdsunder+*extremelyheavyloads(suchaswhenthefifoqueueisfullandwe+*dropincommingconnections).+*/+staticvoid*accept_thread_proc(void*_accept_thread_data)+{+structipc_accept_thread_data*accept_thread_data=_accept_thread_data;+structipc_server_data*server_data=accept_thread_data->server_data;+sigset_told_set;++trace2_thread_start("ipc-accept");++thread_block_sigpipe(&old_set);++for(;;){+intclient_fd=accept_thread__wait_for_connection(+accept_thread_data);++pthread_mutex_lock(&server_data->work_available_mutex);+if(server_data->shutdown_requested){+pthread_mutex_unlock(&server_data->work_available_mutex);+if(client_fd>=0)+close(client_fd);+break;+}++if(client_fd<0){+/* ignore transient accept() errors */+}+else{+fifo_enqueue(server_data,client_fd);+pthread_cond_broadcast(&server_data->work_available_cond);+}+pthread_mutex_unlock(&server_data->work_available_mutex);+}++trace2_thread_exit();+returnNULL;+}++/*+*Wecan'tpredicttheconnectionarrivalraterelativetotheworker+*processingrate,thereforeweallowthe"accept-thread"toqueueup+*agenerousnumberofconnections,sincewe'dratherhavetheclient+*notunnecessarilytimeoutifwecanavoidit.(Theassumptionis+*thatthiswillbeusedforFSMonitorandafewsecondwaitona+*connectionisbetterthanhavingtheclienttimeoutanddothefull+*computationitself.)+*+*TheFIFOqueuesizeissettoamultipleoftheworkerpoolsize.+*Thisvaluechosenatrandom.+*/+#define FIFO_SCALE (100)++/*+*Thebacklogvaluefor`listen(2)`.Thisdoesn'tneedtohuge,+*ratherjustlargeenoughforour"accept-thread"towakeupand+*queueincomingconnectionsontotheFIFOwithoutthekernel+*droppingany.+*+*Thisvaluechosenatrandom.+*/+#define LISTEN_BACKLOG (50)++/*+*Createaunixdomainsocketatthegivenpathtolistenfor+*clientconnections.Theresultingsocketwillthenappear+*inthefilesystemasaninodewithS_IFSOCK.Theinodeis+*itselfcreatedaspartofthe`bind(2)`operation.+*+*Theterm"socket"isambiguousinthiscontext.Wewanttoopena+*"socket-fd"thatisboundtoa"socket-inode"(path)ondisk.We+*listenon"socket-fd"fornewconnectionsandclientstryto+*open/connectusingthe"socket-inode"pathname.+*+*Unixdomainsocketshaveafundamentaldesignflawbecausethe+*"socket-inode"persistsuntilthepathnameisdeleted;closingthelistening+*"socket-fd"onlyclosesthesockethandle/descriptor,itdoesnotdelete+*theinode/pathname.+*+*Well-behavingservicedaemonsareexpectedtoalsodeletetheinode+*beforeshutdown.Ifaservicecrashes(orforgets)itcanleave+*the(nowstale)inodeinthefilesystem.Thisbehaveslikeastale+*".lock"fileandmaypreventfutureserviceinstancesfromstarting+*upcorrectly.(Becausetheywon'tbeabletobind.)+*+*Whenfutureserviceinstancestrytocreatethelistenersocket,+*`bind(2)`willfailwithEADDRINUSE--becausetheinodealready+*exists.However,thenewinstancecannottellifitisastale+*inode*or*anotherserviceinstanceisalreadyrunning.+*+*Onepossiblesolutionistoblindlyunlinktheinodebefore+*attemptingtobindanewsocket-fd(andthuscreate)anew+*socket-inode.Then`bind(2)`shouldalwayssucceed.However,if+*thereisanexistingserviceinstance,itwouldbeorphaned--+*itwouldstillbelisteningonasocket-fdthatisstillbound+*toan(unlinked)socket-inode,butthatsocket-inodeisnolonger+*associatedwiththepathname.Newclientconnectionswillarrive+*atournewsocket-inodeandnottheexistingserver's.(Itisupto+*theexistingservertodetectthatitssocket-inodehasbeen+*stolenandshutdown.)+*+*Sincethisisratherobscureandinfrequent,wetryto"gently"+*createthesocket-inodewithoutdisturbinganexistingservice.+*/+staticintcreate_listener_socket(constchar*path,+conststructipc_server_opts*ipc_opts)+{+intfd_listen;+intfd_client;+structunix_stream_listen_optsuslg_opts={+.listen_backlog_size=LISTEN_BACKLOG,+.force_unlink_before_bind=0,+.disallow_chdir=ipc_opts->uds_disallow_chdir+};++trace2_data_string("ipc-server",NULL,"try-listen-gently",path);++/*+*Assumesocket-inodedoesnotexistandtryto(gently)+*createanewsocket-inodeondiskatpathnameandbind+*socket-fdtoit.+*/+fd_listen=unix_stream_listen_gently(path,&uslg_opts);+if(fd_listen>=0)+returnfd_listen;++if(errno!=EADDRINUSE)+returnerror_errno(_("could not create socket '%s'"),+path);++trace2_data_string("ipc-server",NULL,"try-detect-server",path);++/*+*Asocket-inodeatpathnameexistsondisk,butwedon't+*knowifitaserverisusingitorifitisastaleinode.+*+*pokeitwithatrivialconnectiontotrytofindout.+*/+fd_client=unix_stream_connect(path);+if(fd_client>=0){+/*+*Anexistingserviceprocessisaliveandacceptedour+*connection.+*/+close(fd_client);++/*+*Wecannotcreateanewsocket-inodehere,sowecannot+*startupanewserveronthispathname.+*/+errno=EADDRINUSE;+returnerror_errno(_("socket already in use '%s'"),+path);+}++trace2_data_string("ipc-server",NULL,"try-listen-force",path);++/*+*Asocket-inodeatpathnameexistsondisk,butwewerenot+*abletoconnecttoit,sowebelievethatthisisastale+*socket-inodethatapreviousserverforgottodelete.Use+*thetradionalsolution:forceunlinkitandcreateanew+*one.+*+*TODONotethatitispossiblethatanotherserveris+*listening,butiseitherjuststartingupandnotyet+*responsiveorisstucksomehow.Fornow,I'mOKwith+*stealingthesocket-inodefromitinthiscase.+*/+uslg_opts.force_unlink_before_bind=1;+fd_listen=unix_stream_listen_gently(path,&uslg_opts);+if(fd_listen>=0)+returnfd_listen;++returnerror_errno(_("could not force create socket '%s'"),path);+}++staticintsetup_listener_socket(constchar*path,ino_t*inode,+conststructipc_server_opts*ipc_opts)+{+intfd_listen;+structstatst;++trace2_region_enter("ipc-server","create-listener_socket",NULL);+fd_listen=create_listener_socket(path,ipc_opts);+trace2_region_leave("ipc-server","create-listener_socket",NULL);++if(fd_listen<0)+returnfd_listen;++/*+*Wejustboundasocket(descriptor)toanewlycreatedunix+*domainsocketinthefilesystem.Capturetheinodenumber+*sowecanlaterdetectif/whensomeoneelseforce-createsa+*newsocketandeffectivelystealsthepathfromus.(Which+*wouldleaveuslisteningtoasocketthatnoclientcould+*reach.)+*/+if(lstat(path,&st)<0){+intsaved_errno=errno;++close(fd_listen);+unlink(path);++errno=saved_errno;+returnerror_errno(_("could not lstat listener socket '%s'"),+path);+}++if(set_socket_blocking_flag(fd_listen,1)){+intsaved_errno=errno;++close(fd_listen);+unlink(path);++errno=saved_errno;+returnerror_errno(_("making listener socket nonblocking '%s'"),+path);+}++*inode=st.st_ino;++returnfd_listen;+}++/*+*StartIPCserverinapoolofbackgroundthreads.+*/+intipc_server_run_async(structipc_server_data**returned_server_data,+constchar*path,conststructipc_server_opts*opts,+ipc_server_application_cb*application_cb,+void*application_data)+{+structipc_server_data*server_data;+intfd_listen;+ino_tinode_listen;+intsv[2];+intk;+intnr_threads=opts->nr_threads;++*returned_server_data=NULL;++/*+*Createasocketpairandsetsv[1]tonon-blocking.This+*willusedtosendashutdownmessagetotheaccept-thread+*andallowstheaccept-threadtowaitonEITHERaclient+*connectionorashutdownrequestwithoutspinning.+*/+if(socketpair(AF_UNIX,SOCK_STREAM,0,sv)<0)+returnerror_errno(_("could not create socketpair for '%s'"),+path);++if(set_socket_blocking_flag(sv[1],1)){+intsaved_errno=errno;+close(sv[0]);+close(sv[1]);+errno=saved_errno;+returnerror_errno(_("making socketpair nonblocking '%s'"),+path);+}++fd_listen=setup_listener_socket(path,&inode_listen,opts);+if(fd_listen<0){+intsaved_errno=errno;+close(sv[0]);+close(sv[1]);+errno=saved_errno;+return-1;+}++server_data=xcalloc(1,sizeof(*server_data));+server_data->magic=MAGIC_SERVER_DATA;+server_data->application_cb=application_cb;+server_data->application_data=application_data;+strbuf_init(&server_data->buf_path,0);+strbuf_addstr(&server_data->buf_path,path);++if(nr_threads<1)+nr_threads=1;++pthread_mutex_init(&server_data->work_available_mutex,NULL);+pthread_cond_init(&server_data->work_available_cond,NULL);++server_data->queue_size=nr_threads*FIFO_SCALE;+server_data->fifo_fds=xcalloc(server_data->queue_size,+sizeof(*server_data->fifo_fds));++server_data->accept_thread=+xcalloc(1,sizeof(*server_data->accept_thread));+server_data->accept_thread->magic=MAGIC_ACCEPT_THREAD_DATA;+server_data->accept_thread->server_data=server_data;+server_data->accept_thread->fd_listen=fd_listen;+server_data->accept_thread->inode_listen=inode_listen;+server_data->accept_thread->fd_send_shutdown=sv[0];+server_data->accept_thread->fd_wait_shutdown=sv[1];++if(pthread_create(&server_data->accept_thread->pthread_id,NULL,+accept_thread_proc,server_data->accept_thread))+die_errno(_("could not start accept_thread '%s'"),path);++for(k=0;k<nr_threads;k++){+structipc_worker_thread_data*wtd;++wtd=xcalloc(1,sizeof(*wtd));+wtd->magic=MAGIC_WORKER_THREAD_DATA;+wtd->server_data=server_data;++if(pthread_create(&wtd->pthread_id,NULL,worker_thread_proc,+wtd)){+if(k==0)+die(_("could not start worker[0] for '%s'"),+path);+/*+*Limpalongwiththethreadpoolthatwehave.+*/+break;+}++wtd->next_thread=server_data->worker_thread_list;+server_data->worker_thread_list=wtd;+}++*returned_server_data=server_data;+return0;+}++/*+*GentlytelltheIPCservertreadstoshutdown.+*Canberunonanythread.+*/+intipc_server_stop_async(structipc_server_data*server_data)+{+/* ASSERT NOT holding mutex */++intfd;++if(!server_data)+return0;++trace2_region_enter("ipc-server","server-stop-async",NULL);++pthread_mutex_lock(&server_data->work_available_mutex);++server_data->shutdown_requested=1;++/*+*Writeabytetotheshutdownsocketpairtowakeupthe+*accept-thread.+*/+if(write(server_data->accept_thread->fd_send_shutdown,"Q",1)<0)+error_errno("could not write to fd_send_shutdown");++/*+*Drainthequeueofexistingconnections.+*/+while((fd=fifo_dequeue(server_data))!=-1)+close(fd);++/*+*Gentlytellworkerthreadstostopprocessingnewconnections+*andexit.(Thisdoesnotabortin-processconversations.)+*/+pthread_cond_broadcast(&server_data->work_available_cond);++pthread_mutex_unlock(&server_data->work_available_mutex);++trace2_region_leave("ipc-server","server-stop-async",NULL);++return0;+}++/*+*WaitforallIPCserverthreadstostop.+*/+intipc_server_await(structipc_server_data*server_data)+{+pthread_join(server_data->accept_thread->pthread_id,NULL);++if(!server_data->shutdown_requested)+BUG("ipc-server: accept-thread stopped for '%s'",+server_data->buf_path.buf);++while(server_data->worker_thread_list){+structipc_worker_thread_data*wtd=+server_data->worker_thread_list;++pthread_join(wtd->pthread_id,NULL);++server_data->worker_thread_list=wtd->next_thread;+free(wtd);+}++server_data->is_stopped=1;++return0;+}++voidipc_server_free(structipc_server_data*server_data)+{+structipc_accept_thread_data*accept_thread_data;++if(!server_data)+return;++if(!server_data->is_stopped)+BUG("cannot free ipc-server while running for '%s'",+server_data->buf_path.buf);++accept_thread_data=server_data->accept_thread;+if(accept_thread_data){+if(accept_thread_data->fd_listen!=-1){+/*+*Onlyunlinktheunixdomainsocketifwe+*createdit.Thatis,ifanotherdaemon+*processforce-createdanewsocketatthis+*path,andeffectivelystealsourpath+*(whichpreventsusfromreceivingany+*futureclients),wedon'twanttodothe+*samethingtothem.+*/+if(!socket_was_stolen(+accept_thread_data))+unlink(server_data->buf_path.buf);++close(accept_thread_data->fd_listen);+}+if(accept_thread_data->fd_send_shutdown!=-1)+close(accept_thread_data->fd_send_shutdown);+if(accept_thread_data->fd_wait_shutdown!=-1)+close(accept_thread_data->fd_wait_shutdown);++free(server_data->accept_thread);+}++while(server_data->worker_thread_list){+structipc_worker_thread_data*wtd=+server_data->worker_thread_list;++server_data->worker_thread_list=wtd->next_thread;+free(wtd);+}++pthread_cond_destroy(&server_data->work_available_cond);+pthread_mutex_destroy(&server_data->work_available_mutex);++strbuf_release(&server_data->buf_path);++free(server_data->fifo_fds);+free(server_data);+}
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-01-12 15:33:13
From: Jeff Hostetler <redacted>
Calls to `chdir()` are dangerous in a multi-threaded context. If
`unix_stream_listen()` is given a socket pathname that is too big to
fit in a `sockaddr_un` structure, it will `chdir()` to the parent
directory of the requested socket pathname, create the socket using a
relative pathname, and then `chdir()` back. This is not thread-safe.
Add `disallow_chdir` flag to `struct unix_sockaddr_context` and change
all callers to pass an initialized context structure.
Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag
is set.
Extend the public interface to `unix_stream_listen_gently()` to also
expose this new flag.
Signed-off-by: Jeff Hostetler <redacted>
---
unix-socket.c | 21 +++++++++++++++++----
unix-socket.h | 1 +
2 files changed, 18 insertions(+), 4 deletions(-)
@@ -0,0 +1,485 @@+/*+*test-simple-ipc.c:verifythattheInter-ProcessCommunicationworks.+*/++#include"test-tool.h"+#include"cache.h"+#include"strbuf.h"+#include"simple-ipc.h"+#include"parse-options.h"+#include"thread-utils.h"++#ifndef SUPPORTS_SIMPLE_IPC+intcmd__simple_ipc(intargc,constchar**argv)+{+die("simple IPC not available on this platform");+}+#else++/*+*Thetestdaemondefinesan"application callback"thatsupportsa+*seriesofcommands(see`test_app_cb()`).+*+*Unknowncommandsarecaughthereandwesendanerrormessageback+*totheclientprocess.+*/+staticintapp__unhandled_command(constchar*command,+ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+intret;++strbuf_addf(&buf,"unhandled command: %s",command);+ret=reply_cb(reply_data,buf.buf,buf.len);+strbuf_release(&buf);++returnret;+}++/*+*Replywithasingleverylargebuffer.Thisistoensurethat+*longresponseareproperlyhandled--whetherthechunkingoccurs+*inthekernelorinthe(probablypkt-line)layer.+*/+#define BIG_ROWS (10000)+staticintapp__big_command(ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+introw;+intret;++for(row=0;row<BIG_ROWS;row++)+strbuf_addf(&buf,"big: %.75d\n",row);++ret=reply_cb(reply_data,buf.buf,buf.len);+strbuf_release(&buf);++returnret;+}++/*+*Replywithaseriesoflines.Thisistoensurethatwecanincrementally+*computetheresponseandchunkittotheclient.+*/+#define CHUNK_ROWS (10000)+staticintapp__chunk_command(ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+introw;+intret;++for(row=0;row<CHUNK_ROWS;row++){+strbuf_setlen(&buf,0);+strbuf_addf(&buf,"big: %.75d\n",row);+ret=reply_cb(reply_data,buf.buf,buf.len);+}++strbuf_release(&buf);++returnret;+}++/*+*Slowlyreplywithaseriesoflines.Thisistomodelanexpensiveto+*computechunkedresponse(whichmighthappenifthiscallbackisrunning+*inathreadandisfightingforalockwithotherthreads).+*/+#define SLOW_ROWS (1000)+#define SLOW_DELAY_MS (10)+staticintapp__slow_command(ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+introw;+intret;++for(row=0;row<SLOW_ROWS;row++){+strbuf_setlen(&buf,0);+strbuf_addf(&buf,"big: %.75d\n",row);+ret=reply_cb(reply_data,buf.buf,buf.len);+sleep_millisec(SLOW_DELAY_MS);+}++strbuf_release(&buf);++returnret;+}++/*+*Theclientsentacommandfollowedbya(possiblyvery)largebuffer.+*/+staticintapp__sendbytes_command(constchar*received,+ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf_resp=STRBUF_INIT;+constchar*p="?";+intlen_ballast=0;+intk;+interrs=0;+intret;++if(skip_prefix(received,"sendbytes ",&p))+len_ballast=strlen(p);++/*+*Verifythattheballastisncopiesofasingleletter.+*Andthatthemulti-threadedIOlayerdidn'tcrossthestreams.+*/+for(k=1;k<len_ballast;k++)+if(p[k]!=p[0])+errs++;++if(errs)+strbuf_addf(&buf_resp,"errs:%d\n",errs);+else+strbuf_addf(&buf_resp,"rcvd:%c%08d\n",p[0],len_ballast);++ret=reply_cb(reply_data,buf_resp.buf,buf_resp.len);++strbuf_release(&buf_resp);++returnret;+}++/*+*Anarbitraryfixedaddresstoverifythattheapplicationinstance+*dataishandledproperly.+*/+staticintmy_app_data=42;++staticipc_server_application_cbtest_app_cb;++/*+*Thisis"application callback"thatsitsontopofthe"ipc-server".+*Itcompletelydefinesthesetofcommandverbssupportedbythis+*application.+*/+staticinttest_app_cb(void*application_data,+constchar*command,+ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+/*+*Verifythatwereceivedtheapplication-datathatwepassed+*whenwestartedtheipc-server.(Wehaveseverallayersof+*callbackscallingcallbacksandit'seasytogetthingsmixed+*up(especiallywhensomeare"void*").)+*/+if(application_data!=(void*)&my_app_data)+BUG("application_cb: application_data pointer wrong");++if(!strcmp(command,"quit")){+/*+*Tellipc-servertohangupwithanemptyreply.+*/+returnSIMPLE_IPC_QUIT;+}++if(!strcmp(command,"ping")){+constchar*answer="pong";+returnreply_cb(reply_data,answer,strlen(answer));+}++if(!strcmp(command,"big"))+returnapp__big_command(reply_cb,reply_data);++if(!strcmp(command,"chunk"))+returnapp__chunk_command(reply_cb,reply_data);++if(!strcmp(command,"slow"))+returnapp__slow_command(reply_cb,reply_data);++if(starts_with(command,"sendbytes "))+returnapp__sendbytes_command(command,reply_cb,reply_data);++returnapp__unhandled_command(command,reply_cb,reply_data);+}++/*+*Thisprocesswillrunasasimple-ipcserverandlistenforIPCcommands+*fromclientprocesses.+*/+staticintdaemon__run_server(constchar*path,intargc,constchar**argv)+{+structipc_server_optsopts={+.nr_threads=5+};++constchar*constdaemon_usage[]={+N_("test-helper simple-ipc daemon [<options>"),+NULL+};+structoptiondaemon_options[]={+OPT_INTEGER(0,"threads",&opts.nr_threads,+N_("number of threads in server thread pool")),+OPT_END()+};++argc=parse_options(argc,argv,NULL,daemon_options,daemon_usage,0);++if(opts.nr_threads<1)+opts.nr_threads=1;++/*+*Synchronouslyruntheipc-server.Wedon'tneedanyapplication+*instancedata,sopassanarbitrarypointer(thatwe'lllater+*verifymadetheroundtrip).+*/+returnipc_server_run(path,&opts,test_app_cb,(void*)&my_app_data);+}++/*+*Thisprocesswillrunaquickprobetoseeifasimple-ipcserver+*isactiveonthispath.+*+*Returns0iftheserverisalive.+*/+staticintclient__probe_server(constchar*path)+{+enumipc_active_states;++s=ipc_get_active_state(path);+switch(s){+caseIPC_STATE__LISTENING:+return0;++caseIPC_STATE__NOT_LISTENING:+returnerror("no server listening at '%s'",path);++caseIPC_STATE__PATH_NOT_FOUND:+returnerror("path not found '%s'",path);++caseIPC_STATE__INVALID_PATH:+returnerror("invalid pipe/socket name '%s'",path);++caseIPC_STATE__OTHER_ERROR:+default:+returnerror("other error for '%s'",path);+}+}++/*+*SendanIPCcommandtoanalready-runningserverdaemonandprintthe+*response.+*+*argv[2]containsasimple(1word)commandverbthat`test_app_cb()`+*(inthedaemonprocess)willunderstand.+*/+staticintclient__send_ipc(intargc,constchar**argv,constchar*path)+{+constchar*command=argc>2?argv[2]:"(no command)";+structstrbufbuf=STRBUF_INIT;+structipc_client_connect_optionsoptions+=IPC_CLIENT_CONNECT_OPTIONS_INIT;++options.wait_if_busy=1;+options.wait_if_not_found=0;++if(!ipc_client_send_command(path,&options,command,&buf)){+printf("%s\n",buf.buf);+fflush(stdout);+strbuf_release(&buf);++return0;+}++returnerror("failed to send '%s' to '%s'",command,path);+}++/*+*SendanIPCcommandfollowedbyballasttoconfirmthatalarge+*messagecanbesentandthatthekernelorpkt-linelayerswill+*properlychunkitandthatthedaemonreceivestheentiremessage.+*/+staticintdo_sendbytes(intbytecount,charbyte,constchar*path)+{+structstrbufbuf_send=STRBUF_INIT;+structstrbufbuf_resp=STRBUF_INIT;+structipc_client_connect_optionsoptions+=IPC_CLIENT_CONNECT_OPTIONS_INIT;++options.wait_if_busy=1;+options.wait_if_not_found=0;++strbuf_addstr(&buf_send,"sendbytes ");+strbuf_addchars(&buf_send,byte,bytecount);++if(!ipc_client_send_command(path,&options,buf_send.buf,&buf_resp)){+strbuf_rtrim(&buf_resp);+printf("sent:%c%08d %s\n",byte,bytecount,buf_resp.buf);+fflush(stdout);+strbuf_release(&buf_send);+strbuf_release(&buf_resp);++return0;+}++returnerror("client failed to sendbytes(%d, '%c') to '%s'",+bytecount,byte,path);+}++/*+*SendanIPCcommandwithballasttoanalready-runningserverdaemon.+*/+staticintclient__sendbytes(intargc,constchar**argv,constchar*path)+{+intbytecount=1024;+char*string="x";+constchar*constsendbytes_usage[]={+N_("test-helper simple-ipc sendbytes [<options>]"),+NULL+};+structoptionsendbytes_options[]={+OPT_INTEGER(0,"bytecount",&bytecount,N_("number of bytes")),+OPT_STRING(0,"byte",&string,N_("byte"),N_("ballast")),+OPT_END()+};++argc=parse_options(argc,argv,NULL,sendbytes_options,sendbytes_usage,0);++returndo_sendbytes(bytecount,string[0],path);+}++structmultiple_thread_data{+pthread_tpthread_id;+structmultiple_thread_data*next;+constchar*path;+intbytecount;+intbatchsize;+intsum_errors;+intsum_good;+charletter;+};++staticvoid*multiple_thread_proc(void*_multiple_thread_data)+{+structmultiple_thread_data*d=_multiple_thread_data;+intk;++trace2_thread_start("multiple");++for(k=0;k<d->batchsize;k++){+if(do_sendbytes(d->bytecount+k,d->letter,d->path))+d->sum_errors++;+else+d->sum_good++;+}++trace2_thread_exit();+returnNULL;+}++/*+*Startaclient-sidethreadpool.Eachthreadsendsaseriesof+*IPCrequests.Eachrequestisonanewconnectiontotheserver.+*/+staticintclient__multiple(intargc,constchar**argv,constchar*path)+{+structmultiple_thread_data*list=NULL;+intk;+intnr_threads=5;+intbytecount=1;+intbatchsize=10;+intsum_join_errors=0;+intsum_thread_errors=0;+intsum_good=0;++constchar*constmultiple_usage[]={+N_("test-helper simple-ipc multiple [<options>]"),+NULL+};+structoptionmultiple_options[]={+OPT_INTEGER(0,"bytecount",&bytecount,N_("number of bytes")),+OPT_INTEGER(0,"threads",&nr_threads,N_("number of threads")),+OPT_INTEGER(0,"batchsize",&batchsize,N_("number of requests per thread")),+OPT_END()+};++argc=parse_options(argc,argv,NULL,multiple_options,multiple_usage,0);++if(bytecount<1)+bytecount=1;+if(nr_threads<1)+nr_threads=1;+if(batchsize<1)+batchsize=1;++for(k=0;k<nr_threads;k++){+structmultiple_thread_data*d=xcalloc(1,sizeof(*d));+d->next=list;+d->path=path;+d->bytecount=bytecount+batchsize*(k/26);+d->batchsize=batchsize;+d->sum_errors=0;+d->sum_good=0;+d->letter='A'+(k%26);++if(pthread_create(&d->pthread_id,NULL,multiple_thread_proc,d)){+warning("failed to create thread[%d] skipping remainder",k);+free(d);+break;+}++list=d;+}++while(list){+structmultiple_thread_data*d=list;++if(pthread_join(d->pthread_id,NULL))+sum_join_errors++;++sum_thread_errors+=d->sum_errors;+sum_good+=d->sum_good;++list=d->next;+free(d);+}++printf("client (good %d) (join %d), (errors %d)\n",+sum_good,sum_join_errors,sum_thread_errors);++return(sum_join_errors+sum_thread_errors)?1:0;+}++intcmd__simple_ipc(intargc,constchar**argv)+{+constchar*path="ipc-test";++if(argc==2&&!strcmp(argv[1],"SUPPORTS_SIMPLE_IPC"))+return0;++/* Use '!!' on all dispatch functions to map from `error()` style+*(returns-1)styleto`test_must_fail`style(expects1)and+*getlessconfusingshellerrormessages.+*/++if(argc==2&&!strcmp(argv[1],"is-active"))+return!!client__probe_server(path);++if(argc>=2&&!strcmp(argv[1],"daemon"))+return!!daemon__run_server(path,argc,argv);++/*+*Clientcommandsfollow.Ensureaserverisrunningbefore+*goinganyfurther.+*/+if(client__probe_server(path))+return1;++if((argc==2||argc==3)&&!strcmp(argv[1],"send"))+return!!client__send_ipc(argc,argv,path);++if(argc>=2&&!strcmp(argv[1],"sendbytes"))+return!!client__sendbytes(argc,argv,path);++if(argc>=2&&!strcmp(argv[1],"multiple"))+return!!client__multiple(argc,argv,path);++die("Unhandled argv[1]: '%s'",argv[1]);+}+#endif
@@ -0,0 +1,129 @@+#!/bin/sh++test_description='simple command server'++../test-lib.sh++test-toolsimple-ipcSUPPORTS_SIMPLE_IPC||{+skip_all='simple IPC not supported on this platform'+test_done+}++stop_simple_IPC_server(){+test-n"$SIMPLE_IPC_PID"||return0++kill"$SIMPLE_IPC_PID"&&+SIMPLE_IPC_PID=+}++test_expect_success'start simple command server''+{test-toolsimple-ipcdaemon--threads=8&}&&+SIMPLE_IPC_PID=$!&&+test_atexitstop_simple_IPC_server&&++sleep1&&++test-toolsimple-ipcis-active+'++test_expect_success'simple command server''+test-toolsimple-ipcsendping>actual&&+echopong>expect&&+test_cmpexpectactual+'++test_expect_success'servers cannot share the same path''+test_must_failtest-toolsimple-ipcdaemon&&+test-toolsimple-ipcis-active+'++test_expect_success'big response''+test-toolsimple-ipcsendbig>actual&&+test_line_count-ge10000actual&&+grep-q"big: [0]*9999\$"actual+'++test_expect_success'chunk response''+test-toolsimple-ipcsendchunk>actual&&+test_line_count-ge10000actual&&+grep-q"big: [0]*9999\$"actual+'++test_expect_success'slow response''+test-toolsimple-ipcsendslow>actual&&+test_line_count-ge100actual&&+grep-q"big: [0]*99\$"actual+'++# Send an IPC with n=100,000 bytes of ballast. This should be large enough+# to force both the kernel and the pkt-line layer to chunk the message to the+# daemon and for the daemon to receive it in chunks.+#+test_expect_success'sendbytes''+test-toolsimple-ipcsendbytes--bytecount=100000--byte=A>actual&&+grep"sent:A00100000 rcvd:A00100000"actual+'++# Start a series of <threads> client threads that each make <batchsize>+# IPC requests to the server. Each (<threads> * <batchsize>) request+# will open a new connection to the server and randomly bind to a server+# thread. Each client thread exits after completing its batch. So the+# total number of live client threads will be smaller than the total.+# Each request will send a message containing at least <bytecount> bytes+# of ballast. (Responses are small.)+#+# The purpose here is to test threading in the server and responding to+# many concurrent client requests (regardless of whether they come from+# 1 client process or many). And to test that the server side of the+# named pipe/socket is stable. (On Windows this means that the server+# pipe is properly recycled.)+#+# On Windows it also lets us adjust the connection timeout in the+# `ipc_client_send_command()`.+#+# Note it is easy to drive the system into failure by requesting an+# insane number of threads on client or server and/or increasing the+# per-thread batchsize or the per-request bytecount (ballast).+# On Windows these failures look like "pipe is busy" errors.+# So I've chosen fairly conservative values for now.+#+# We expect output of the form "sent:<letter><length> ..."+# With terms (7, 19, 13) we expect:+# <letter> in [A-G]+# <length> in [19+0 .. 19+(13-1)]+# and (7 * 13) successful responses.+#+test_expect_success'stress test threads''+test-toolsimple-ipcmultiple\+--threads=7\+--bytecount=19\+--batchsize=13\+>actual&&+test_line_count=92actual&&+grep"good 91"actual&&+grep"sent:A"<actual>actual_a&&+cat>expect_a<<-EOF&&+sent:A00000019rcvd:A00000019+sent:A00000020rcvd:A00000020+sent:A00000021rcvd:A00000021+sent:A00000022rcvd:A00000022+sent:A00000023rcvd:A00000023+sent:A00000024rcvd:A00000024+sent:A00000025rcvd:A00000025+sent:A00000026rcvd:A00000026+sent:A00000027rcvd:A00000027+sent:A00000028rcvd:A00000028+sent:A00000029rcvd:A00000029+sent:A00000030rcvd:A00000030+sent:A00000031rcvd:A00000031+EOF+test_cmpexpect_aactual_a+'++test_expect_success'`quit` works''+test-toolsimple-ipcsendquit&&+test_must_failtest-toolsimple-ipcis-active&&+test_must_failtest-toolsimple-ipcsendping+'++test_done
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-01-12 15:33:14
From: Jeff Hostetler <redacted>
Create a gentle version of `unix_stream_listen()`. This version does
not call `die()` if a socket-fd cannot be created and does not assume
that it is safe to `unlink()` an existing socket-inode.
`unix_stream_listen()` uses `unix_stream_socket()` helper function to
create the socket-fd. Avoid that helper because it calls `die()` on
errors.
`unix_stream_listen()` always tries to `unlink()` the socket-path before
calling `bind()`. If there is an existing server/daemon already bound
and listening on that socket-path, our `unlink()` would have the effect
of disassociating the existing server's bound-socket-fd from the socket-path
without notifying the existing server. The existing server could continue
to service existing connections (accepted-socket-fd's), but would not
receive any futher new connections (since clients rendezvous via the
socket-path). The existing server would effectively be offline but yet
appear to be active.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
interval between the forced `unlink()` and the subsequent `bind()` (which
recreates the socket-path that is bound to a new socket-fd in the current
process).
Signed-off-by: Jeff Hostetler <redacted>
---
unix-socket.c | 39 +++++++++++++++++++++++++++++++++++++++
unix-socket.h | 8 ++++++++
2 files changed, 47 insertions(+)
From: Jeff King <hidden> Date: 2021-01-13 14:07:27
On Tue, Jan 12, 2021 at 03:31:29PM +0000, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <redacted>
Create a gentle version of `unix_stream_listen()`. This version does
not call `die()` if a socket-fd cannot be created and does not assume
that it is safe to `unlink()` an existing socket-inode.
The existing one is meant to be gentle. Maybe it is worth fixing it
instead.
`unix_stream_listen()` uses `unix_stream_socket()` helper function to
create the socket-fd. Avoid that helper because it calls `die()` on
errors.
Yeah, I think this is just a bug. My thinking in the original was that
socket() would basically never fail. And it generally wouldn't, but
things like EMFILE do happen. There are only two callers, and both would
be one-liners to propagate the error up the stack.
`unix_stream_listen()` always tries to `unlink()` the socket-path before
calling `bind()`. If there is an existing server/daemon already bound
and listening on that socket-path, our `unlink()` would have the effect
of disassociating the existing server's bound-socket-fd from the socket-path
without notifying the existing server. The existing server could continue
to service existing connections (accepted-socket-fd's), but would not
receive any futher new connections (since clients rendezvous via the
socket-path). The existing server would effectively be offline but yet
appear to be active.
The trouble here is that one cannot tell if the existing file is active,
and you are orphaning an existing server, or if there is leftover cruft
from an exited server that did not clean up after itself (you will get
EADDRINUSE either way).
Handling those cases (and especially doing so in a non-racy way) is
probably outside the scope of unix_stream_listen(), but it makes sense
for this to be an option. And it looks like you even made it so here,
so unix_stream_listen() could just become a wrapper that sets the
option. Or since there is only one caller in the whole code-base,
perhaps it could just learn to pass the option struct. :)
Likewise for the no-chdir option added in the follow-on patch.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
interval between the forced `unlink()` and the subsequent `bind()` (which
recreates the socket-path that is bound to a new socket-fd in the current
process).
I'll be curious to see how you do this atomically. From my skim of patch
10, you will connect to see if it's active, and unlink if it's not. But
then two simultaneous new processes could both see an inactive one and
race to forcefully create the new one. One of them will lose and be
orphaned with a socket that has no filesystem name.
There might be a solution using link() to have an atomic winner, but it
gets tricky around unlinking the old name out of the way. You might need
a separate dot-lock to make sure only one process does the
unlink-and-create process at a time.
-Peff
From: Chris Torek <hidden> Date: 2021-01-14 01:43:42
I had saved this to comment on, but Peff beat me to it :-)
On Wed, Jan 13, 2021 at 6:07 AM Jeff King [off-list ref] wrote:
There might be a solution using link() to have an atomic winner, but it
gets tricky around unlinking the old name out of the way.
You definitely should be able to do this atomically with link(), but
the cleanup is indeed messy, and there's already existing locking
code, so it's probably better to press that into service here.
Chris
@@ -0,0 +1,723 @@+#include"cache.h"+#include"simple-ipc.h"+#include"strbuf.h"+#include"pkt-line.h"+#include"thread-utils.h"++#ifndef GIT_WINDOWS_NATIVE+#error This file can only be compiled on Windows+#endif++staticintinitialize_pipe_name(constchar*path,wchar_t*wpath,size_talloc)+{+intoff=0;+structstrbufrealpath=STRBUF_INIT;++if(!strbuf_realpath(&realpath,path,0))+return-1;++off=swprintf(wpath,alloc,L"\\\\.\\pipe\\");+if(xutftowcs(wpath+off,realpath.buf,alloc-off)<0)+return-1;++/* Handle drive prefix */+if(wpath[off]&&wpath[off+1]==L':'){+wpath[off+1]=L'_';+off+=2;+}++for(;wpath[off];off++)+if(wpath[off]==L'/')+wpath[off]=L'\\';++strbuf_release(&realpath);+return0;+}++staticenumipc_active_stateget_active_state(wchar_t*pipe_path)+{+if(WaitNamedPipeW(pipe_path,NMPWAIT_USE_DEFAULT_WAIT))+returnIPC_STATE__LISTENING;++if(GetLastError()==ERROR_SEM_TIMEOUT)+returnIPC_STATE__NOT_LISTENING;++if(GetLastError()==ERROR_FILE_NOT_FOUND)+returnIPC_STATE__PATH_NOT_FOUND;++returnIPC_STATE__OTHER_ERROR;+}++enumipc_active_stateipc_get_active_state(constchar*path)+{+wchar_tpipe_path[MAX_PATH];++if(initialize_pipe_name(path,pipe_path,ARRAY_SIZE(pipe_path))<0)+returnIPC_STATE__INVALID_PATH;++returnget_active_state(pipe_path);+}++#define WAIT_STEP_MS (50)++staticenumipc_active_stateconnect_to_server(+constwchar_t*wpath,+DWORDtimeout_ms,+conststructipc_client_connect_options*options,+int*pfd)+{+DWORDt_start_ms,t_waited_ms;+DWORDstep_ms;+HANDLEhPipe=INVALID_HANDLE_VALUE;+DWORDmode=PIPE_READMODE_BYTE;+DWORDgle;++*pfd=-1;++for(;;){+hPipe=CreateFileW(wpath,GENERIC_READ|GENERIC_WRITE,+0,NULL,OPEN_EXISTING,0,NULL);+if(hPipe!=INVALID_HANDLE_VALUE)+break;++gle=GetLastError();++switch(gle){+caseERROR_FILE_NOT_FOUND:+if(!options->wait_if_not_found)+returnIPC_STATE__PATH_NOT_FOUND;+if(!timeout_ms)+returnIPC_STATE__PATH_NOT_FOUND;++step_ms=(timeout_ms<WAIT_STEP_MS)?+timeout_ms:WAIT_STEP_MS;+sleep_millisec(step_ms);++timeout_ms-=step_ms;+break;/* try again */++caseERROR_PIPE_BUSY:+if(!options->wait_if_busy)+returnIPC_STATE__NOT_LISTENING;+if(!timeout_ms)+returnIPC_STATE__NOT_LISTENING;++t_start_ms=(DWORD)(getnanotime()/1000000);++if(!WaitNamedPipeW(wpath,timeout_ms)){+if(GetLastError()==ERROR_SEM_TIMEOUT)+returnIPC_STATE__NOT_LISTENING;++returnIPC_STATE__OTHER_ERROR;+}++/*+*Apipeserverinstancebecameavailable.+*Raceotherclientprocessestoconnectto+*it.+*+*Butfirstdecrementouroveralltimeoutso+*thatwedon'tstarveifwekeeplosingthe+*race.Butalsoguardagainstspecial+*NPMWAIT_values(0and-1).+*/+t_waited_ms=(DWORD)(getnanotime()/1000000)-t_start_ms;+if(t_waited_ms<timeout_ms)+timeout_ms-=t_waited_ms;+else+timeout_ms=1;+break;/* try again */++default:+returnIPC_STATE__OTHER_ERROR;+}+}++if(!SetNamedPipeHandleState(hPipe,&mode,NULL,NULL)){+CloseHandle(hPipe);+returnIPC_STATE__OTHER_ERROR;+}++*pfd=_open_osfhandle((intptr_t)hPipe,O_RDWR|O_BINARY);+if(*pfd<0){+CloseHandle(hPipe);+returnIPC_STATE__OTHER_ERROR;+}++/* fd now owns hPipe */++returnIPC_STATE__LISTENING;+}++/*+*ThedefaultconnectiontimeoutforWindowsclients.+*+*Thisisnotcurrentlypartoftheipc_API(northeconfigsettings)+*becauseofdifferencesbetweenWindowsandotherplatforms.+*+*Thisvaluewaschosenatrandom.+*/+#define WINDOWS_CONNECTION_TIMEOUT_MS (30000)++enumipc_active_stateipc_client_try_connect(+constchar*path,+conststructipc_client_connect_options*options,+int*pfd)+{+wchar_twpath[MAX_PATH];+enumipc_active_statestate=IPC_STATE__OTHER_ERROR;++*pfd=-1;++trace2_region_enter("ipc-client","try-connect",NULL);+trace2_data_string("ipc-client",NULL,"try-connect/path",path);++if(initialize_pipe_name(path,wpath,ARRAY_SIZE(wpath))<0)+state=IPC_STATE__INVALID_PATH;+else+state=connect_to_server(wpath,WINDOWS_CONNECTION_TIMEOUT_MS,+options,pfd);++trace2_data_intmax("ipc-client",NULL,"try-connect/state",+(intmax_t)state);+trace2_region_leave("ipc-client","try-connect",NULL);+returnstate;+}++intipc_client_send_command_to_fd(intfd,constchar*message,+structstrbuf*answer)+{+intret=0;++strbuf_setlen(answer,0);++trace2_region_enter("ipc-client","send-command",NULL);++if(write_packetized_from_buf(message,strlen(message),fd,1)<0){+ret=error(_("could not send IPC command"));+gotodone;+}++FlushFileBuffers((HANDLE)_get_osfhandle(fd));++if(read_packetized_to_strbuf(fd,answer,PACKET_READ_NEVER_DIE)<0){+ret=error(_("could not read IPC response"));+gotodone;+}++done:+trace2_region_leave("ipc-client","send-command",NULL);+returnret;+}++intipc_client_send_command(constchar*path,+conststructipc_client_connect_options*options,+constchar*message,structstrbuf*response)+{+intfd;+intret=-1;+enumipc_active_statestate;++state=ipc_client_try_connect(path,options,&fd);++if(state!=IPC_STATE__LISTENING)+returnret;++ret=ipc_client_send_command_to_fd(fd,message,response);+close(fd);+returnret;+}++/*+*Duplicatethegivenpipehandleandwrapitinafiledescriptorso+*thatwecanusepkt-lineonit.+*/+staticintdup_fd_from_pipe(constHANDLEpipe)+{+HANDLEprocess=GetCurrentProcess();+HANDLEhandle;+intfd;++if(!DuplicateHandle(process,pipe,process,&handle,0,FALSE,+DUPLICATE_SAME_ACCESS)){+errno=err_win_to_posix(GetLastError());+return-1;+}++fd=_open_osfhandle((intptr_t)handle,O_RDWR|O_BINARY);+if(fd<0){+errno=err_win_to_posix(GetLastError());+CloseHandle(handle);+return-1;+}++/*+*`handle`isnowownedby`fd`andwillbeautomaticallyclosed+*whenthedescriptorisclosed.+*/++returnfd;+}++/*+*Magicnumbersusedtoannotatecallbackinstancedata.+*Theseareusedtohelpguardagainstaccidentallypassingthe+*wronginstancedataacrossmultiplelevelsofcallbacks(which+*iseasytodoifthereare`void*`arguments).+*/+enummagic{+MAGIC_SERVER_REPLY_DATA,+MAGIC_SERVER_THREAD_DATA,+MAGIC_SERVER_DATA,+};++structipc_server_reply_data{+enummagicmagic;+intfd;+structipc_server_thread_data*server_thread_data;+};++structipc_server_thread_data{+enummagicmagic;+structipc_server_thread_data*next_thread;+structipc_server_data*server_data;+pthread_tpthread_id;+HANDLEhPipe;+};++/*+*OnWindows,theconceptual"ipc-server"isimplementedasapoolof+*nidential/peer"server-thread"threads.Thatis,thereisno+*hierarchyofthreads;andthereforenocontrollerthreadmanaging+*thepool.Eachthreadhasanindependenthandletothenamedpipe,+*receivesincomingconnections,processestheclient,andre-uses+*thepipeforthenextclientconnection.+*+*Therefore,the"ipc-server"onlyneedstomaintainalistofthe+*spawnedthreadsforeventual"join"purposes.+*+*Asingle"stop-event"isvisibletoalloftheserverthreadsto+*tellthemtoshutdown(whenidle).+*/+structipc_server_data{+enummagicmagic;+ipc_server_application_cb*application_cb;+void*application_data;+structstrbufbuf_path;+wchar_twpath[MAX_PATH];++HANDLEhEventStopRequested;+structipc_server_thread_data*thread_list;+intis_stopped;+};++enumconnect_result{+CR_CONNECTED=0,+CR_CONNECT_PENDING,+CR_CONNECT_ERROR,+CR_WAIT_ERROR,+CR_SHUTDOWN,+};++staticenumconnect_resultqueue_overlapped_connect(+structipc_server_thread_data*server_thread_data,+OVERLAPPED*lpo)+{+if(ConnectNamedPipe(server_thread_data->hPipe,lpo))+gotofailed;++switch(GetLastError()){+caseERROR_IO_PENDING:+returnCR_CONNECT_PENDING;++caseERROR_PIPE_CONNECTED:+SetEvent(lpo->hEvent);+returnCR_CONNECTED;++default:+break;+}++failed:+error(_("ConnectNamedPipe failed for '%s' (%lu)"),+server_thread_data->server_data->buf_path.buf,+GetLastError());+returnCR_CONNECT_ERROR;+}++/*+*UseWindowsOverlappedIOtowaitforaconnectionorforourevent+*tobesignalled.+*/+staticenumconnect_resultwait_for_connection(+structipc_server_thread_data*server_thread_data,+OVERLAPPED*lpo)+{+enumconnect_resultr;+HANDLEwaitHandles[2];+DWORDdwWaitResult;++r=queue_overlapped_connect(server_thread_data,lpo);+if(r!=CR_CONNECT_PENDING)+returnr;++waitHandles[0]=server_thread_data->server_data->hEventStopRequested;+waitHandles[1]=lpo->hEvent;++dwWaitResult=WaitForMultipleObjects(2,waitHandles,FALSE,INFINITE);+switch(dwWaitResult){+caseWAIT_OBJECT_0+0:+returnCR_SHUTDOWN;++caseWAIT_OBJECT_0+1:+ResetEvent(lpo->hEvent);+returnCR_CONNECTED;++default:+returnCR_WAIT_ERROR;+}+}++/*+*Forwarddeclareourreplycallbackfunctionsothatanycompiler+*errorsarereportedwhenweactuallydefinethefunction(inaddition+*toanyerrorsreportedwhenwetrytopassthiscallbackfunctionas+*aparameterinafunctioncall).Theformerareeasiertounderstand.+*/+staticipc_server_reply_cbdo_io_reply_callback;++/*+*Relayapplication'sresponsemessagetotheclientprocess.+*(Wedonotflushatthispointbecauseweallowthecaller+*tochunkdatatotheclientthruus.)+*/+staticintdo_io_reply_callback(structipc_server_reply_data*reply_data,+constchar*response,size_tresponse_len)+{+if(reply_data->magic!=MAGIC_SERVER_REPLY_DATA)+BUG("reply_cb called with wrong instance data");++returnwrite_packetized_from_buf(response,response_len,+reply_data->fd,0);+}++/*+*Receivetherequest/commandfromtheclientandpassittothe+*registeredrequest-callback.Therequest-callbackwillcompose+*aresponseandcallourreply-callbacktosendittotheclient.+*+*Simple-IPConlycontainsoneroundtrip,soweflushandclose+*hereaftertheresponse.+*/+staticintdo_io(structipc_server_thread_data*server_thread_data)+{+structstrbufbuf=STRBUF_INIT;+structipc_server_reply_datareply_data;+intret=0;++reply_data.magic=MAGIC_SERVER_REPLY_DATA;+reply_data.server_thread_data=server_thread_data;++reply_data.fd=dup_fd_from_pipe(server_thread_data->hPipe);+if(reply_data.fd<0)+returnerror(_("could not create fd from pipe for '%s'"),+server_thread_data->server_data->buf_path.buf);++ret=read_packetized_to_strbuf(reply_data.fd,&buf,+PACKET_READ_NEVER_DIE);+if(ret>=0){+ret=server_thread_data->server_data->application_cb(+server_thread_data->server_data->application_data,+buf.buf,do_io_reply_callback,&reply_data);++packet_flush_gently(reply_data.fd);++FlushFileBuffers((HANDLE)_get_osfhandle((reply_data.fd)));+}+else{+/*+*Theclientprobablydisconnected/shutdownbeforeit+*couldsendawell-formedmessage.Ignoreit.+*/+}++strbuf_release(&buf);+close(reply_data.fd);++returnret;+}++/*+*HandleIPCrequestandresponsewiththisconnectedclient.Andreset+*thepipetoprepareforthenextclient.+*/+staticintuse_connection(structipc_server_thread_data*server_thread_data)+{+intret;++ret=do_io(server_thread_data);++FlushFileBuffers(server_thread_data->hPipe);+DisconnectNamedPipe(server_thread_data->hPipe);++returnret;+}++/*+*ThreadprocforanIPCserverworkerthread.Ithandlesaseriesof+*connectionsfromclients.ItcleansandreusesthehPipebetweeneach+*client.+*/+staticvoid*server_thread_proc(void*_server_thread_data)+{+structipc_server_thread_data*server_thread_data=_server_thread_data;+HANDLEhEventConnected=INVALID_HANDLE_VALUE;+OVERLAPPEDoConnect;+enumconnect_resultcr;+intret;++assert(server_thread_data->hPipe!=INVALID_HANDLE_VALUE);++trace2_thread_start("ipc-server");+trace2_data_string("ipc-server",NULL,"pipe",+server_thread_data->server_data->buf_path.buf);++hEventConnected=CreateEventW(NULL,TRUE,FALSE,NULL);++memset(&oConnect,0,sizeof(oConnect));+oConnect.hEvent=hEventConnected;++for(;;){+cr=wait_for_connection(server_thread_data,&oConnect);++switch(cr){+caseCR_SHUTDOWN:+gotofinished;++caseCR_CONNECTED:+ret=use_connection(server_thread_data);+if(ret==SIMPLE_IPC_QUIT){+ipc_server_stop_async(+server_thread_data->server_data);+gotofinished;+}+if(ret>0){+/*+*Ignore(transient)IOerrorswiththis+*clientandresetforthenextclient.+*/+}+break;++caseCR_CONNECT_PENDING:+/* By construction, this should not happen. */+BUG("ipc-server[%s]: unexpeced CR_CONNECT_PENDING",+server_thread_data->server_data->buf_path.buf);++caseCR_CONNECT_ERROR:+caseCR_WAIT_ERROR:+/*+*Ignorethesetheoreticalerrors.+*/+DisconnectNamedPipe(server_thread_data->hPipe);+break;++default:+BUG("unandled case after wait_for_connection");+}+}++finished:+CloseHandle(server_thread_data->hPipe);+CloseHandle(hEventConnected);++trace2_thread_exit();+returnNULL;+}++staticHANDLEcreate_new_pipe(wchar_t*wpath,intis_first)+{+HANDLEhPipe;+DWORDdwOpenMode,dwPipeMode;+LPSECURITY_ATTRIBUTESlpsa=NULL;++dwOpenMode=PIPE_ACCESS_INBOUND|PIPE_ACCESS_OUTBOUND|+FILE_FLAG_OVERLAPPED;++dwPipeMode=PIPE_TYPE_MESSAGE|PIPE_READMODE_BYTE|PIPE_WAIT|+PIPE_REJECT_REMOTE_CLIENTS;++if(is_first){+dwOpenMode|=FILE_FLAG_FIRST_PIPE_INSTANCE;++/*+*OnWindows,thefirstserverpipeinstancegetsto+*settheACL/SecurityAttributesonthenamed+*pipe;subsequentinstancesinheritandcannot+*changethem.+*+*TODOShouldweallowtheapplicationlayerto+*specifysecurityattributes,suchas`LocalService`+*or`LocalSystem`,whenwecreatethenamedpipe?+*Thisquestionisprobablynotimportantwhenthe+*daemonisstartedbyaforegrounduserprocessand+*onlyneedstotalktothecurrentuser,butmaybe+*ifthedaemonisrunviatheControlPanelasa+*SystemService.+*/+}++hPipe=CreateNamedPipeW(wpath,dwOpenMode,dwPipeMode,+PIPE_UNLIMITED_INSTANCES,1024,1024,0,lpsa);++returnhPipe;+}++intipc_server_run_async(structipc_server_data**returned_server_data,+constchar*path,conststructipc_server_opts*opts,+ipc_server_application_cb*application_cb,+void*application_data)+{+structipc_server_data*server_data;+wchar_twpath[MAX_PATH];+HANDLEhPipeFirst=INVALID_HANDLE_VALUE;+intk;+intret=0;+intnr_threads=opts->nr_threads;++*returned_server_data=NULL;++ret=initialize_pipe_name(path,wpath,ARRAY_SIZE(wpath));+if(ret<0)+returnerror(+_("could not create normalized wchar_t path for '%s'"),+path);++hPipeFirst=create_new_pipe(wpath,1);+if(hPipeFirst==INVALID_HANDLE_VALUE)+returnerror(_("IPC server already running on '%s'"),path);++server_data=xcalloc(1,sizeof(*server_data));+server_data->magic=MAGIC_SERVER_DATA;+server_data->application_cb=application_cb;+server_data->application_data=application_data;+server_data->hEventStopRequested=CreateEvent(NULL,TRUE,FALSE,NULL);+strbuf_init(&server_data->buf_path,0);+strbuf_addstr(&server_data->buf_path,path);+wcscpy(server_data->wpath,wpath);++if(nr_threads<1)+nr_threads=1;++for(k=0;k<nr_threads;k++){+structipc_server_thread_data*std;++std=xcalloc(1,sizeof(*std));+std->magic=MAGIC_SERVER_THREAD_DATA;+std->server_data=server_data;+std->hPipe=INVALID_HANDLE_VALUE;++std->hPipe=(k==0)+?hPipeFirst+:create_new_pipe(server_data->wpath,0);++if(std->hPipe==INVALID_HANDLE_VALUE){+/*+*Ifwe'vereachedapipeinstancelimitfor+*thispath,justusefewerthreads.+*/+free(std);+break;+}++if(pthread_create(&std->pthread_id,NULL,+server_thread_proc,std)){+/*+*Likewise,ifwe'reoutofthreads,justuse+*fewerthreadsthanrequested.+*+*However,wejustgiveupifwecan'tevenget+*onethread.Thisshouldnothappen.+*/+if(k==0)+die(_("could not start thread[0] for '%s'"),+path);++CloseHandle(std->hPipe);+free(std);+break;+}++std->next_thread=server_data->thread_list;+server_data->thread_list=std;+}++*returned_server_data=server_data;+return0;+}++intipc_server_stop_async(structipc_server_data*server_data)+{+if(!server_data)+return0;++/*+*Gentlytellalloftheipc_serverthreadstoshutdown.+*Thiswillbeseenthenexttimetheyareidle(andwaiting+*foraconnection).+*+*WeDONOTattempttoforcethemtodropanactiveconnection.+*/+SetEvent(server_data->hEventStopRequested);+return0;+}++intipc_server_await(structipc_server_data*server_data)+{+DWORDdwWaitResult;++if(!server_data)+return0;++dwWaitResult=WaitForSingleObject(server_data->hEventStopRequested,INFINITE);+if(dwWaitResult!=WAIT_OBJECT_0)+returnerror(_("wait for hEvent failed for '%s'"),+server_data->buf_path.buf);++while(server_data->thread_list){+structipc_server_thread_data*std=server_data->thread_list;++pthread_join(std->pthread_id,NULL);++server_data->thread_list=std->next_thread;+free(std);+}++server_data->is_stopped=1;++return0;+}++voidipc_server_free(structipc_server_data*server_data)+{+if(!server_data)+return;++if(!server_data->is_stopped)+BUG("cannot free ipc-server while running for '%s'",+server_data->buf_path.buf);++strbuf_release(&server_data->buf_path);++if(server_data->hEventStopRequested!=INVALID_HANDLE_VALUE)+CloseHandle(server_data->hEventStopRequested);++while(server_data->thread_list){+structipc_server_thread_data*std=server_data->thread_list;++server_data->thread_list=std->next_thread;+free(std);+}++free(server_data);+}
On Tue, Jan 12 2021, Jeff Hostetler via GitGitGadget wrote:
This series introduces a multi-threaded IPC mechanism called "Simple IPC".
This is a library-layer feature to make it easy to create very long running
daemon/service applications and for unrelated Git commands to communicate
with them. Communication uses pkt-line messaging over a Windows named pipe
or Unix domain socket.
On the server side, Simple IPC implements a (platform-specific) connection
listener and worker thread-pool to accept and handle a series of client
connections. The server functionality is completely hidden behind the
ipc_server_run() and ipc_server_run_async() APIs. The daemon/service
application only needs to define an application-specific callback to handle
client requests.
Note that Simple IPC is completely unrelated to the long running process
feature (described in sub-process.h) where the lifetime of a "sub-process"
child is bound to that of the invoking parent process and communication
occurs over the child's stdin/stdout.
Simple IPC will serve as a basis for a future builtin FSMonitor daemon
feature.
From: Jeff Hostetler <hidden> Date: 2021-01-12 18:36:35
On 1/12/21 11:50 AM, Ævar Arnfjörð Bjarmason wrote:
On Tue, Jan 12 2021, Jeff Hostetler via GitGitGadget wrote:
quoted
This series introduces a multi-threaded IPC mechanism called "Simple IPC".
This is a library-layer feature to make it easy to create very long running
daemon/service applications and for unrelated Git commands to communicate
with them. Communication uses pkt-line messaging over a Windows named pipe
or Unix domain socket.
On the server side, Simple IPC implements a (platform-specific) connection
listener and worker thread-pool to accept and handle a series of client
connections. The server functionality is completely hidden behind the
ipc_server_run() and ipc_server_run_async() APIs. The daemon/service
application only needs to define an application-specific callback to handle
client requests.
Note that Simple IPC is completely unrelated to the long running process
feature (described in sub-process.h) where the lifetime of a "sub-process"
child is bound to that of the invoking parent process and communication
occurs over the child's stdin/stdout.
Simple IPC will serve as a basis for a future builtin FSMonitor daemon
feature.
I'm starting with the model used by the existing FSMonitor feature
that Ben Peart and Kevin Willford added to Git.
Item [3] looks to be an earlier draft of that effort. The idea there
was to add the fsmonitor hook that could talk to a daemon like Watchman
and quickly update the in-memory cache-entry flags without the need to
lstat() and similarly update the untracked-cache. An index extension
was added to remember the last fsmonitor response processed.
Currently in Git, we have a fsmonitor hook (usually a perl script) that
talks to Watchman and translates the Watchman response back into
something that the Git client can understand. This comes back as a
list of files that have changed since some timestamp (or in V2, relative
to some daemon-specific token).
Items [1,2] are not related to that. That was a different effort to
quickly fetch a read-only copy of an already-parsed index via shared
memory. In the last version I saw, there were 2 daemons. index-helper
kept a fresh view of the index in shared memory and could give it to
the Git client. The client could just mmap the pre-parsed index and
avoid calling `read_index()`. Index-helper would drive Watchman to
keep track of cache-entries as they changed and handle the lstat's.
I'm not familiar with [4] (and I only quickly scanned it). There are
several ideas for finding slow spots while reading the index. I don't
want to go into all of them, but several are obsolete now. They didn't
contribute to the current effort.
The Simple IPC series (and a soon to be submitted fsmonitor--daemon
series) are intended to be a follow on to FSMonitor effort that is
currently in Git.
1. Build a git-native daemon to watch the file system and avoid needing
a third-party tool. This doesn't preclude the use of Watchman, but
having a builtin tool might simplify engineering support costs when
deploying to a large team.
2. Use direct IPC between the Git command and the daemon to avoid the
expense of the Hook API (which is expensive on Windows).
3. Make the daemon Git-aware. For example, it might want to pre-filter
ignored files. (This might not be present in V1. And we might extend
the daemon to do more of this as we improve performance.)
Jeff
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:47:39
Here is version 2 of my "Simple IPC" series and addresses the following
review comments:
[1] Redo packet_write_gently() to take a scratch buffer argument and fixup
callers to avoid potential thread-stack problems caused by a very large
stack buffer when used multi-threaded callers. This turned out to be a
little more involved than anticipated because the pkt-line code doesn't know
about its thread state nor an opportunity to initialize thread-state.
[2] Deleted the unix_stream_socket() helper function and inline it in the
few callers and then let those call sites decide whether to call die() or
not.
[3] Refactor unix_stream_listen() to take an "options" structure and to
incorporate the changes I described in my earlier
unix_stream_listen_gently().
[4] Update unix_stream_connect() to return errors rather than calling die().
[5] Update the simple-ipc server startup to detect dead and/or in-use Unix
domain sockets and/or create a new socket in a race-friendly way. I now use
a variation of the atomic lock-rename-trick when creating the socket
(details are in a large comment in the code).
Jeff Hostetler (10):
pkt-line: promote static buffer in packet_write_gently() to callers
pkt-line: add write_packetized_from_buf2() that takes scratch buffer
simple-ipc: design documentation for new IPC mechanism
simple-ipc: add win32 implementation
simple-ipc: add t/helper/test-simple-ipc and t0052
unix-socket: elimiate static unix_stream_socket() helper function
unix-socket: add options to unix_stream_listen()
unix-socket: add no-chdir option to unix_stream_listen()
unix-socket: do not call die in unix_stream_connect()
simple-ipc: add Unix domain socket implementation
Johannes Schindelin (3):
pkt-line: optionally skip the flush packet in
write_packetized_from_buf()
pkt-line: (optionally) libify the packet readers
pkt-line: accept additional options in read_packetized_to_strbuf()
Junio C Hamano (1):
ci/install-depends: attempt to fix "brew cask" stuff
Documentation/technical/api-simple-ipc.txt | 34 +
Makefile | 8 +
builtin/credential-cache--daemon.c | 3 +-
ci/install-dependencies.sh | 8 +-
compat/simple-ipc/ipc-shared.c | 28 +
compat/simple-ipc/ipc-unix-socket.c | 1127 ++++++++++++++++++++
compat/simple-ipc/ipc-win32.c | 751 +++++++++++++
config.mak.uname | 2 +
contrib/buildsystems/CMakeLists.txt | 6 +
convert.c | 4 +-
pkt-line.c | 70 +-
pkt-line.h | 26 +-
simple-ipc.h | 230 ++++
t/helper/test-simple-ipc.c | 485 +++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
t/t0052-simple-ipc.sh | 129 +++
unix-socket.c | 67 +-
unix-socket.h | 16 +-
19 files changed, 2949 insertions(+), 47 deletions(-)
create mode 100644 Documentation/technical/api-simple-ipc.txt
create mode 100644 compat/simple-ipc/ipc-shared.c
create mode 100644 compat/simple-ipc/ipc-unix-socket.c
create mode 100644 compat/simple-ipc/ipc-win32.c
create mode 100644 simple-ipc.h
create mode 100644 t/helper/test-simple-ipc.c
create mode 100755 t/t0052-simple-ipc.sh
base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-766%2Fjeffhostetler%2Fsimple-ipc-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-766/jeffhostetler/simple-ipc-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/766
Range-diff vs v1:
1: 1155a45cf64 < -: ----------- pkt-line: use stack rather than static buffer in packet_write_gently()
-: ----------- > 1: 4c6766d4183 ci/install-depends: attempt to fix "brew cask" stuff
-: ----------- > 2: 3b03a8ff7a7 pkt-line: promote static buffer in packet_write_gently() to callers
-: ----------- > 3: e671894b4c0 pkt-line: add write_packetized_from_buf2() that takes scratch buffer
3: edf5ac95d66 ! 4: 0832f7d324d pkt-line: optionally skip the flush packet in write_packetized_from_buf()
@@ Commit message
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
+ Signed-off-by: Jeff Hostetler [off-list ref]
Signed-off-by: Johannes Schindelin [off-list ref]
## convert.c ##
@@ pkt-line.c: int write_packetized_from_fd(int fd_in, int fd_out)
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+ int flush_at_end)
+ {
+ static struct packet_scratch_space scratch;
+
+- return write_packetized_from_buf2(src_in, len, fd_out, &scratch);
++ return write_packetized_from_buf2(src_in, len, fd_out,
++ flush_at_end, &scratch);
+ }
+
+ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
++ int flush_at_end,
+ struct packet_scratch_space *scratch)
{
int err = 0;
- size_t bytes_written = 0;
-@@ pkt-line.c: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)
- err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write);
+@@ pkt-line.c: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
+ err = packet_write_gently(fd_out, src_in + bytes_written, bytes_to_write, scratch);
bytes_written += bytes_to_write;
}
- if (!err)
@@ pkt-line.h: void packet_buf_write_len(struct strbuf *buf, const char *data, size
-int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
+int write_packetized_from_buf(const char *src_in, size_t len, int fd_out,
+ int flush_at_end);
+ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
++ int flush_at_end,
+ struct packet_scratch_space *scratch);
/*
- * Read a packetized line into the buffer, which must be at least size bytes
2: b7d678bc918 ! 5: 43bc4a26b79 pkt-line: (optionally) libify the packet readers
@@ pkt-line.c: enum packet_read_status packet_read_with_status(int fd, char **src_b
*pktlen = -1;
## pkt-line.h ##
-@@ pkt-line.h: int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);
+@@ pkt-line.h: int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,
*
* If options contains PACKET_READ_DIE_ON_ERR_PACKET, it dies when it sees an
* ERR packet.
4: 2f399ac107c = 6: 6a389a35335 pkt-line: accept additional options in read_packetized_to_strbuf()
5: 7064c5e9ffa ! 7: a7275b4bdc2 simple-ipc: design documentation for new IPC mechanism
@@ Documentation/technical/api-simple-ipc.txt (new)
+
+This IPC mechanism differs from the existing `sub-process.c` model
+(Documentation/technical/long-running-process-protocol.txt) and used
-+by applications like Git-LFS because the server is assumed to be very
-+long running system service. In contrast, a "sub-process model process"
-+is started with the foreground process and exits when the foreground
-+process terminates. How the server is started is also outside the
-+scope of the IPC mechanism.
++by applications like Git-LFS. In the simple-ipc model the server is
++assumed to be a very long-running system service. In contrast, in the
++LFS-style sub-process model the helper is started with the foreground
++process and exits when the foreground process terminates.
++
++How the simple-ipc server is started is also outside the scope of the
++IPC mechanism. For example, the server might be started during
++maintenance operations.
+
+The IPC protocol consists of a single request message from the client and
+an optional request message from the server. For simplicity, pkt-line
6: 9e27c07d785 ! 8: 388366913d4 simple-ipc: add win32 implementation
@@ compat/simple-ipc/ipc-win32.c (new)
+enum ipc_active_state ipc_client_try_connect(
+ const char *path,
+ const struct ipc_client_connect_options *options,
-+ int *pfd)
++ struct ipc_client_connection **p_connection)
+{
+ wchar_t wpath[MAX_PATH];
+ enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
++ int fd = -1;
+
-+ *pfd = -1;
++ *p_connection = NULL;
+
+ trace2_region_enter("ipc-client", "try-connect", NULL);
+ trace2_data_string("ipc-client", NULL, "try-connect/path", path);
@@ compat/simple-ipc/ipc-win32.c (new)
+ state = IPC_STATE__INVALID_PATH;
+ else
+ state = connect_to_server(wpath, WINDOWS_CONNECTION_TIMEOUT_MS,
-+ options, pfd);
++ options, &fd);
+
+ trace2_data_intmax("ipc-client", NULL, "try-connect/state",
+ (intmax_t)state);
+ trace2_region_leave("ipc-client", "try-connect", NULL);
++
++ if (state == IPC_STATE__LISTENING) {
++ (*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
++ (*p_connection)->fd = fd;
++ }
++
+ return state;
+}
+
-+int ipc_client_send_command_to_fd(int fd, const char *message,
-+ struct strbuf *answer)
++void ipc_client_close_connection(struct ipc_client_connection *connection)
++{
++ if (!connection)
++ return;
++
++ if (connection->fd != -1)
++ close(connection->fd);
++
++ free(connection);
++}
++
++int ipc_client_send_command_to_connection(
++ struct ipc_client_connection *connection,
++ const char *message, struct strbuf *answer)
+{
+ int ret = 0;
+
@@ compat/simple-ipc/ipc-win32.c (new)
+
+ trace2_region_enter("ipc-client", "send-command", NULL);
+
-+ if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) {
++ if (write_packetized_from_buf2(message, strlen(message),
++ connection->fd, 1,
++ &connection->scratch_write_buffer) < 0) {
+ ret = error(_("could not send IPC command"));
+ goto done;
+ }
+
-+ FlushFileBuffers((HANDLE)_get_osfhandle(fd));
++ FlushFileBuffers((HANDLE)_get_osfhandle(connection->fd));
+
-+ if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) {
++ if (read_packetized_to_strbuf(connection->fd, answer,
++ PACKET_READ_NEVER_DIE) < 0) {
+ ret = error(_("could not read IPC response"));
+ goto done;
+ }
@@ compat/simple-ipc/ipc-win32.c (new)
+ const struct ipc_client_connect_options *options,
+ const char *message, struct strbuf *response)
+{
-+ int fd;
+ int ret = -1;
+ enum ipc_active_state state;
++ struct ipc_client_connection *connection = NULL;
+
-+ state = ipc_client_try_connect(path, options, &fd);
++ state = ipc_client_try_connect(path, options, &connection);
+
+ if (state != IPC_STATE__LISTENING)
+ return ret;
+
-+ ret = ipc_client_send_command_to_fd(fd, message, response);
-+ close(fd);
++ ret = ipc_client_send_command_to_connection(connection, message, response);
++
++ ipc_client_close_connection(connection);
++
+ return ret;
+}
+
@@ compat/simple-ipc/ipc-win32.c (new)
+ struct ipc_server_data *server_data;
+ pthread_t pthread_id;
+ HANDLE hPipe;
++ struct packet_scratch_space scratch_write_buffer;
+};
+
+/*
@@ compat/simple-ipc/ipc-win32.c (new)
+static int do_io_reply_callback(struct ipc_server_reply_data *reply_data,
+ const char *response, size_t response_len)
+{
++ struct packet_scratch_space *scratch =
++ &reply_data->server_thread_data->scratch_write_buffer;
++
+ if (reply_data->magic != MAGIC_SERVER_REPLY_DATA)
+ BUG("reply_cb called with wrong instance data");
+
-+ return write_packetized_from_buf(response, response_len,
-+ reply_data->fd, 0);
++ return write_packetized_from_buf2(response, response_len,
++ reply_data->fd, 0, scratch);
+}
+
+/*
@@ simple-ipc.h (new)
+#endif
+
+#ifdef SUPPORTS_SIMPLE_IPC
++#include "pkt-line.h"
+
+/*
+ * Simple IPC Client Side API.
@@ simple-ipc.h (new)
+ */
+enum ipc_active_state ipc_get_active_state(const char *path);
+
++struct ipc_client_connection {
++ int fd;
++ struct packet_scratch_space scratch_write_buffer;
++};
++
+/*
+ * Try to connect to the daemon on the named pipe or socket.
+ *
-+ * Returns IPC_STATE__LISTENING (and an fd) when connected.
++ * Returns IPC_STATE__LISTENING and a connection handle.
+ *
+ * Otherwise, returns info to help decide whether to retry or to
+ * spawn/respawn the server.
@@ simple-ipc.h (new)
+enum ipc_active_state ipc_client_try_connect(
+ const char *path,
+ const struct ipc_client_connect_options *options,
-+ int *pfd);
++ struct ipc_client_connection **p_connection);
++
++void ipc_client_close_connection(struct ipc_client_connection *connection);
+
+/*
+ * Used by the client to synchronously send and receive a message with
-+ * the server on the provided fd.
++ * the server on the provided client connection.
+ *
+ * Returns 0 when successful.
+ *
+ * Calls error() and returns non-zero otherwise.
+ */
-+int ipc_client_send_command_to_fd(int fd, const char *message,
-+ struct strbuf *answer);
++int ipc_client_send_command_to_connection(
++ struct ipc_client_connection *connection,
++ const char *message, struct strbuf *answer);
+
+/*
+ * Used by the client to synchronously connect and send and receive a
9: 69969c2b8d3 = 9: f0bebf1cdb3 simple-ipc: add t/helper/test-simple-ipc and t0052
-: ----------- > 10: f5d5445cf42 unix-socket: elimiate static unix_stream_socket() helper function
7: 96268351ac6 ! 11: 7a6a69dfc20 unix-socket: create gentle version of unix_stream_listen()
@@ Metadata
Author: Jeff Hostetler [off-list ref]
## Commit message ##
- unix-socket: create gentle version of unix_stream_listen()
+ unix-socket: add options to unix_stream_listen()
- Create a gentle version of `unix_stream_listen()`. This version does
- not call `die()` if a socket-fd cannot be created and does not assume
- that it is safe to `unlink()` an existing socket-inode.
+ Update `unix_stream_listen()` to take an options structure to override
+ default behaviors. This includes the size of the `listen()` backlog
+ and whether it should always unlink the socket file before trying to
+ create a new one. Also eliminate calls to `die()` if it cannot create
+ a socket.
- `unix_stream_listen()` uses `unix_stream_socket()` helper function to
- create the socket-fd. Avoid that helper because it calls `die()` on
- errors.
-
- `unix_stream_listen()` always tries to `unlink()` the socket-path before
- calling `bind()`. If there is an existing server/daemon already bound
- and listening on that socket-path, our `unlink()` would have the effect
- of disassociating the existing server's bound-socket-fd from the socket-path
- without notifying the existing server. The existing server could continue
- to service existing connections (accepted-socket-fd's), but would not
- receive any futher new connections (since clients rendezvous via the
- socket-path). The existing server would effectively be offline but yet
- appear to be active.
+ Normally, `unix_stream_listen()` always tries to `unlink()` the
+ socket-path before calling `bind()`. If there is an existing
+ server/daemon already bound and listening on that socket-path, our
+ `unlink()` would have the effect of disassociating the existing
+ server's bound-socket-fd from the socket-path without notifying the
+ existing server. The existing server could continue to service
+ existing connections (accepted-socket-fd's), but would not receive any
+ futher new connections (since clients rendezvous via the socket-path).
+ The existing server would effectively be offline but yet appear to be
+ active.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
@@ Commit message
Signed-off-by: Jeff Hostetler [off-list ref]
+ ## builtin/credential-cache--daemon.c ##
+@@ builtin/credential-cache--daemon.c: static int serve_cache_loop(int fd)
+
+ static void serve_cache(const char *socket_path, int debug)
+ {
++ struct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+ int fd;
+
+- fd = unix_stream_listen(socket_path);
++ fd = unix_stream_listen(socket_path, &opts);
+ if (fd < 0)
+ die_errno("unable to bind to '%s'", socket_path);
+
+
## unix-socket.c ##
-@@ unix-socket.c: int unix_stream_listen(const char *path)
- errno = saved_errno;
+@@ unix-socket.c: int unix_stream_connect(const char *path)
return -1;
}
-+
-+int unix_stream_listen_gently(const char *path,
-+ const struct unix_stream_listen_opts *opts)
-+{
+
+-int unix_stream_listen(const char *path)
++int unix_stream_listen(const char *path,
++ const struct unix_stream_listen_opts *opts)
+ {
+- int fd, saved_errno;
+ int fd = -1;
-+ int bind_successful = 0;
+ int saved_errno;
-+ struct sockaddr_un sa;
-+ struct unix_sockaddr_context ctx;
-+
-+ if (unix_sockaddr_init(&sa, path, &ctx) < 0)
-+ goto fail;
++ int bind_successful = 0;
++ int backlog;
+ struct sockaddr_un sa;
+ struct unix_sockaddr_context ctx;
+
+- unlink(path);
+-
+ if (unix_sockaddr_init(&sa, path, &ctx) < 0)
+ return -1;
+
-+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
-+ if (fd < 0)
+ fd = socket(AF_UNIX, SOCK_STREAM, 0);
+ if (fd < 0)
+- die_errno("unable to create socket");
+ goto fail;
+
+ if (opts->force_unlink_before_bind)
+ unlink(path);
-+
-+ if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
-+ goto fail;
+
+ if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
+ goto fail;
+ bind_successful = 1;
-+
-+ if (listen(fd, opts->listen_backlog_size) < 0)
-+ goto fail;
-+
-+ unix_sockaddr_cleanup(&ctx);
-+ return fd;
-+
-+fail:
-+ saved_errno = errno;
-+ unix_sockaddr_cleanup(&ctx);
-+ close(fd);
+
+- if (listen(fd, 5) < 0)
++ if (opts->listen_backlog_size > 0)
++ backlog = opts->listen_backlog_size;
++ else
++ backlog = 5;
++ if (listen(fd, backlog) < 0)
+ goto fail;
+
+ unix_sockaddr_cleanup(&ctx);
+@@ unix-socket.c: int unix_stream_listen(const char *path)
+ fail:
+ saved_errno = errno;
+ unix_sockaddr_cleanup(&ctx);
+- close(fd);
++ if (fd != -1)
++ close(fd);
+ if (bind_successful)
+ unlink(path);
-+ errno = saved_errno;
-+ return -1;
-+}
+ errno = saved_errno;
+ return -1;
+ }
## unix-socket.h ##
@@
- int unix_stream_connect(const char *path);
- int unix_stream_listen(const char *path);
+ #ifndef UNIX_SOCKET_H
+ #define UNIX_SOCKET_H
+struct unix_stream_listen_opts {
+ int listen_backlog_size;
+ unsigned int force_unlink_before_bind:1;
+};
+
-+int unix_stream_listen_gently(const char *path,
-+ const struct unix_stream_listen_opts *opts);
++#define UNIX_STREAM_LISTEN_OPTS_INIT \
++{ \
++ .listen_backlog_size = 5, \
++ .force_unlink_before_bind = 1, \
++}
+
+ int unix_stream_connect(const char *path);
+-int unix_stream_listen(const char *path);
++int unix_stream_listen(const char *path,
++ const struct unix_stream_listen_opts *opts);
+
#endif /* UNIX_SOCKET_H */
8: 383a9755669 ! 12: 745b6d5fb74 unix-socket: add no-chdir option to unix_stream_listen_gently()
@@ Metadata
Author: Jeff Hostetler [off-list ref]
## Commit message ##
- unix-socket: add no-chdir option to unix_stream_listen_gently()
+ unix-socket: add no-chdir option to unix_stream_listen()
Calls to `chdir()` are dangerous in a multi-threaded context. If
`unix_stream_listen()` is given a socket pathname that is too big to
@@ Commit message
Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag
is set.
- Extend the public interface to `unix_stream_listen_gently()` to also
- expose this new flag.
-
Signed-off-by: Jeff Hostetler [off-list ref]
## unix-socket.c ##
@@ unix-socket.c: int unix_stream_connect(const char *path)
if (unix_sockaddr_init(&sa, path, &ctx) < 0)
return -1;
-@@ unix-socket.c: int unix_stream_listen(const char *path)
- {
- int fd, saved_errno;
- struct sockaddr_un sa;
-- struct unix_sockaddr_context ctx;
-+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
-
- unlink(path);
-
-@@ unix-socket.c: int unix_stream_listen_gently(const char *path,
+@@ unix-socket.c: int unix_stream_listen(const char *path,
int bind_successful = 0;
- int saved_errno;
+ int backlog;
struct sockaddr_un sa;
- struct unix_sockaddr_context ctx;
+ struct unix_sockaddr_context ctx = UNIX_SOCKADDR_CONTEXT_INIT;
@@ unix-socket.c: int unix_stream_listen_gently(const char *path,
+ ctx.disallow_chdir = opts->disallow_chdir;
if (unix_sockaddr_init(&sa, path, &ctx) < 0)
- goto fail;
+ return -1;
## unix-socket.h ##
-@@ unix-socket.h: int unix_stream_listen(const char *path);
+@@
struct unix_stream_listen_opts {
int listen_backlog_size;
unsigned int force_unlink_before_bind:1;
+ unsigned int disallow_chdir:1;
};
- int unix_stream_listen_gently(const char *path,
+ #define UNIX_STREAM_LISTEN_OPTS_INIT \
+ { \
+ .listen_backlog_size = 5, \
+ .force_unlink_before_bind = 1, \
++ .disallow_chdir = 0, \
+ }
+
+ int unix_stream_connect(const char *path);
-: ----------- > 13: 2cca15a10ec unix-socket: do not call die in unix_stream_connect()
10: a1b15fb5cb0 ! 14: 72c1c209c38 simple-ipc: add Unix domain socket implementation
@@ Commit message
Create Unix domain socket based implementation of "simple-ipc".
+ A set of `ipc_client` routines implement a client library to connect
+ to an `ipc_server` over a Unix domain socket, send a simple request,
+ and receive a single response. Clients use blocking IO on the socket.
+
+ A set of `ipc_server` routines implement a thread pool to listen for
+ and concurrently service client connections.
+
+ The server creates a new Unix domain socket at a known location. If a
+ socket already exists with that name, the server tries to determine if
+ another server is already listening on the socket or if the socket is
+ dead. If socket is busy, the server exits with an error rather than
+ stealing the socket. If the socket is dead, the server creates a new
+ one and starts up.
+
+ If while running, the server detects that its socket has been stolen
+ by another server, it automatically exits.
+
Signed-off-by: Jeff Hostetler [off-list ref]
## Makefile ##
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ struct ipc_client_connect_options options
+ = IPC_CLIENT_CONNECT_OPTIONS_INIT;
+ struct stat st;
-+ int fd_test = -1;
++ struct ipc_client_connection *connection_test = NULL;
+
+ options.wait_if_busy = 0;
+ options.wait_if_not_found = 0;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * at `path`, doesn't mean it that there is a server listening.
+ * Ping it to be sure.
+ */
-+ state = ipc_client_try_connect(path, &options, &fd_test);
-+ close(fd_test);
++ state = ipc_client_try_connect(path, &options, &connection_test);
++ ipc_client_close_connection(connection_test);
+
+ return state;
+}
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+enum ipc_active_state ipc_client_try_connect(
+ const char *path,
+ const struct ipc_client_connect_options *options,
-+ int *pfd)
++ struct ipc_client_connection **p_connection)
+{
+ enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
++ int fd = -1;
+
-+ *pfd = -1;
++ *p_connection = NULL;
+
+ trace2_region_enter("ipc-client", "try-connect", NULL);
+ trace2_data_string("ipc-client", NULL, "try-connect/path", path);
+
+ state = connect_to_server(path, MY_CONNECTION_TIMEOUT_MS,
-+ options, pfd);
++ options, &fd);
+
+ trace2_data_intmax("ipc-client", NULL, "try-connect/state",
+ (intmax_t)state);
+ trace2_region_leave("ipc-client", "try-connect", NULL);
++
++ if (state == IPC_STATE__LISTENING) {
++ (*p_connection) = xcalloc(1, sizeof(struct ipc_client_connection));
++ (*p_connection)->fd = fd;
++ }
++
+ return state;
+}
+
-+int ipc_client_send_command_to_fd(int fd, const char *message,
-+ struct strbuf *answer)
++void ipc_client_close_connection(struct ipc_client_connection *connection)
++{
++ if (!connection)
++ return;
++
++ if (connection->fd != -1)
++ close(connection->fd);
++
++ free(connection);
++}
++
++int ipc_client_send_command_to_connection(
++ struct ipc_client_connection *connection,
++ const char *message, struct strbuf *answer)
+{
+ int ret = 0;
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+
+ trace2_region_enter("ipc-client", "send-command", NULL);
+
-+ if (write_packetized_from_buf(message, strlen(message), fd, 1) < 0) {
++ if (write_packetized_from_buf2(message, strlen(message),
++ connection->fd, 1,
++ &connection->scratch_write_buffer) < 0) {
+ ret = error(_("could not send IPC command"));
+ goto done;
+ }
+
-+ if (read_packetized_to_strbuf(fd, answer, PACKET_READ_NEVER_DIE) < 0) {
++ if (read_packetized_to_strbuf(connection->fd, answer,
++ PACKET_READ_NEVER_DIE) < 0) {
+ ret = error(_("could not read IPC response"));
+ goto done;
+ }
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ const struct ipc_client_connect_options *options,
+ const char *message, struct strbuf *answer)
+{
-+ int fd;
+ int ret = -1;
+ enum ipc_active_state state;
++ struct ipc_client_connection *connection = NULL;
+
-+ state = ipc_client_try_connect(path, options, &fd);
++ state = ipc_client_try_connect(path, options, &connection);
+
+ if (state != IPC_STATE__LISTENING)
+ return ret;
+
-+ ret = ipc_client_send_command_to_fd(fd, message, answer);
-+ close(fd);
++ ret = ipc_client_send_command_to_connection(connection, message, answer);
++
++ ipc_client_close_connection(connection);
++
+ return ret;
+}
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ struct ipc_worker_thread_data *next_thread;
+ struct ipc_server_data *server_data;
+ pthread_t pthread_id;
++ struct packet_scratch_space scratch_write_buffer;
+};
+
+struct ipc_accept_thread_data {
+ enum magic magic;
+ struct ipc_server_data *server_data;
++
+ int fd_listen;
-+ ino_t inode_listen;
++ struct stat st_listen;
++
+ int fd_send_shutdown;
+ int fd_wait_shutdown;
+ pthread_t pthread_id;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+static int do_io_reply_callback(struct ipc_server_reply_data *reply_data,
+ const char *response, size_t response_len)
+{
++ struct packet_scratch_space *scratch =
++ &reply_data->worker_thread_data->scratch_write_buffer;
++
+ if (reply_data->magic != MAGIC_SERVER_REPLY_DATA)
+ BUG("reply_cb called with wrong instance data");
+
-+ return write_packetized_from_buf(response, response_len,
-+ reply_data->fd, 0);
++ return write_packetized_from_buf2(response, response_len,
++ reply_data->fd, 0, scratch);
+}
+
+/* A randomly chosen value. */
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+static int socket_was_stolen(struct ipc_accept_thread_data *accept_thread_data)
+{
+ struct stat st;
++ struct stat *ref_st = &accept_thread_data->st_listen;
+
+ if (lstat(accept_thread_data->server_data->buf_path.buf, &st) == -1)
+ return 1;
+
-+ if (st.st_ino != accept_thread_data->inode_listen)
++ if (st.st_ino != ref_st->st_ino)
+ return 1;
+
++ /* We might also consider the creation time on some platforms. */
++
+ return 0;
+}
+
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * open/connect using the "socket-inode" pathname.
+ *
+ * Unix domain sockets have a fundamental design flaw because the
-+ * "socket-inode" persists until the pathname is deleted; closing the listening
-+ * "socket-fd" only closes the socket handle/descriptor, it does not delete
-+ * the inode/pathname.
++ * "socket-inode" persists until the pathname is deleted; closing the
++ * listening "socket-fd" only closes the socket handle/descriptor, it
++ * does not delete the inode/pathname.
+ *
+ * Well-behaving service daemons are expected to also delete the inode
+ * before shutdown. If a service crashes (or forgets) it can leave
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ * inode *or* another service instance is already running.
+ *
+ * One possible solution is to blindly unlink the inode before
-+ * attempting to bind a new socket-fd (and thus create) a new
++ * attempting to bind a new socket-fd and thus create a new
+ * socket-inode. Then `bind(2)` should always succeed. However, if
-+ * there is an existing service instance, it would be orphaned --
-+ * it would still be listening on a socket-fd that is still bound
-+ * to an (unlinked) socket-inode, but that socket-inode is no longer
++ * there is an existing service instance, it would be orphaned -- it
++ * would still be listening on a socket-fd that is still bound to an
++ * (unlinked) socket-inode, but that socket-inode is no longer
+ * associated with the pathname. New client connections will arrive
-+ * at our new socket-inode and not the existing server's. (It is upto
-+ * the existing server to detect that its socket-inode has been
-+ * stolen and shutdown.)
++ * at OUR new socket-inode -- rather than the existing server's
++ * socket. (I suppose it is up to the existing server to detect that
++ * its socket-inode has been stolen and shutdown.)
++ *
++ * Another possible solution is to try to use the ".lock" trick, but
++ * bind() does not have a exclusive-create use bit like open() does,
++ * so we cannot have multiple servers fighting/racing to create the
++ * same file name without having losers lose without knowing that they
++ * lost.
++ *
++ * We try to avoid such stealing and would rather fail to run than
++ * steal an existing socket-inode (because we assume that the
++ * existing server has more context and value to the clients than a
++ * freshly started server). However, if multiple servers are racing
++ * to start, we don't care which one wins -- none of them have any
++ * state information yet worth fighting for.
++ *
++ * Create a "unique" socket-inode (with our PID in it (and assume that
++ * we can force-delete an existing socket with that name)). Stat it
++ * to get the inode number and ctime -- so that we can identify it as
++ * the one we created. Then use the atomic-rename trick to install it
++ * in the real location. (This will unlink an existing socket with
++ * that pathname -- and thereby steal the real socket-inode from an
++ * existing server.)
+ *
-+ * Since this is rather obscure and infrequent, we try to "gently"
-+ * create the socket-inode without disturbing an existing service.
++ * Elsewhere, our thread will periodically poll the socket-inode to
++ * see if someone else steals ours.
+ */
+static int create_listener_socket(const char *path,
-+ const struct ipc_server_opts *ipc_opts)
++ const struct ipc_server_opts *ipc_opts,
++ struct stat *st_socket)
+{
++ struct stat st;
++ struct strbuf buf_uniq = STRBUF_INIT;
+ int fd_listen;
-+ int fd_client;
-+ struct unix_stream_listen_opts uslg_opts = {
-+ .listen_backlog_size = LISTEN_BACKLOG,
-+ .force_unlink_before_bind = 0,
-+ .disallow_chdir = ipc_opts->uds_disallow_chdir
-+ };
-+
-+ trace2_data_string("ipc-server", NULL, "try-listen-gently", path);
-+
-+ /*
-+ * Assume socket-inode does not exist and try to (gently)
-+ * create a new socket-inode on disk at pathname and bind
-+ * socket-fd to it.
-+ */
-+ fd_listen = unix_stream_listen_gently(path, &uslg_opts);
-+ if (fd_listen >= 0)
-+ return fd_listen;
++ struct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;
+
-+ if (errno != EADDRINUSE)
-+ return error_errno(_("could not create socket '%s'"),
-+ path);
-+
-+ trace2_data_string("ipc-server", NULL, "try-detect-server", path);
-+
-+ /*
-+ * A socket-inode at pathname exists on disk, but we don't
-+ * know if it a server is using it or if it is a stale inode.
-+ *
-+ * poke it with a trivial connection to try to find out.
-+ */
-+ fd_client = unix_stream_connect(path);
-+ if (fd_client >= 0) {
++ if (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {
++ int fd_client;
+ /*
-+ * An existing service process is alive and accepted our
-+ * connection.
++ * A socket-inode at `path` exists on disk, but we
++ * don't know whether it belongs to an active server
++ * or if the last server died without cleaning up.
++ *
++ * Poke it with a trivial connection to try to find out.
+ */
-+ close(fd_client);
-+
-+ /*
-+ * We cannot create a new socket-inode here, so we cannot
-+ * startup a new server on this pathname.
-+ */
-+ errno = EADDRINUSE;
-+ return error_errno(_("socket already in use '%s'"),
++ trace2_data_string("ipc-server", NULL, "try-detect-server",
+ path);
++ fd_client = unix_stream_connect(path);
++ if (fd_client >= 0) {
++ close(fd_client);
++ errno = EADDRINUSE;
++ return error_errno(_("socket already in use '%s'"),
++ path);
++ }
+ }
+
-+ trace2_data_string("ipc-server", NULL, "try-listen-force", path);
-+
+ /*
-+ * A socket-inode at pathname exists on disk, but we were not
-+ * able to connect to it, so we believe that this is a stale
-+ * socket-inode that a previous server forgot to delete. Use
-+ * the tradional solution: force unlink it and create a new
-+ * one.
-+ *
-+ * TODO Note that it is possible that another server is
-+ * listening, but is either just starting up and not yet
-+ * responsive or is stuck somehow. For now, I'm OK with
-+ * stealing the socket-inode from it in this case.
++ * Create pathname to our "unique" socket and set it up for
++ * business.
+ */
-+ uslg_opts.force_unlink_before_bind = 1;
-+ fd_listen = unix_stream_listen_gently(path, &uslg_opts);
-+ if (fd_listen >= 0)
-+ return fd_listen;
-+
-+ return error_errno(_("could not force create socket '%s'"), path);
-+}
-+
-+static int setup_listener_socket(const char *path, ino_t *inode,
-+ const struct ipc_server_opts *ipc_opts)
-+{
-+ int fd_listen;
-+ struct stat st;
-+
-+ trace2_region_enter("ipc-server", "create-listener_socket", NULL);
-+ fd_listen = create_listener_socket(path, ipc_opts);
-+ trace2_region_leave("ipc-server", "create-listener_socket", NULL);
++ strbuf_addf(&buf_uniq, "%s.%d", path, getpid());
+
-+ if (fd_listen < 0)
-+ return fd_listen;
-+
-+ /*
-+ * We just bound a socket (descriptor) to a newly created unix
-+ * domain socket in the filesystem. Capture the inode number
-+ * so we can later detect if/when someone else force-creates a
-+ * new socket and effectively steals the path from us. (Which
-+ * would leave us listening to a socket that no client could
-+ * reach.)
-+ */
-+ if (lstat(path, &st) < 0) {
++ uslg_opts.listen_backlog_size = LISTEN_BACKLOG;
++ uslg_opts.force_unlink_before_bind = 1;
++ uslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;
++ fd_listen = unix_stream_listen(buf_uniq.buf, &uslg_opts);
++ if (fd_listen < 0) {
+ int saved_errno = errno;
++ error_errno(_("could not create listener socket '%s'"),
++ buf_uniq.buf);
++ strbuf_release(&buf_uniq);
++ errno = saved_errno;
++ return -1;
++ }
+
++ if (lstat(buf_uniq.buf, st_socket)) {
++ int saved_errno = errno;
++ error_errno(_("could not stat listener socket '%s'"),
++ buf_uniq.buf);
+ close(fd_listen);
-+ unlink(path);
-+
++ unlink(buf_uniq.buf);
++ strbuf_release(&buf_uniq);
+ errno = saved_errno;
-+ return error_errno(_("could not lstat listener socket '%s'"),
-+ path);
++ return -1;
+ }
+
+ if (set_socket_blocking_flag(fd_listen, 1)) {
+ int saved_errno = errno;
-+
++ error_errno(_("could not set listener socket nonblocking '%s'"),
++ buf_uniq.buf);
+ close(fd_listen);
-+ unlink(path);
++ unlink(buf_uniq.buf);
++ strbuf_release(&buf_uniq);
++ errno = saved_errno;
++ return -1;
++ }
+
++ /*
++ * Install it as the "real" socket so that clients will starting
++ * connecting to our socket.
++ */
++ if (rename(buf_uniq.buf, path)) {
++ int saved_errno = errno;
++ error_errno(_("could not create listener socket '%s'"), path);
++ close(fd_listen);
++ unlink(buf_uniq.buf);
++ strbuf_release(&buf_uniq);
+ errno = saved_errno;
-+ return error_errno(_("making listener socket nonblocking '%s'"),
-+ path);
++ return -1;
+ }
+
-+ *inode = st.st_ino;
++ strbuf_release(&buf_uniq);
++ trace2_data_string("ipc-server", NULL, "try-listen", path);
++ return fd_listen;
++}
++
++static int setup_listener_socket(const char *path, struct stat *st_socket,
++ const struct ipc_server_opts *ipc_opts)
++{
++ int fd_listen;
++
++ trace2_region_enter("ipc-server", "create-listener_socket", NULL);
++ fd_listen = create_listener_socket(path, ipc_opts, st_socket);
++ trace2_region_leave("ipc-server", "create-listener_socket", NULL);
+
+ return fd_listen;
+}
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+{
+ struct ipc_server_data *server_data;
+ int fd_listen;
-+ ino_t inode_listen;
++ struct stat st_listen;
+ int sv[2];
+ int k;
+ int nr_threads = opts->nr_threads;
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ path);
+ }
+
-+ fd_listen = setup_listener_socket(path, &inode_listen, opts);
++ fd_listen = setup_listener_socket(path, &st_listen, opts);
+ if (fd_listen < 0) {
+ int saved_errno = errno;
+ close(sv[0]);
@@ compat/simple-ipc/ipc-unix-socket.c (new)
+ server_data->accept_thread->magic = MAGIC_ACCEPT_THREAD_DATA;
+ server_data->accept_thread->server_data = server_data;
+ server_data->accept_thread->fd_listen = fd_listen;
-+ server_data->accept_thread->inode_listen = inode_listen;
++ server_data->accept_thread->st_listen = st_listen;
+ server_data->accept_thread->fd_send_shutdown = sv[0];
+ server_data->accept_thread->fd_wait_shutdown = sv[1];
+
--
gitgitgadget
From: Junio C Hamano via GitGitGadget <hidden> Date: 2021-02-01 19:47:06
From: Junio C Hamano <redacted>
We run "git pull" against "$cask_repo"; clarify that we are
expecting not to have any of our own modifications and running "git
pull" to merely update, by passing "--ff-only" on the command line.
Also, the "brew cask install" command line triggers an error message
that says:
Error: Calling brew cask install is disabled! Use brew install
[--cask] instead.
In addition, "brew install caskroom/cask/perforce" step triggers an
error that says:
Error: caskroom/cask was moved. Tap homebrew/cask instead.
Attempt to see if blindly following the suggestion in these error
messages gets us into a better shape.
Signed-off-by: Junio C Hamano <redacted>
---
ci/install-dependencies.sh | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-02-01 19:47:39
From: Johannes Schindelin <redacted>
This function currently has only one caller: `apply_multi_file_filter()`
in `convert.c`. That caller wants a flush packet to be written after
writing the payload.
However, we are about to introduce a user that wants to write many
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
Signed-off-by: Jeff Hostetler <redacted>
Signed-off-by: Johannes Schindelin <redacted>
---
convert.c | 2 +-
pkt-line.c | 9 ++++++---
pkt-line.h | 4 +++-
3 files changed, 10 insertions(+), 5 deletions(-)
@@ -275,14 +275,17 @@ int write_packetized_from_fd(int fd_in, int fd_out)returnerr;}-intwrite_packetized_from_buf(constchar*src_in,size_tlen,intfd_out)+intwrite_packetized_from_buf(constchar*src_in,size_tlen,intfd_out,+intflush_at_end){staticstructpacket_scratch_spacescratch;-returnwrite_packetized_from_buf2(src_in,len,fd_out,&scratch);+returnwrite_packetized_from_buf2(src_in,len,fd_out,+flush_at_end,&scratch);}intwrite_packetized_from_buf2(constchar*src_in,size_tlen,intfd_out,+intflush_at_end,structpacket_scratch_space*scratch){interr=0;
@@ -299,7 +302,7 @@ int write_packetized_from_buf2(const char *src_in, size_t len, int fd_out,err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write,scratch);bytes_written+=bytes_to_write;}-if(!err)+if(!err&&flush_at_end)err=packet_flush_gently(fd_out);returnerr;}
From: Jeff King <hidden> Date: 2021-02-02 09:49:44
On Mon, Feb 01, 2021 at 07:45:37PM +0000, Johannes Schindelin via GitGitGadget wrote:
From: Johannes Schindelin <redacted>
This function currently has only one caller: `apply_multi_file_filter()`
in `convert.c`. That caller wants a flush packet to be written after
writing the payload.
However, we are about to introduce a user that wants to write many
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
I think this is a sign that the function is not very well-designed in
the first place. It seems like the code would be easier to understand
overall if that caller just explicitly did the flush itself. It even
already does so in other cases!
Something like (untested):
convert.c | 4 ++++
pkt-line.c | 4 ----
2 files changed, 4 insertions(+), 4 deletions(-)
@@ -256,8 +256,6 @@ int write_packetized_from_fd(int fd_in, int fd_out)break;err=packet_write_gently(fd_out,buf,bytes_to_write);}-if(!err)-err=packet_flush_gently(fd_out);returnerr;}
@@ -277,8 +275,6 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write);bytes_written+=bytes_to_write;}-if(!err)-err=packet_flush_gently(fd_out);returnerr;}-Peff
From: Johannes Schindelin <hidden> Date: 2021-02-02 22:58:06
Hi Peff,
On Tue, 2 Feb 2021, Jeff King wrote:
On Mon, Feb 01, 2021 at 07:45:37PM +0000, Johannes Schindelin via GitGitGadget wrote:
quoted
From: Johannes Schindelin <redacted>
This function currently has only one caller: `apply_multi_file_filter()`
in `convert.c`. That caller wants a flush packet to be written after
writing the payload.
However, we are about to introduce a user that wants to write many
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
I think this is a sign that the function is not very well-designed in
the first place. It seems like the code would be easier to understand
overall if that caller just explicitly did the flush itself. It even
already does so in other cases!
Something like (untested):
@@ -256,8 +256,6 @@ int write_packetized_from_fd(int fd_in, int fd_out)break;err=packet_write_gently(fd_out,buf,bytes_to_write);}-if(!err)-err=packet_flush_gently(fd_out);returnerr;}
@@ -277,8 +275,6 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write);bytes_written+=bytes_to_write;}-if(!err)-err=packet_flush_gently(fd_out);returnerr;}-Peff
From: Jeff Hostetler <hidden> Date: 2021-02-05 18:38:24
On 2/2/21 4:48 AM, Jeff King wrote:
On Mon, Feb 01, 2021 at 07:45:37PM +0000, Johannes Schindelin via GitGitGadget wrote:
quoted
From: Johannes Schindelin <redacted>
This function currently has only one caller: `apply_multi_file_filter()`
in `convert.c`. That caller wants a flush packet to be written after
writing the payload.
However, we are about to introduce a user that wants to write many
packets before a final flush packet, so let's extend this function to
prepare for that scenario.
I think this is a sign that the function is not very well-designed in
the first place. It seems like the code would be easier to understand
overall if that caller just explicitly did the flush itself. It even
already does so in other cases!
I agree. I'll move flush to the caller and rename the write packetized
function slightly to guard against new callers assuming the old behavior
during the transition.
Jeff
@@ -256,8 +256,6 @@ int write_packetized_from_fd(int fd_in, int fd_out)break;err=packet_write_gently(fd_out,buf,bytes_to_write);}-if(!err)-err=packet_flush_gently(fd_out);returnerr;}
@@ -277,8 +275,6 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write);bytes_written+=bytes_to_write;}-if(!err)-err=packet_flush_gently(fd_out);returnerr;}-Peff
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-02-01 19:48:14
From: Johannes Schindelin <redacted>
So far, the (possibly indirect) callers of `get_packet_data()` can ask
that function to return an error instead of `die()`ing upon end-of-file.
However, random read errors will still cause the process to die.
So let's introduce an explicit option to tell the packet reader
machinery to please be nice and only return an error.
This change prepares pkt-line for use by long-running daemon processes.
Such processes should be able to serve multiple concurrent clients and
and survive random IO errors. If there is an error on one connection,
a daemon should be able to drop that connection and continue serving
existing and future connections.
This ability will be used by a Git-aware "Internal FSMonitor" feature
in a later patch series.
Signed-off-by: Johannes Schindelin <redacted>
---
pkt-line.c | 19 +++++++++++++++++--
pkt-line.h | 4 ++++
2 files changed, 21 insertions(+), 2 deletions(-)
@@ -323,8 +323,11 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,*src_size-=ret;}else{ret=read_in_full(fd,dst,size);-if(ret<0)+if(ret<0){+if(options&PACKET_READ_NEVER_DIE)+returnerror_errno(_("read error"));die_errno(_("read error"));+}}/* And complain if we didn't get enough bytes to satisfy the read. */
@@ -332,6 +335,8 @@ static int get_packet_data(int fd, char **src_buf, size_t *src_size,if(options&PACKET_READ_GENTLE_ON_EOF)return-1;+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("the remote end hung up unexpectedly"));die(_("the remote end hung up unexpectedly"));}
@@ -360,6 +365,9 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,len=packet_length(linelen);if(len<0){+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("protocol error: bad line length "+"character: %.4s"),linelen);die(_("protocol error: bad line length character: %.4s"),linelen);}elseif(!len){packet_trace("0000",4,0);
@@ -374,12 +382,19 @@ enum packet_read_status packet_read_with_status(int fd, char **src_buffer,*pktlen=0;returnPACKET_READ_RESPONSE_END;}elseif(len<4){+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("protocol error: bad line length %d"),+len);die(_("protocol error: bad line length %d"),len);}len-=4;-if((unsigned)len>=size)+if((unsigned)len>=size){+if(options&PACKET_READ_NEVER_DIE)+returnerror(_("protocol error: bad line length %d"),+len);die(_("protocol error: bad line length %d"),len);+}if(get_packet_data(fd,src_buffer,src_len,buffer,len,options)<0){*pktlen=-1;
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2021-02-01 19:48:14
From: Johannes Schindelin <redacted>
The `read_packetized_to_strbuf()` function reads packets into a strbuf
until a flush packet has been received. So far, it has only one caller:
`apply_multi_file_filter()` in `convert.c`. This caller really only
needs the `PACKET_READ_GENTLE_ON_EOF` option to be passed to
`packet_read()` (which makes sense in the scenario where packets should
be read until a flush packet is received).
We are about to introduce a caller that wants to pass other options
through to `packet_read()`, so let's extend the function signature
accordingly.
Signed-off-by: Johannes Schindelin <redacted>
---
convert.c | 2 +-
pkt-line.c | 4 ++--
pkt-line.h | 6 +++++-
3 files changed, 8 insertions(+), 4 deletions(-)
This feels a little magical to me. Since read_packetized_to_strbuf only
has the one caller you mention, why not have the caller pass all of the
options (including PACKET_READ_GENTLE_ON_EOF)?
quoted hunk
if (packet_len <= 0)
break;
sb_out->len += packet_len;
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:48:44
From: Jeff Hostetler <redacted>
Teach `unix_stream_connect()` to return error rather than calling `die()`
when a socket cannot be created.
Signed-off-by: Jeff Hostetler <redacted>
---
unix-socket.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:48:44
From: Jeff Hostetler <redacted>
Update `unix_stream_listen()` to take an options structure to override
default behaviors. This includes the size of the `listen()` backlog
and whether it should always unlink the socket file before trying to
create a new one. Also eliminate calls to `die()` if it cannot create
a socket.
Normally, `unix_stream_listen()` always tries to `unlink()` the
socket-path before calling `bind()`. If there is an existing
server/daemon already bound and listening on that socket-path, our
`unlink()` would have the effect of disassociating the existing
server's bound-socket-fd from the socket-path without notifying the
existing server. The existing server could continue to service
existing connections (accepted-socket-fd's), but would not receive any
futher new connections (since clients rendezvous via the socket-path).
The existing server would effectively be offline but yet appear to be
active.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
interval between the forced `unlink()` and the subsequent `bind()` (which
recreates the socket-path that is bound to a new socket-fd in the current
process).
Signed-off-by: Jeff Hostetler <redacted>
---
builtin/credential-cache--daemon.c | 3 ++-
unix-socket.c | 28 +++++++++++++++++++++-------
unix-socket.h | 14 +++++++++++++-
3 files changed, 36 insertions(+), 9 deletions(-)
@@ -203,9 +203,10 @@ static int serve_cache_loop(int fd)staticvoidserve_cache(constchar*socket_path,intdebug){+structunix_stream_listen_optsopts=UNIX_STREAM_LISTEN_OPTS_INIT;intfd;-fd=unix_stream_listen(socket_path);+fd=unix_stream_listen(socket_path,&opts);if(fd<0)die_errno("unable to bind to '%s'",socket_path);
From: Jeff King <hidden> Date: 2021-02-02 10:15:29
On Mon, Feb 01, 2021 at 07:45:44PM +0000, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <redacted>
Update `unix_stream_listen()` to take an options structure to override
default behaviors. This includes the size of the `listen()` backlog
and whether it should always unlink the socket file before trying to
create a new one. Also eliminate calls to `die()` if it cannot create
a socket.
I sent a follow-up on the previous patch, but I think this part about
the die() should be folded in there.
Likewise I think it would probably be easier to follow if we added the
backlog parameter and the unlink options in separate patches. The
backlog thing is small, but the unlink part is subtle and requires
explanation. That's a good sign it might do better in its own commit.
Normally, `unix_stream_listen()` always tries to `unlink()` the
socket-path before calling `bind()`. If there is an existing
server/daemon already bound and listening on that socket-path, our
`unlink()` would have the effect of disassociating the existing
server's bound-socket-fd from the socket-path without notifying the
existing server. The existing server could continue to service
existing connections (accepted-socket-fd's), but would not receive any
futher new connections (since clients rendezvous via the socket-path).
The existing server would effectively be offline but yet appear to be
active.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
interval between the forced `unlink()` and the subsequent `bind()` (which
recreates the socket-path that is bound to a new socket-fd in the current
process).
OK. I'm still not sure of the endgame here for writing non-racy code to
establish the socket (which is going to require either some atomic
renaming or some dot-locking in the caller). But it's plausible to me
that this option will be a useful primitive.
The implementation looks correct, though here are a few small
observations/questions/nits:
-int unix_stream_listen(const char *path)
+int unix_stream_listen(const char *path,
+ const struct unix_stream_listen_opts *opts)
{
- int fd, saved_errno;
+ int fd = -1;
+ int saved_errno;
+ int bind_successful = 0;
+ int backlog;
struct sockaddr_un sa;
struct unix_sockaddr_context ctx;
- unlink(path);
-
if (unix_sockaddr_init(&sa, path, &ctx) < 0)
return -1;
We can return directly here, because we know there is nothing to clean
up. Which I thought mean that here...
+
fd = socket(AF_UNIX, SOCK_STREAM, 0);
if (fd < 0)
- die_errno("unable to create socket");
+ goto fail;
...we are in the same boat. We did not create a socket, so we can just
return. That makes our cleanup code a bit simpler. But we can't do that,
because unix_sockaddr_init() may have done things that need cleaning up
(like chdir). So what you have here is correct.
IMHO that is all the more reason to push this (and the similar code in
unix_stream_connect() added in patch 13) into the previous patch.
+ if (opts->force_unlink_before_bind)
+ unlink(path);
if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
goto fail;
+ bind_successful = 1;
And this one needs to mark a flag explicitly, because we have no other
visible way of knowing we need to do the unlink. Makes sense.
- if (listen(fd, 5) < 0)
+ if (opts->listen_backlog_size > 0)
+ backlog = opts->listen_backlog_size;
+ else
+ backlog = 5;
+ if (listen(fd, backlog) < 0)
The default-to-5 is a bit funny here. We already set the default to 5 in
UNIX_STREAM_LISTEN_OPTS_INIT. Should it be "0" there, so callers can
treat that as "use the default", which we fill in here? It probably
doesn't matter much in practice, but it seems cleaner to have only one
spot with the magic number.
quoted hunk
@@ -114,7 +125,10 @@ int unix_stream_listen(const char *path) fail: saved_errno = errno; unix_sockaddr_cleanup(&ctx);- close(fd);+ if (fd != -1)+ close(fd);+ if (bind_successful)+ unlink(path); errno = saved_errno; return -1; }
Should we unlink before closing? I usually try to undo actions in the
reverse order that they were done. I thought at first it might even
matter here, such that we'd atomically relinquish the name without
having a moment where it still points to a closed socket (which might be
less confusing to somebody else trying to connect). But I guess there
will always be such a moment, because it's not like we would ever
accept() or service a request.
-Peff
From: Jeff Hostetler <hidden> Date: 2021-02-06 03:01:37
On 2/2/21 5:14 AM, Jeff King wrote:
On Mon, Feb 01, 2021 at 07:45:44PM +0000, Jeff Hostetler via GitGitGadget wrote:
quoted
From: Jeff Hostetler <redacted>
Update `unix_stream_listen()` to take an options structure to override
default behaviors. This includes the size of the `listen()` backlog
and whether it should always unlink the socket file before trying to
create a new one. Also eliminate calls to `die()` if it cannot create
a socket.
I sent a follow-up on the previous patch, but I think this part about
the die() should be folded in there.
Likewise I think it would probably be easier to follow if we added the
backlog parameter and the unlink options in separate patches. The
backlog thing is small, but the unlink part is subtle and requires
explanation. That's a good sign it might do better in its own commit.
Yes, that helped having them in 2 patches each with 1 concern.
quoted
Normally, `unix_stream_listen()` always tries to `unlink()` the
socket-path before calling `bind()`. If there is an existing
server/daemon already bound and listening on that socket-path, our
`unlink()` would have the effect of disassociating the existing
server's bound-socket-fd from the socket-path without notifying the
existing server. The existing server could continue to service
existing connections (accepted-socket-fd's), but would not receive any
futher new connections (since clients rendezvous via the socket-path).
The existing server would effectively be offline but yet appear to be
active.
Furthermore, `unix_stream_listen()` creates an opportunity for a brief
race condition for connecting clients if they try to connect in the
interval between the forced `unlink()` and the subsequent `bind()` (which
recreates the socket-path that is bound to a new socket-fd in the current
process).
OK. I'm still not sure of the endgame here for writing non-racy code to
establish the socket (which is going to require either some atomic
renaming or some dot-locking in the caller). But it's plausible to me
that this option will be a useful primitive.
In part 14/14 in `ipc-unix-sockets.c:create_listener_socket()` I have
code in the calling layer to (try to) handle both the startup races
and basic collisions with existing long-running servers already using
the socket.
But you're right, it might be good to revisit that as a primitive at
this layer. We only have 1 other caller right now and I don't know
enough about `credential-cache--daemon` to know if it would benefit
from this or not.
The implementation looks correct, though here are a few small
observations/questions/nits:
quoted
-int unix_stream_listen(const char *path)
+int unix_stream_listen(const char *path,
+ const struct unix_stream_listen_opts *opts)
{
- int fd, saved_errno;
+ int fd = -1;
+ int saved_errno;
+ int bind_successful = 0;
+ int backlog;
struct sockaddr_un sa;
struct unix_sockaddr_context ctx;
- unlink(path);
-
if (unix_sockaddr_init(&sa, path, &ctx) < 0)
return -1;
We can return directly here, because we know there is nothing to clean
up. Which I thought mean that here...
quoted
+
fd = socket(AF_UNIX, SOCK_STREAM, 0);
if (fd < 0)
- die_errno("unable to create socket");
+ goto fail;
...we are in the same boat. We did not create a socket, so we can just
return. That makes our cleanup code a bit simpler. But we can't do that,
because unix_sockaddr_init() may have done things that need cleaning up
(like chdir). So what you have here is correct.
IMHO that is all the more reason to push this (and the similar code in
unix_stream_connect() added in patch 13) into the previous patch.
Agreed.
quoted
+ if (opts->force_unlink_before_bind)
+ unlink(path);
if (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)
goto fail;
+ bind_successful = 1;
And this one needs to mark a flag explicitly, because we have no other
visible way of knowing we need to do the unlink. Makes sense.
quoted
- if (listen(fd, 5) < 0)
+ if (opts->listen_backlog_size > 0)
+ backlog = opts->listen_backlog_size;
+ else
+ backlog = 5;
+ if (listen(fd, backlog) < 0)
The default-to-5 is a bit funny here. We already set the default to 5 in
UNIX_STREAM_LISTEN_OPTS_INIT. Should it be "0" there, so callers can
treat that as "use the default", which we fill in here? It probably
doesn't matter much in practice, but it seems cleaner to have only one
spot with the magic number.
I'll refactor this a bit.
quoted
@@ -114,7 +125,10 @@ int unix_stream_listen(const char *path) fail: saved_errno = errno; unix_sockaddr_cleanup(&ctx);- close(fd);+ if (fd != -1)+ close(fd);+ if (bind_successful)+ unlink(path); errno = saved_errno; return -1; }
Should we unlink before closing? I usually try to undo actions in the
reverse order that they were done. I thought at first it might even
matter here, such that we'd atomically relinquish the name without
having a moment where it still points to a closed socket (which might be
less confusing to somebody else trying to connect). But I guess there
will always be such a moment, because it's not like we would ever
accept() or service a request.
I'm not sure it matters, but it does look better to unwind things
in reverse order. And yes, unlinking first is a little bit safer.
From: Jeff King <hidden> Date: 2021-02-09 16:33:29
On Fri, Feb 05, 2021 at 06:28:13PM -0500, Jeff Hostetler wrote:
quoted
OK. I'm still not sure of the endgame here for writing non-racy code to
establish the socket (which is going to require either some atomic
renaming or some dot-locking in the caller). But it's plausible to me
that this option will be a useful primitive.
In part 14/14 in `ipc-unix-sockets.c:create_listener_socket()` I have
code in the calling layer to (try to) handle both the startup races
and basic collisions with existing long-running servers already using
the socket.
There you make a temp socket and then try to rename it into place. But
because rename() overwrites the destination, it still seems like two
creating processes can race each other. Something like:
0. There's no "foo" socket (or maybe there is a stale one that
nobody's listening on).
1. Process A wants to become the listener. So it creates foo.A.
2. Process B likewise. It creates foo.B.
3. Process A renames foo.A to foo. It believes it will now service
clients.
4. Process B renames foo.B to foo. Now process A is stranded but
doesn't realize it.
I.e., I don't think this is much different than an unlink+create
strategy. You've eliminated the window where a process C shows up during
steps 3 and 4 and sees no socket (because somebody else is in the midst
of a non-atomic unlink+create operation). But there's no atomicity
between the "ping the socket" and "create the socket" steps.
But you're right, it might be good to revisit that as a primitive at
this layer. We only have 1 other caller right now and I don't know
enough about `credential-cache--daemon` to know if it would benefit
from this or not.
Yeah, having seen patch 14, it looks like your only new caller always
sets the new unlink option to 1. So it might not be worth making it
optional if you don't need it (especially because the rename trick,
assuming it's portable, is superior to unlink+create; and you'd always
be fine with an unlink on the temp socket).
The call in credential-cache--daemon is definitely racy. It's pretty
much the same thing: it pings the socket to see if it's alive, but is
still susceptible to the problem above. I was was never too concerned
about it, since the whole point of the daemon is to hang around until
its contents expire. If it loses the race and nobody contacts it, the
worst case is it waits 30 seconds for somebody to give it data before
exiting. It would benefit slightly from switching to the rename
strategy, but the bigger race would remain.
-Peff
From: Jeff Hostetler <hidden> Date: 2021-02-09 17:40:56
On 2/9/21 11:32 AM, Jeff King wrote:
On Fri, Feb 05, 2021 at 06:28:13PM -0500, Jeff Hostetler wrote:
quoted
quoted
OK. I'm still not sure of the endgame here for writing non-racy code to
establish the socket (which is going to require either some atomic
renaming or some dot-locking in the caller). But it's plausible to me
that this option will be a useful primitive.
In part 14/14 in `ipc-unix-sockets.c:create_listener_socket()` I have
code in the calling layer to (try to) handle both the startup races
and basic collisions with existing long-running servers already using
the socket.
There you make a temp socket and then try to rename it into place. But
because rename() overwrites the destination, it still seems like two
creating processes can race each other. Something like:
0. There's no "foo" socket (or maybe there is a stale one that
nobody's listening on).
1. Process A wants to become the listener. So it creates foo.A.
2. Process B likewise. It creates foo.B.
3. Process A renames foo.A to foo. It believes it will now service
clients.
4. Process B renames foo.B to foo. Now process A is stranded but
doesn't realize it.
Yeah, in my version two processes could still create uniquely named
sockets and then do the rename trick. But they capture the inode
number of the socket before they do that. They periodically lstat
the socket to see if the inode number has changed and if so, assume
it has been stolen from them. (A bit of a hack, I admit.)
And I was assuming that 2 servers starting at about the same time
are effectively equivalent -- it doesn't matter which one dies, since
they both should have the same amount of cached state. Unlike the
case where a long-running server (with lots of state) is replaced by
a newcomer.
I.e., I don't think this is much different than an unlink+create
strategy. You've eliminated the window where a process C shows up during
steps 3 and 4 and sees no socket (because somebody else is in the midst
of a non-atomic unlink+create operation). But there's no atomicity
between the "ping the socket" and "create the socket" steps.
quoted
But you're right, it might be good to revisit that as a primitive at
this layer. We only have 1 other caller right now and I don't know
enough about `credential-cache--daemon` to know if it would benefit
from this or not.
Yeah, having seen patch 14, it looks like your only new caller always
sets the new unlink option to 1. So it might not be worth making it
optional if you don't need it (especially because the rename trick,
assuming it's portable, is superior to unlink+create; and you'd always
be fine with an unlink on the temp socket).
I am wondering if we can use the .LOCK file magic to our advantage
here (in sort of an off-label use). If we have the server create a
lockfile "<path>.LOCK" and if successful leave it open/locked for the
life of the server (rather than immediately renaming it onto <path>)
and let the normal shutdown code rollback/delete the lockfile in the
cleanup/atexit.
If the server successfully creates the lockfile, then unlink+create
the socket at <path>.
That would give us the unique/exclusive creation (on the lock) that
we need. Then wrap that with all the edge case cleanup code to
create/delete/manage the peer socket. Basically if the lock exists,
there should be a live server listening to the socket (unless there
was a crash...).
And yes, then I don't think I need the `preserve_existing` bit in the
opts struct.
The call in credential-cache--daemon is definitely racy. It's pretty
much the same thing: it pings the socket to see if it's alive, but is
still susceptible to the problem above. I was was never too concerned
about it, since the whole point of the daemon is to hang around until
its contents expire. If it loses the race and nobody contacts it, the
worst case is it waits 30 seconds for somebody to give it data before
exiting. It would benefit slightly from switching to the rename
strategy, but the bigger race would remain.
-Peff
From: Jeff King <hidden> Date: 2021-02-10 15:56:17
On Tue, Feb 09, 2021 at 12:39:22PM -0500, Jeff Hostetler wrote:
Yeah, in my version two processes could still create uniquely named
sockets and then do the rename trick. But they capture the inode
number of the socket before they do that. They periodically lstat
the socket to see if the inode number has changed and if so, assume
it has been stolen from them. (A bit of a hack, I admit.)
OK, that makes more sense. I saw the mention of the inode stuff in a
comment, but I didn't see it in the code (I guess if it's a periodic
check it's not in that initial socket creation function).
And I was assuming that 2 servers starting at about the same time
are effectively equivalent -- it doesn't matter which one dies, since
they both should have the same amount of cached state. Unlike the
case where a long-running server (with lots of state) is replaced by
a newcomer.
Yeah, I agree with that notion in general. I do think it would be easier
to reason about if the creation were truly race-proof (probably with a
dot-lock; see below), rather than the later "check if we got replaced"
thing. OTOH, that "check" strategy covers a variety of cases (including
that somebody tried to ping us, decided we weren't alive due to a
timeout or some other system reason, and then replaced our socket).
Another strategy there could be having the daemon just decide to quit if
nobody contacts it for N time units. It is, after all, a cache. Even if
nobody replaces the socket, it probably makes sense to eventually decide
that the memory we're holding isn't going to good use.
I am wondering if we can use the .LOCK file magic to our advantage
here (in sort of an off-label use). If we have the server create a
lockfile "<path>.LOCK" and if successful leave it open/locked for the
life of the server (rather than immediately renaming it onto <path>)
and let the normal shutdown code rollback/delete the lockfile in the
cleanup/atexit.
If the server successfully creates the lockfile, then unlink+create
the socket at <path>.
I don't even think this is off-label. Though the normal use is for the
.lock file to get renamed into place as the official file, there are a
few other places where we use it solely for mutual exclusion. You just
always end with rollback_lock_file(), and never "commit" it.
So something like:
1. Optimistically see if socket "foo" is present and accepting
connections.
2. If not, then take "foo.lock". If somebody else is holding it, loop
with a timeout waiting for them to come alive.
3. Assuming we got the lock, then either unlink+create the socket as
"foo", or rename-into-place. I don't think it matters that much
which.
4. Rollback "foo.lock", unlinking it.
Then one process wins the lock and creates the socket, while any
simultaneous creators spin in step 2, and eventually connect to the
winner.
That would give us the unique/exclusive creation (on the lock) that
we need. Then wrap that with all the edge case cleanup code to
create/delete/manage the peer socket. Basically if the lock exists,
there should be a live server listening to the socket (unless there
was a crash...).
I think you'd want to delete the lock as soon as you're done with the
setup. That reduces the chances that a dead server (e.g., killed by a
power outage without the chance to clean up after itself) leaves a stale
lock sitting around.
-Peff
From: Jeff Hostetler <hidden> Date: 2021-02-10 21:32:12
On 2/10/21 10:55 AM, Jeff King wrote:
On Tue, Feb 09, 2021 at 12:39:22PM -0500, Jeff Hostetler wrote:
quoted
Yeah, in my version two processes could still create uniquely named
sockets and then do the rename trick. But they capture the inode
number of the socket before they do that. They periodically lstat
the socket to see if the inode number has changed and if so, assume
it has been stolen from them. (A bit of a hack, I admit.)
OK, that makes more sense. I saw the mention of the inode stuff in a
comment, but I didn't see it in the code (I guess if it's a periodic
check it's not in that initial socket creation function).
Yeah, there's a very slow poll(2) loop in the listen/accept thread
that watches for that and new connections (and quit messages).
quoted
And I was assuming that 2 servers starting at about the same time
are effectively equivalent -- it doesn't matter which one dies, since
they both should have the same amount of cached state. Unlike the
case where a long-running server (with lots of state) is replaced by
a newcomer.
Yeah, I agree with that notion in general. I do think it would be easier
to reason about if the creation were truly race-proof (probably with a
dot-lock; see below), rather than the later "check if we got replaced"
thing. OTOH, that "check" strategy covers a variety of cases (including
that somebody tried to ping us, decided we weren't alive due to a
timeout or some other system reason, and then replaced our socket).
Another strategy there could be having the daemon just decide to quit if
nobody contacts it for N time units. It is, after all, a cache. Even if
nobody replaces the socket, it probably makes sense to eventually decide
that the memory we're holding isn't going to good use.
I have the poll(2) loop set to recheck the inode for theft every 60
seconds (randomly chosen).
Assuming the socket isn't stolen, I want to leave any thoughts of
an auto-shutdown to the application layer above it. My next patch
series will use this ipc mechanism to build a FSMonitor daemon that
will watch the filesystem for changes and then be able to quickly
respond to a `git status`, so it is important that it be allowed to
run without any clients for a while (such a during a build). Yes,
memory concerns are important, so I do want it to auto-shutdown if
the socket is stolen (or the workdir is deleted).
quoted
I am wondering if we can use the .LOCK file magic to our advantage
here (in sort of an off-label use). If we have the server create a
lockfile "<path>.LOCK" and if successful leave it open/locked for the
life of the server (rather than immediately renaming it onto <path>)
and let the normal shutdown code rollback/delete the lockfile in the
cleanup/atexit.
If the server successfully creates the lockfile, then unlink+create
the socket at <path>.
I don't even think this is off-label. Though the normal use is for the
.lock file to get renamed into place as the official file, there are a
few other places where we use it solely for mutual exclusion. You just
always end with rollback_lock_file(), and never "commit" it.
So something like:
1. Optimistically see if socket "foo" is present and accepting
connections.
2. If not, then take "foo.lock". If somebody else is holding it, loop
with a timeout waiting for them to come alive.
3. Assuming we got the lock, then either unlink+create the socket as
"foo", or rename-into-place. I don't think it matters that much
which.
4. Rollback "foo.lock", unlinking it.
Then one process wins the lock and creates the socket, while any
simultaneous creators spin in step 2, and eventually connect to the
winner.
quoted
That would give us the unique/exclusive creation (on the lock) that
we need. Then wrap that with all the edge case cleanup code to
create/delete/manage the peer socket. Basically if the lock exists,
there should be a live server listening to the socket (unless there
was a crash...).
I think you'd want to delete the lock as soon as you're done with the
setup. That reduces the chances that a dead server (e.g., killed by a
power outage without the chance to clean up after itself) leaves a stale
lock sitting around.
Thanks this helps. I've got a version now that is a slight variation on
what you have here that seems to work nicely and has the short-lived
lock file. I'll post this shortly.
Jeff
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:48:44
From: Jeff Hostetler <redacted>
Brief design documentation for new IPC mechanism allowing
foreground Git client to talk with an existing daemon process
at a known location using a named pipe or unix domain socket.
Signed-off-by: Johannes Schindelin <redacted>
Signed-off-by: Jeff Hostetler <redacted>
---
Documentation/technical/api-simple-ipc.txt | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/technical/api-simple-ipc.txt
@@ -0,0 +1,34 @@+simple-ipc API+==============++The simple-ipc API is used to send an IPC message and response between+a (presumably) foreground Git client process to a background server or+daemon process. The server process must already be running. Multiple+client processes can simultaneously communicate with the server+process.++Communication occurs over a named pipe on Windows and a Unix domain+socket on other platforms. Clients and the server rendezvous at a+previously agreed-to application-specific pathname (which is outside+the scope of this design).++This IPC mechanism differs from the existing `sub-process.c` model+(Documentation/technical/long-running-process-protocol.txt) and used+by applications like Git-LFS. In the simple-ipc model the server is+assumed to be a very long-running system service. In contrast, in the+LFS-style sub-process model the helper is started with the foreground+process and exits when the foreground process terminates.++How the simple-ipc server is started is also outside the scope of the+IPC mechanism. For example, the server might be started during+maintenance operations.++The IPC protocol consists of a single request message from the client and+an optional request message from the server. For simplicity, pkt-line+routines are used to hide chunking and buffering concerns. Each side+terminates their message with a flush packet.+(Documentation/technical/protocol-common.txt)++The actual format of the client and server messages is application+specific. The IPC layer transmits and receives an opaque buffer without+any concern for the content within.
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:48:59
From: Jeff Hostetler <redacted>
Create Unix domain socket based implementation of "simple-ipc".
A set of `ipc_client` routines implement a client library to connect
to an `ipc_server` over a Unix domain socket, send a simple request,
and receive a single response. Clients use blocking IO on the socket.
A set of `ipc_server` routines implement a thread pool to listen for
and concurrently service client connections.
The server creates a new Unix domain socket at a known location. If a
socket already exists with that name, the server tries to determine if
another server is already listening on the socket or if the socket is
dead. If socket is busy, the server exits with an error rather than
stealing the socket. If the socket is dead, the server creates a new
one and starts up.
If while running, the server detects that its socket has been stolen
by another server, it automatically exits.
Signed-off-by: Jeff Hostetler <redacted>
---
Makefile | 2 +
compat/simple-ipc/ipc-unix-socket.c | 1127 +++++++++++++++++++++++++++
contrib/buildsystems/CMakeLists.txt | 2 +
simple-ipc.h | 7 +-
4 files changed, 1137 insertions(+), 1 deletion(-)
create mode 100644 compat/simple-ipc/ipc-unix-socket.c
@@ -0,0 +1,1127 @@+#include"cache.h"+#include"simple-ipc.h"+#include"strbuf.h"+#include"pkt-line.h"+#include"thread-utils.h"+#include"unix-socket.h"++#ifdef NO_UNIX_SOCKETS+#error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets+#endif++enumipc_active_stateipc_get_active_state(constchar*path)+{+enumipc_active_statestate=IPC_STATE__OTHER_ERROR;+structipc_client_connect_optionsoptions+=IPC_CLIENT_CONNECT_OPTIONS_INIT;+structstatst;+structipc_client_connection*connection_test=NULL;++options.wait_if_busy=0;+options.wait_if_not_found=0;++if(lstat(path,&st)==-1){+switch(errno){+caseENOENT:+caseENOTDIR:+returnIPC_STATE__NOT_LISTENING;+default:+returnIPC_STATE__INVALID_PATH;+}+}++/* also complain if a plain file is in the way */+if((st.st_mode&S_IFMT)!=S_IFSOCK)+returnIPC_STATE__INVALID_PATH;++/*+*JustbecausethefilesystemhasaS_IFSOCKtypeinode+*at`path`,doesn'tmeanitthatthereisaserverlistening.+*Pingittobesure.+*/+state=ipc_client_try_connect(path,&options,&connection_test);+ipc_client_close_connection(connection_test);++returnstate;+}++/*+*Thisvaluewaschosenatrandom.+*/+#define WAIT_STEP_MS (50)++/*+*Trytoconnecttotheserver.Iftheserverisjuststartingupor+*isverybusy,wemaynotgetaconnectionthefirsttime.+*/+staticenumipc_active_stateconnect_to_server(+constchar*path,+inttimeout_ms,+conststructipc_client_connect_options*options,+int*pfd)+{+intwait_ms=50;+intk;++*pfd=-1;++for(k=0;k<timeout_ms;k+=wait_ms){+intfd=unix_stream_connect(path);++if(fd!=-1){+*pfd=fd;+returnIPC_STATE__LISTENING;+}++if(errno==ENOENT){+if(!options->wait_if_not_found)+returnIPC_STATE__PATH_NOT_FOUND;++gotosleep_and_try_again;+}++if(errno==ETIMEDOUT){+if(!options->wait_if_busy)+returnIPC_STATE__NOT_LISTENING;++gotosleep_and_try_again;+}++if(errno==ECONNREFUSED){+if(!options->wait_if_busy)+returnIPC_STATE__NOT_LISTENING;++gotosleep_and_try_again;+}++returnIPC_STATE__OTHER_ERROR;++sleep_and_try_again:+sleep_millisec(wait_ms);+}++returnIPC_STATE__NOT_LISTENING;+}++/*+*Arandomlychosentimeoutvalue.+*/+#define MY_CONNECTION_TIMEOUT_MS (1000)++enumipc_active_stateipc_client_try_connect(+constchar*path,+conststructipc_client_connect_options*options,+structipc_client_connection**p_connection)+{+enumipc_active_statestate=IPC_STATE__OTHER_ERROR;+intfd=-1;++*p_connection=NULL;++trace2_region_enter("ipc-client","try-connect",NULL);+trace2_data_string("ipc-client",NULL,"try-connect/path",path);++state=connect_to_server(path,MY_CONNECTION_TIMEOUT_MS,+options,&fd);++trace2_data_intmax("ipc-client",NULL,"try-connect/state",+(intmax_t)state);+trace2_region_leave("ipc-client","try-connect",NULL);++if(state==IPC_STATE__LISTENING){+(*p_connection)=xcalloc(1,sizeof(structipc_client_connection));+(*p_connection)->fd=fd;+}++returnstate;+}++voidipc_client_close_connection(structipc_client_connection*connection)+{+if(!connection)+return;++if(connection->fd!=-1)+close(connection->fd);++free(connection);+}++intipc_client_send_command_to_connection(+structipc_client_connection*connection,+constchar*message,structstrbuf*answer)+{+intret=0;++strbuf_setlen(answer,0);++trace2_region_enter("ipc-client","send-command",NULL);++if(write_packetized_from_buf2(message,strlen(message),+connection->fd,1,+&connection->scratch_write_buffer)<0){+ret=error(_("could not send IPC command"));+gotodone;+}++if(read_packetized_to_strbuf(connection->fd,answer,+PACKET_READ_NEVER_DIE)<0){+ret=error(_("could not read IPC response"));+gotodone;+}++done:+trace2_region_leave("ipc-client","send-command",NULL);+returnret;+}++intipc_client_send_command(constchar*path,+conststructipc_client_connect_options*options,+constchar*message,structstrbuf*answer)+{+intret=-1;+enumipc_active_statestate;+structipc_client_connection*connection=NULL;++state=ipc_client_try_connect(path,options,&connection);++if(state!=IPC_STATE__LISTENING)+returnret;++ret=ipc_client_send_command_to_connection(connection,message,answer);++ipc_client_close_connection(connection);++returnret;+}++staticintset_socket_blocking_flag(intfd,intmake_nonblocking)+{+intflags;++flags=fcntl(fd,F_GETFL,NULL);++if(flags<0)+return-1;++if(make_nonblocking)+flags|=O_NONBLOCK;+else+flags&=~O_NONBLOCK;++returnfcntl(fd,F_SETFL,flags);+}++/*+*Magicnumbersusedtoannotatecallbackinstancedata.+*Theseareusedtohelpguardagainstaccidentallypassingthe+*wronginstancedataacrossmultiplelevelsofcallbacks(which+*iseasytodoifthereare`void*`arguments).+*/+enummagic{+MAGIC_SERVER_REPLY_DATA,+MAGIC_WORKER_THREAD_DATA,+MAGIC_ACCEPT_THREAD_DATA,+MAGIC_SERVER_DATA,+};++structipc_server_reply_data{+enummagicmagic;+intfd;+structipc_worker_thread_data*worker_thread_data;+};++structipc_worker_thread_data{+enummagicmagic;+structipc_worker_thread_data*next_thread;+structipc_server_data*server_data;+pthread_tpthread_id;+structpacket_scratch_spacescratch_write_buffer;+};++structipc_accept_thread_data{+enummagicmagic;+structipc_server_data*server_data;++intfd_listen;+structstatst_listen;++intfd_send_shutdown;+intfd_wait_shutdown;+pthread_tpthread_id;+};++/*+*Withunix-sockets,theconceptual"ipc-server"isimplementedasasingle+*controller"accept-thread"threadandapoolof"worker-thread"threads.+*Theformerdoestheusual`accept()`loopanddispatchesconnections+*toanidleworkerthread.Theworkerthreadswaitinanidleloopfor+*anewconnection,communicatewiththeclientandrelaydatato/from+*the`application_cb`andthenwaitforanotherconnectionfromthe+*serverthread.Thisavoidstheoverheadofconstantlycreatingand+*destroyingthreads.+*/+structipc_server_data{+enummagicmagic;+ipc_server_application_cb*application_cb;+void*application_data;+structstrbufbuf_path;++structipc_accept_thread_data*accept_thread;+structipc_worker_thread_data*worker_thread_list;++pthread_mutex_twork_available_mutex;+pthread_cond_twork_available_cond;++/*+*Acceptedbutnotyetprocessedclientconnectionsarekept+*inacircularbufferFIFO.Thequeueisemptywhenthe+*positionsareequal.+*/+int*fifo_fds;+intqueue_size;+intback_pos;+intfront_pos;++intshutdown_requested;+intis_stopped;+};++/*+*Removeandreturntheoldestqueuedconnection.+*+*Returns-1ifempty.+*/+staticintfifo_dequeue(structipc_server_data*server_data)+{+/* ASSERT holding mutex */++intfd;++if(server_data->back_pos==server_data->front_pos)+return-1;++fd=server_data->fifo_fds[server_data->front_pos];+server_data->fifo_fds[server_data->front_pos]=-1;++server_data->front_pos++;+if(server_data->front_pos==server_data->queue_size)+server_data->front_pos=0;++returnfd;+}++/*+*Pushanewfdontothebackofthequeue.+*+*Dropitandreturn-1ifqueueisalreadyfull.+*/+staticintfifo_enqueue(structipc_server_data*server_data,intfd)+{+/* ASSERT holding mutex */++intnext_back_pos;++next_back_pos=server_data->back_pos+1;+if(next_back_pos==server_data->queue_size)+next_back_pos=0;++if(next_back_pos==server_data->front_pos){+/* Queue is full. Just drop it. */+close(fd);+return-1;+}++server_data->fifo_fds[server_data->back_pos]=fd;+server_data->back_pos=next_back_pos;++returnfd;+}++/*+*WaitforaconnectiontobequeuedtotheFIFOandreturnit.+*+*Returns-1ifsomeonehasalreadyrequestedashutdown.+*/+staticintworker_thread__wait_for_connection(+structipc_worker_thread_data*worker_thread_data)+{+/* ASSERT NOT holding mutex */++structipc_server_data*server_data=worker_thread_data->server_data;+intfd=-1;++pthread_mutex_lock(&server_data->work_available_mutex);+for(;;){+if(server_data->shutdown_requested)+break;++fd=fifo_dequeue(server_data);+if(fd>=0)+break;++pthread_cond_wait(&server_data->work_available_cond,+&server_data->work_available_mutex);+}+pthread_mutex_unlock(&server_data->work_available_mutex);++returnfd;+}++/*+*Forwarddeclareourreplycallbackfunctionsothatanycompiler+*errorsarereportedwhenweactuallydefinethefunction(inaddition+*toanyerrorsreportedwhenwetrytopassthiscallbackfunctionas+*aparameterinafunctioncall).Theformerareeasiertounderstand.+*/+staticipc_server_reply_cbdo_io_reply_callback;++/*+*Relayapplication'sresponsemessagetotheclientprocess.+*(Wedonotflushatthispointbecauseweallowthecaller+*tochunkdatatotheclientthruus.)+*/+staticintdo_io_reply_callback(structipc_server_reply_data*reply_data,+constchar*response,size_tresponse_len)+{+structpacket_scratch_space*scratch=+&reply_data->worker_thread_data->scratch_write_buffer;++if(reply_data->magic!=MAGIC_SERVER_REPLY_DATA)+BUG("reply_cb called with wrong instance data");++returnwrite_packetized_from_buf2(response,response_len,+reply_data->fd,0,scratch);+}++/* A randomly chosen value. */+#define MY_WAIT_POLL_TIMEOUT_MS (10)++/*+*Iftheclienthangsupwithoutsendinganydataonthewire,just+*quietlyclosethesocketandignorethisclient.+*+*ThisworkerthreadiscommittedtoreadingtheIPCrequestdata+*fromtheclientattheotherendofthisfd.Waithereforthe+*clienttoactuallyputsomethingonthewire--becauseifthe+*clientjustdoesaping(connectandhangupwithoutsendingany+*data),ouruseofthepkt-linereadroutineswillspewanerror+*message.+*+*Return-1iftheclienthungup.+*Return0ifdata(possiblyincomplete)isready.+*/+staticintworker_thread__wait_for_io_start(+structipc_worker_thread_data*worker_thread_data,+intfd)+{+structipc_server_data*server_data=worker_thread_data->server_data;+structpollfdpollfd[1];+intresult;++for(;;){+pollfd[0].fd=fd;+pollfd[0].events=POLLIN;++result=poll(pollfd,1,MY_WAIT_POLL_TIMEOUT_MS);+if(result<0){+if(errno==EINTR)+continue;+gotocleanup;+}++if(result==0){+/* a timeout */++intin_shutdown;++pthread_mutex_lock(&server_data->work_available_mutex);+in_shutdown=server_data->shutdown_requested;+pthread_mutex_unlock(&server_data->work_available_mutex);++/*+*Ifashutdownisalreadyinprogressandthis+*clienthasnotstartedtalkingyet,justdropit.+*/+if(in_shutdown)+gotocleanup;+continue;+}++if(pollfd[0].revents&POLLHUP)+gotocleanup;++if(pollfd[0].revents&POLLIN)+return0;++gotocleanup;+}++cleanup:+close(fd);+return-1;+}++/*+*Receivetherequest/commandfromtheclientandpassittothe+*registeredrequest-callback.Therequest-callbackwillcompose+*aresponseandcallourreply-callbacktosendittotheclient.+*/+staticintworker_thread__do_io(+structipc_worker_thread_data*worker_thread_data,+intfd)+{+/* ASSERT NOT holding lock */++structstrbufbuf=STRBUF_INIT;+structipc_server_reply_datareply_data;+intret=0;++reply_data.magic=MAGIC_SERVER_REPLY_DATA;+reply_data.worker_thread_data=worker_thread_data;++reply_data.fd=fd;++ret=read_packetized_to_strbuf(reply_data.fd,&buf,+PACKET_READ_NEVER_DIE);+if(ret>=0){+ret=worker_thread_data->server_data->application_cb(+worker_thread_data->server_data->application_data,+buf.buf,do_io_reply_callback,&reply_data);++packet_flush_gently(reply_data.fd);+}+else{+/*+*Theclientprobablydisconnected/shutdownbeforeit+*couldsendawell-formedmessage.Ignoreit.+*/+}++strbuf_release(&buf);+close(reply_data.fd);++returnret;+}++/*+*BlockSIGPIPEonthecurrentthread(sothatwegetEPIPEfrom+*write()ratherthananactualsignal).+*+*Notethatusingsigchain_push()and_pop()tocontrolSIGPIPE+*aroundourIOcallsisnotthreadsafe:+*[]Itusesaglobalstackofhandlerframes.+*[]ItusesALLOC_GROW()toresizeit.+*[]Finally,accordingtothe`signal(2)`man-page:+*"The effects of `signal()` in a multithreaded process are unspecified."+*/+staticvoidthread_block_sigpipe(sigset_t*old_set)+{+sigset_tnew_set;++sigemptyset(&new_set);+sigaddset(&new_set,SIGPIPE);++sigemptyset(old_set);+pthread_sigmask(SIG_BLOCK,&new_set,old_set);+}++/*+*ThreadprocforanIPCworkerthread.Ithandlesaseriesof+*connectionsfromclients.Itpullsthenextfdfromthequeue+*processesit,andthenwaitsforthenextclient.+*+*BlockSIGPIPEinthisworkerthreadforthelifeofthethread.+*Thisavoidsstray(andsometimesdelayed)SIGPIPEsignalscaused+*byclienterrorsand/orwhenweareunderextremelyheavyIOload.+*+*ThismeansthattheapplicationcallbackwillhaveSIGPIPEblocked.+*Thecallbackshouldnotchangeit.+*/+staticvoid*worker_thread_proc(void*_worker_thread_data)+{+structipc_worker_thread_data*worker_thread_data=_worker_thread_data;+structipc_server_data*server_data=worker_thread_data->server_data;+sigset_told_set;+intfd,io;+intret;++trace2_thread_start("ipc-worker");++thread_block_sigpipe(&old_set);++for(;;){+fd=worker_thread__wait_for_connection(worker_thread_data);+if(fd==-1)+break;/* in shutdown */++io=worker_thread__wait_for_io_start(worker_thread_data,fd);+if(io==-1)+continue;/* client hung up without sending anything */++ret=worker_thread__do_io(worker_thread_data,fd);++if(ret==SIMPLE_IPC_QUIT){+trace2_data_string("ipc-worker",NULL,"queue_stop_async",+"application_quit");+/* The application told us to shutdown. */+ipc_server_stop_async(server_data);+break;+}+}++trace2_thread_exit();+returnNULL;+}++/*+*Return1ifsomeonedeletedorstoletheon-disksocketfromus.+*/+staticintsocket_was_stolen(structipc_accept_thread_data*accept_thread_data)+{+structstatst;+structstat*ref_st=&accept_thread_data->st_listen;++if(lstat(accept_thread_data->server_data->buf_path.buf,&st)==-1)+return1;++if(st.st_ino!=ref_st->st_ino)+return1;++/* We might also consider the creation time on some platforms. */++return0;+}++/* A randomly chosen value. */+#define MY_ACCEPT_POLL_TIMEOUT_MS (60 * 1000)++/*+*Acceptanewclientconnectiononoursocket.Thisusesnon-blocking+*IOsothatwecanalsowaitforshutdownrequestsonoursocket-pair+*withoutactuallyspinningonafasttimeout.+*/+staticintaccept_thread__wait_for_connection(+structipc_accept_thread_data*accept_thread_data)+{+structpollfdpollfd[2];+intresult;++for(;;){+pollfd[0].fd=accept_thread_data->fd_wait_shutdown;+pollfd[0].events=POLLIN;++pollfd[1].fd=accept_thread_data->fd_listen;+pollfd[1].events=POLLIN;++result=poll(pollfd,2,MY_ACCEPT_POLL_TIMEOUT_MS);+if(result<0){+if(errno==EINTR)+continue;+returnresult;+}++if(result==0){+/* a timeout */++/*+*Ifsomeonedeletesorforce-createsanewunix+*domainsocketatoutpath,allfutureclients+*willberoutedelsewhereandwesilentlystarve.+*Ifthathappens,justqueueashutdown.+*/+if(socket_was_stolen(+accept_thread_data)){+trace2_data_string("ipc-accept",NULL,+"queue_stop_async",+"socket_stolen");+ipc_server_stop_async(+accept_thread_data->server_data);+}+continue;+}++if(pollfd[0].revents&POLLIN){+/* shutdown message queued to socketpair */+return-1;+}++if(pollfd[1].revents&POLLIN){+/* a connection is available on fd_listen */++intclient_fd=accept(accept_thread_data->fd_listen,+NULL,NULL);+if(client_fd>=0)+returnclient_fd;++/*+*Anerrorhereisunlikely--itprobably+*indicatesthattheconnectingprocesshas+*alreadydroppedtheconnection.+*/+continue;+}++BUG("unandled poll result errno=%d r[0]=%d r[1]=%d",+errno,pollfd[0].revents,pollfd[1].revents);+}+}++/*+*ThreadprocfortheIPCserver"accept thread".Thiswaitsfor+*anincomingsocketconnection,appendsittothequeueofavailable+*connections,andnotifiesaworkerthreadtoprocessit.+*+*BlockSIGPIPEinthisthreadforthelifeofthethread.This+*avoidsanystraySIGPIPEsignalswhenclosingpipefdsunder+*extremelyheavyloads(suchaswhenthefifoqueueisfullandwe+*dropincommingconnections).+*/+staticvoid*accept_thread_proc(void*_accept_thread_data)+{+structipc_accept_thread_data*accept_thread_data=_accept_thread_data;+structipc_server_data*server_data=accept_thread_data->server_data;+sigset_told_set;++trace2_thread_start("ipc-accept");++thread_block_sigpipe(&old_set);++for(;;){+intclient_fd=accept_thread__wait_for_connection(+accept_thread_data);++pthread_mutex_lock(&server_data->work_available_mutex);+if(server_data->shutdown_requested){+pthread_mutex_unlock(&server_data->work_available_mutex);+if(client_fd>=0)+close(client_fd);+break;+}++if(client_fd<0){+/* ignore transient accept() errors */+}+else{+fifo_enqueue(server_data,client_fd);+pthread_cond_broadcast(&server_data->work_available_cond);+}+pthread_mutex_unlock(&server_data->work_available_mutex);+}++trace2_thread_exit();+returnNULL;+}++/*+*Wecan'tpredicttheconnectionarrivalraterelativetotheworker+*processingrate,thereforeweallowthe"accept-thread"toqueueup+*agenerousnumberofconnections,sincewe'dratherhavetheclient+*notunnecessarilytimeoutifwecanavoidit.(Theassumptionis+*thatthiswillbeusedforFSMonitorandafewsecondwaitona+*connectionisbetterthanhavingtheclienttimeoutanddothefull+*computationitself.)+*+*TheFIFOqueuesizeissettoamultipleoftheworkerpoolsize.+*Thisvaluechosenatrandom.+*/+#define FIFO_SCALE (100)++/*+*Thebacklogvaluefor`listen(2)`.Thisdoesn'tneedtohuge,+*ratherjustlargeenoughforour"accept-thread"towakeupand+*queueincomingconnectionsontotheFIFOwithoutthekernel+*droppingany.+*+*Thisvaluechosenatrandom.+*/+#define LISTEN_BACKLOG (50)++/*+*Createaunixdomainsocketatthegivenpathtolistenfor+*clientconnections.Theresultingsocketwillthenappear+*inthefilesystemasaninodewithS_IFSOCK.Theinodeis+*itselfcreatedaspartofthe`bind(2)`operation.+*+*Theterm"socket"isambiguousinthiscontext.Wewanttoopena+*"socket-fd"thatisboundtoa"socket-inode"(path)ondisk.We+*listenon"socket-fd"fornewconnectionsandclientstryto+*open/connectusingthe"socket-inode"pathname.+*+*Unixdomainsocketshaveafundamentaldesignflawbecausethe+*"socket-inode"persistsuntilthepathnameisdeleted;closingthe+*listening"socket-fd"onlyclosesthesockethandle/descriptor,it+*doesnotdeletetheinode/pathname.+*+*Well-behavingservicedaemonsareexpectedtoalsodeletetheinode+*beforeshutdown.Ifaservicecrashes(orforgets)itcanleave+*the(nowstale)inodeinthefilesystem.Thisbehaveslikeastale+*".lock"fileandmaypreventfutureserviceinstancesfromstarting+*upcorrectly.(Becausetheywon'tbeabletobind.)+*+*Whenfutureserviceinstancestrytocreatethelistenersocket,+*`bind(2)`willfailwithEADDRINUSE--becausetheinodealready+*exists.However,thenewinstancecannottellifitisastale+*inode*or*anotherserviceinstanceisalreadyrunning.+*+*Onepossiblesolutionistoblindlyunlinktheinodebefore+*attemptingtobindanewsocket-fdandthuscreateanew+*socket-inode.Then`bind(2)`shouldalwayssucceed.However,if+*thereisanexistingserviceinstance,itwouldbeorphaned--it+*wouldstillbelisteningonasocket-fdthatisstillboundtoan+*(unlinked)socket-inode,butthatsocket-inodeisnolonger+*associatedwiththepathname.Newclientconnectionswillarrive+*atOURnewsocket-inode--ratherthantheexistingserver's+*socket.(Isupposeitisuptotheexistingservertodetectthat+*itssocket-inodehasbeenstolenandshutdown.)+*+*Anotherpossiblesolutionistotrytousethe".lock"trick,but+*bind()doesnothaveaexclusive-createusebitlikeopen()does,+*sowecannothavemultipleserversfighting/racingtocreatethe+*samefilenamewithouthavingloserslosewithoutknowingthatthey+*lost.+*+*Wetrytoavoidsuchstealingandwouldratherfailtorunthan+*stealanexistingsocket-inode(becauseweassumethatthe+*existingserverhasmorecontextandvaluetotheclientsthana+*freshlystartedserver).However,ifmultipleserversareracing+*tostart,wedon'tcarewhichonewins--noneofthemhaveany+*stateinformationyetworthfightingfor.+*+*Createa"unique"socket-inode(withourPIDinit(andassumethat+*wecanforce-deleteanexistingsocketwiththatname)).Statit+*togettheinodenumberandctime--sothatwecanidentifyitas+*theonewecreated.Thenusetheatomic-renametricktoinstallit+*inthereallocation.(Thiswillunlinkanexistingsocketwith+*thatpathname--andtherebystealtherealsocket-inodefroman+*existingserver.)+*+*Elsewhere,ourthreadwillperiodicallypollthesocket-inodeto+*seeifsomeoneelsestealsours.+*/+staticintcreate_listener_socket(constchar*path,+conststructipc_server_opts*ipc_opts,+structstat*st_socket)+{+structstatst;+structstrbufbuf_uniq=STRBUF_INIT;+intfd_listen;+structunix_stream_listen_optsuslg_opts=UNIX_STREAM_LISTEN_OPTS_INIT;++if(!lstat(path,&st)&&S_ISSOCK(st.st_mode)){+intfd_client;+/*+*Asocket-inodeat`path`existsondisk,butwe+*don'tknowwhetheritbelongstoanactiveserver+*orifthelastserverdiedwithoutcleaningup.+*+*Pokeitwithatrivialconnectiontotrytofindout.+*/+trace2_data_string("ipc-server",NULL,"try-detect-server",+path);+fd_client=unix_stream_connect(path);+if(fd_client>=0){+close(fd_client);+errno=EADDRINUSE;+returnerror_errno(_("socket already in use '%s'"),+path);+}+}++/*+*Createpathnametoour"unique"socketandsetitupfor+*business.+*/+strbuf_addf(&buf_uniq,"%s.%d",path,getpid());++uslg_opts.listen_backlog_size=LISTEN_BACKLOG;+uslg_opts.force_unlink_before_bind=1;+uslg_opts.disallow_chdir=ipc_opts->uds_disallow_chdir;+fd_listen=unix_stream_listen(buf_uniq.buf,&uslg_opts);+if(fd_listen<0){+intsaved_errno=errno;+error_errno(_("could not create listener socket '%s'"),+buf_uniq.buf);+strbuf_release(&buf_uniq);+errno=saved_errno;+return-1;+}++if(lstat(buf_uniq.buf,st_socket)){+intsaved_errno=errno;+error_errno(_("could not stat listener socket '%s'"),+buf_uniq.buf);+close(fd_listen);+unlink(buf_uniq.buf);+strbuf_release(&buf_uniq);+errno=saved_errno;+return-1;+}++if(set_socket_blocking_flag(fd_listen,1)){+intsaved_errno=errno;+error_errno(_("could not set listener socket nonblocking '%s'"),+buf_uniq.buf);+close(fd_listen);+unlink(buf_uniq.buf);+strbuf_release(&buf_uniq);+errno=saved_errno;+return-1;+}++/*+*Installitasthe"real"socketsothatclientswillstarting+*connectingtooursocket.+*/+if(rename(buf_uniq.buf,path)){+intsaved_errno=errno;+error_errno(_("could not create listener socket '%s'"),path);+close(fd_listen);+unlink(buf_uniq.buf);+strbuf_release(&buf_uniq);+errno=saved_errno;+return-1;+}++strbuf_release(&buf_uniq);+trace2_data_string("ipc-server",NULL,"try-listen",path);+returnfd_listen;+}++staticintsetup_listener_socket(constchar*path,structstat*st_socket,+conststructipc_server_opts*ipc_opts)+{+intfd_listen;++trace2_region_enter("ipc-server","create-listener_socket",NULL);+fd_listen=create_listener_socket(path,ipc_opts,st_socket);+trace2_region_leave("ipc-server","create-listener_socket",NULL);++returnfd_listen;+}++/*+*StartIPCserverinapoolofbackgroundthreads.+*/+intipc_server_run_async(structipc_server_data**returned_server_data,+constchar*path,conststructipc_server_opts*opts,+ipc_server_application_cb*application_cb,+void*application_data)+{+structipc_server_data*server_data;+intfd_listen;+structstatst_listen;+intsv[2];+intk;+intnr_threads=opts->nr_threads;++*returned_server_data=NULL;++/*+*Createasocketpairandsetsv[1]tonon-blocking.This+*willusedtosendashutdownmessagetotheaccept-thread+*andallowstheaccept-threadtowaitonEITHERaclient+*connectionorashutdownrequestwithoutspinning.+*/+if(socketpair(AF_UNIX,SOCK_STREAM,0,sv)<0)+returnerror_errno(_("could not create socketpair for '%s'"),+path);++if(set_socket_blocking_flag(sv[1],1)){+intsaved_errno=errno;+close(sv[0]);+close(sv[1]);+errno=saved_errno;+returnerror_errno(_("making socketpair nonblocking '%s'"),+path);+}++fd_listen=setup_listener_socket(path,&st_listen,opts);+if(fd_listen<0){+intsaved_errno=errno;+close(sv[0]);+close(sv[1]);+errno=saved_errno;+return-1;+}++server_data=xcalloc(1,sizeof(*server_data));+server_data->magic=MAGIC_SERVER_DATA;+server_data->application_cb=application_cb;+server_data->application_data=application_data;+strbuf_init(&server_data->buf_path,0);+strbuf_addstr(&server_data->buf_path,path);++if(nr_threads<1)+nr_threads=1;++pthread_mutex_init(&server_data->work_available_mutex,NULL);+pthread_cond_init(&server_data->work_available_cond,NULL);++server_data->queue_size=nr_threads*FIFO_SCALE;+server_data->fifo_fds=xcalloc(server_data->queue_size,+sizeof(*server_data->fifo_fds));++server_data->accept_thread=+xcalloc(1,sizeof(*server_data->accept_thread));+server_data->accept_thread->magic=MAGIC_ACCEPT_THREAD_DATA;+server_data->accept_thread->server_data=server_data;+server_data->accept_thread->fd_listen=fd_listen;+server_data->accept_thread->st_listen=st_listen;+server_data->accept_thread->fd_send_shutdown=sv[0];+server_data->accept_thread->fd_wait_shutdown=sv[1];++if(pthread_create(&server_data->accept_thread->pthread_id,NULL,+accept_thread_proc,server_data->accept_thread))+die_errno(_("could not start accept_thread '%s'"),path);++for(k=0;k<nr_threads;k++){+structipc_worker_thread_data*wtd;++wtd=xcalloc(1,sizeof(*wtd));+wtd->magic=MAGIC_WORKER_THREAD_DATA;+wtd->server_data=server_data;++if(pthread_create(&wtd->pthread_id,NULL,worker_thread_proc,+wtd)){+if(k==0)+die(_("could not start worker[0] for '%s'"),+path);+/*+*Limpalongwiththethreadpoolthatwehave.+*/+break;+}++wtd->next_thread=server_data->worker_thread_list;+server_data->worker_thread_list=wtd;+}++*returned_server_data=server_data;+return0;+}++/*+*GentlytelltheIPCservertreadstoshutdown.+*Canberunonanythread.+*/+intipc_server_stop_async(structipc_server_data*server_data)+{+/* ASSERT NOT holding mutex */++intfd;++if(!server_data)+return0;++trace2_region_enter("ipc-server","server-stop-async",NULL);++pthread_mutex_lock(&server_data->work_available_mutex);++server_data->shutdown_requested=1;++/*+*Writeabytetotheshutdownsocketpairtowakeupthe+*accept-thread.+*/+if(write(server_data->accept_thread->fd_send_shutdown,"Q",1)<0)+error_errno("could not write to fd_send_shutdown");++/*+*Drainthequeueofexistingconnections.+*/+while((fd=fifo_dequeue(server_data))!=-1)+close(fd);++/*+*Gentlytellworkerthreadstostopprocessingnewconnections+*andexit.(Thisdoesnotabortin-processconversations.)+*/+pthread_cond_broadcast(&server_data->work_available_cond);++pthread_mutex_unlock(&server_data->work_available_mutex);++trace2_region_leave("ipc-server","server-stop-async",NULL);++return0;+}++/*+*WaitforallIPCserverthreadstostop.+*/+intipc_server_await(structipc_server_data*server_data)+{+pthread_join(server_data->accept_thread->pthread_id,NULL);++if(!server_data->shutdown_requested)+BUG("ipc-server: accept-thread stopped for '%s'",+server_data->buf_path.buf);++while(server_data->worker_thread_list){+structipc_worker_thread_data*wtd=+server_data->worker_thread_list;++pthread_join(wtd->pthread_id,NULL);++server_data->worker_thread_list=wtd->next_thread;+free(wtd);+}++server_data->is_stopped=1;++return0;+}++voidipc_server_free(structipc_server_data*server_data)+{+structipc_accept_thread_data*accept_thread_data;++if(!server_data)+return;++if(!server_data->is_stopped)+BUG("cannot free ipc-server while running for '%s'",+server_data->buf_path.buf);++accept_thread_data=server_data->accept_thread;+if(accept_thread_data){+if(accept_thread_data->fd_listen!=-1){+/*+*Onlyunlinktheunixdomainsocketifwe+*createdit.Thatis,ifanotherdaemon+*processforce-createdanewsocketatthis+*path,andeffectivelystealsourpath+*(whichpreventsusfromreceivingany+*futureclients),wedon'twanttodothe+*samethingtothem.+*/+if(!socket_was_stolen(+accept_thread_data))+unlink(server_data->buf_path.buf);++close(accept_thread_data->fd_listen);+}+if(accept_thread_data->fd_send_shutdown!=-1)+close(accept_thread_data->fd_send_shutdown);+if(accept_thread_data->fd_wait_shutdown!=-1)+close(accept_thread_data->fd_wait_shutdown);++free(server_data->accept_thread);+}++while(server_data->worker_thread_list){+structipc_worker_thread_data*wtd=+server_data->worker_thread_list;++server_data->worker_thread_list=wtd->next_thread;+free(wtd);+}++pthread_cond_destroy(&server_data->work_available_cond);+pthread_mutex_destroy(&server_data->work_available_mutex);++strbuf_release(&server_data->buf_path);++free(server_data->fifo_fds);+free(server_data);+}
@@ -0,0 +1,751 @@+#include"cache.h"+#include"simple-ipc.h"+#include"strbuf.h"+#include"pkt-line.h"+#include"thread-utils.h"++#ifndef GIT_WINDOWS_NATIVE+#error This file can only be compiled on Windows+#endif++staticintinitialize_pipe_name(constchar*path,wchar_t*wpath,size_talloc)+{+intoff=0;+structstrbufrealpath=STRBUF_INIT;++if(!strbuf_realpath(&realpath,path,0))+return-1;++off=swprintf(wpath,alloc,L"\\\\.\\pipe\\");+if(xutftowcs(wpath+off,realpath.buf,alloc-off)<0)+return-1;++/* Handle drive prefix */+if(wpath[off]&&wpath[off+1]==L':'){+wpath[off+1]=L'_';+off+=2;+}++for(;wpath[off];off++)+if(wpath[off]==L'/')+wpath[off]=L'\\';++strbuf_release(&realpath);+return0;+}++staticenumipc_active_stateget_active_state(wchar_t*pipe_path)+{+if(WaitNamedPipeW(pipe_path,NMPWAIT_USE_DEFAULT_WAIT))+returnIPC_STATE__LISTENING;++if(GetLastError()==ERROR_SEM_TIMEOUT)+returnIPC_STATE__NOT_LISTENING;++if(GetLastError()==ERROR_FILE_NOT_FOUND)+returnIPC_STATE__PATH_NOT_FOUND;++returnIPC_STATE__OTHER_ERROR;+}++enumipc_active_stateipc_get_active_state(constchar*path)+{+wchar_tpipe_path[MAX_PATH];++if(initialize_pipe_name(path,pipe_path,ARRAY_SIZE(pipe_path))<0)+returnIPC_STATE__INVALID_PATH;++returnget_active_state(pipe_path);+}++#define WAIT_STEP_MS (50)++staticenumipc_active_stateconnect_to_server(+constwchar_t*wpath,+DWORDtimeout_ms,+conststructipc_client_connect_options*options,+int*pfd)+{+DWORDt_start_ms,t_waited_ms;+DWORDstep_ms;+HANDLEhPipe=INVALID_HANDLE_VALUE;+DWORDmode=PIPE_READMODE_BYTE;+DWORDgle;++*pfd=-1;++for(;;){+hPipe=CreateFileW(wpath,GENERIC_READ|GENERIC_WRITE,+0,NULL,OPEN_EXISTING,0,NULL);+if(hPipe!=INVALID_HANDLE_VALUE)+break;++gle=GetLastError();++switch(gle){+caseERROR_FILE_NOT_FOUND:+if(!options->wait_if_not_found)+returnIPC_STATE__PATH_NOT_FOUND;+if(!timeout_ms)+returnIPC_STATE__PATH_NOT_FOUND;++step_ms=(timeout_ms<WAIT_STEP_MS)?+timeout_ms:WAIT_STEP_MS;+sleep_millisec(step_ms);++timeout_ms-=step_ms;+break;/* try again */++caseERROR_PIPE_BUSY:+if(!options->wait_if_busy)+returnIPC_STATE__NOT_LISTENING;+if(!timeout_ms)+returnIPC_STATE__NOT_LISTENING;++t_start_ms=(DWORD)(getnanotime()/1000000);++if(!WaitNamedPipeW(wpath,timeout_ms)){+if(GetLastError()==ERROR_SEM_TIMEOUT)+returnIPC_STATE__NOT_LISTENING;++returnIPC_STATE__OTHER_ERROR;+}++/*+*Apipeserverinstancebecameavailable.+*Raceotherclientprocessestoconnectto+*it.+*+*Butfirstdecrementouroveralltimeoutso+*thatwedon'tstarveifwekeeplosingthe+*race.Butalsoguardagainstspecial+*NPMWAIT_values(0and-1).+*/+t_waited_ms=(DWORD)(getnanotime()/1000000)-t_start_ms;+if(t_waited_ms<timeout_ms)+timeout_ms-=t_waited_ms;+else+timeout_ms=1;+break;/* try again */++default:+returnIPC_STATE__OTHER_ERROR;+}+}++if(!SetNamedPipeHandleState(hPipe,&mode,NULL,NULL)){+CloseHandle(hPipe);+returnIPC_STATE__OTHER_ERROR;+}++*pfd=_open_osfhandle((intptr_t)hPipe,O_RDWR|O_BINARY);+if(*pfd<0){+CloseHandle(hPipe);+returnIPC_STATE__OTHER_ERROR;+}++/* fd now owns hPipe */++returnIPC_STATE__LISTENING;+}++/*+*ThedefaultconnectiontimeoutforWindowsclients.+*+*Thisisnotcurrentlypartoftheipc_API(northeconfigsettings)+*becauseofdifferencesbetweenWindowsandotherplatforms.+*+*Thisvaluewaschosenatrandom.+*/+#define WINDOWS_CONNECTION_TIMEOUT_MS (30000)++enumipc_active_stateipc_client_try_connect(+constchar*path,+conststructipc_client_connect_options*options,+structipc_client_connection**p_connection)+{+wchar_twpath[MAX_PATH];+enumipc_active_statestate=IPC_STATE__OTHER_ERROR;+intfd=-1;++*p_connection=NULL;++trace2_region_enter("ipc-client","try-connect",NULL);+trace2_data_string("ipc-client",NULL,"try-connect/path",path);++if(initialize_pipe_name(path,wpath,ARRAY_SIZE(wpath))<0)+state=IPC_STATE__INVALID_PATH;+else+state=connect_to_server(wpath,WINDOWS_CONNECTION_TIMEOUT_MS,+options,&fd);++trace2_data_intmax("ipc-client",NULL,"try-connect/state",+(intmax_t)state);+trace2_region_leave("ipc-client","try-connect",NULL);++if(state==IPC_STATE__LISTENING){+(*p_connection)=xcalloc(1,sizeof(structipc_client_connection));+(*p_connection)->fd=fd;+}++returnstate;+}++voidipc_client_close_connection(structipc_client_connection*connection)+{+if(!connection)+return;++if(connection->fd!=-1)+close(connection->fd);++free(connection);+}++intipc_client_send_command_to_connection(+structipc_client_connection*connection,+constchar*message,structstrbuf*answer)+{+intret=0;++strbuf_setlen(answer,0);++trace2_region_enter("ipc-client","send-command",NULL);++if(write_packetized_from_buf2(message,strlen(message),+connection->fd,1,+&connection->scratch_write_buffer)<0){+ret=error(_("could not send IPC command"));+gotodone;+}++FlushFileBuffers((HANDLE)_get_osfhandle(connection->fd));++if(read_packetized_to_strbuf(connection->fd,answer,+PACKET_READ_NEVER_DIE)<0){+ret=error(_("could not read IPC response"));+gotodone;+}++done:+trace2_region_leave("ipc-client","send-command",NULL);+returnret;+}++intipc_client_send_command(constchar*path,+conststructipc_client_connect_options*options,+constchar*message,structstrbuf*response)+{+intret=-1;+enumipc_active_statestate;+structipc_client_connection*connection=NULL;++state=ipc_client_try_connect(path,options,&connection);++if(state!=IPC_STATE__LISTENING)+returnret;++ret=ipc_client_send_command_to_connection(connection,message,response);++ipc_client_close_connection(connection);++returnret;+}++/*+*Duplicatethegivenpipehandleandwrapitinafiledescriptorso+*thatwecanusepkt-lineonit.+*/+staticintdup_fd_from_pipe(constHANDLEpipe)+{+HANDLEprocess=GetCurrentProcess();+HANDLEhandle;+intfd;++if(!DuplicateHandle(process,pipe,process,&handle,0,FALSE,+DUPLICATE_SAME_ACCESS)){+errno=err_win_to_posix(GetLastError());+return-1;+}++fd=_open_osfhandle((intptr_t)handle,O_RDWR|O_BINARY);+if(fd<0){+errno=err_win_to_posix(GetLastError());+CloseHandle(handle);+return-1;+}++/*+*`handle`isnowownedby`fd`andwillbeautomaticallyclosed+*whenthedescriptorisclosed.+*/++returnfd;+}++/*+*Magicnumbersusedtoannotatecallbackinstancedata.+*Theseareusedtohelpguardagainstaccidentallypassingthe+*wronginstancedataacrossmultiplelevelsofcallbacks(which+*iseasytodoifthereare`void*`arguments).+*/+enummagic{+MAGIC_SERVER_REPLY_DATA,+MAGIC_SERVER_THREAD_DATA,+MAGIC_SERVER_DATA,+};++structipc_server_reply_data{+enummagicmagic;+intfd;+structipc_server_thread_data*server_thread_data;+};++structipc_server_thread_data{+enummagicmagic;+structipc_server_thread_data*next_thread;+structipc_server_data*server_data;+pthread_tpthread_id;+HANDLEhPipe;+structpacket_scratch_spacescratch_write_buffer;+};++/*+*OnWindows,theconceptual"ipc-server"isimplementedasapoolof+*nidential/peer"server-thread"threads.Thatis,thereisno+*hierarchyofthreads;andthereforenocontrollerthreadmanaging+*thepool.Eachthreadhasanindependenthandletothenamedpipe,+*receivesincomingconnections,processestheclient,andre-uses+*thepipeforthenextclientconnection.+*+*Therefore,the"ipc-server"onlyneedstomaintainalistofthe+*spawnedthreadsforeventual"join"purposes.+*+*Asingle"stop-event"isvisibletoalloftheserverthreadsto+*tellthemtoshutdown(whenidle).+*/+structipc_server_data{+enummagicmagic;+ipc_server_application_cb*application_cb;+void*application_data;+structstrbufbuf_path;+wchar_twpath[MAX_PATH];++HANDLEhEventStopRequested;+structipc_server_thread_data*thread_list;+intis_stopped;+};++enumconnect_result{+CR_CONNECTED=0,+CR_CONNECT_PENDING,+CR_CONNECT_ERROR,+CR_WAIT_ERROR,+CR_SHUTDOWN,+};++staticenumconnect_resultqueue_overlapped_connect(+structipc_server_thread_data*server_thread_data,+OVERLAPPED*lpo)+{+if(ConnectNamedPipe(server_thread_data->hPipe,lpo))+gotofailed;++switch(GetLastError()){+caseERROR_IO_PENDING:+returnCR_CONNECT_PENDING;++caseERROR_PIPE_CONNECTED:+SetEvent(lpo->hEvent);+returnCR_CONNECTED;++default:+break;+}++failed:+error(_("ConnectNamedPipe failed for '%s' (%lu)"),+server_thread_data->server_data->buf_path.buf,+GetLastError());+returnCR_CONNECT_ERROR;+}++/*+*UseWindowsOverlappedIOtowaitforaconnectionorforourevent+*tobesignalled.+*/+staticenumconnect_resultwait_for_connection(+structipc_server_thread_data*server_thread_data,+OVERLAPPED*lpo)+{+enumconnect_resultr;+HANDLEwaitHandles[2];+DWORDdwWaitResult;++r=queue_overlapped_connect(server_thread_data,lpo);+if(r!=CR_CONNECT_PENDING)+returnr;++waitHandles[0]=server_thread_data->server_data->hEventStopRequested;+waitHandles[1]=lpo->hEvent;++dwWaitResult=WaitForMultipleObjects(2,waitHandles,FALSE,INFINITE);+switch(dwWaitResult){+caseWAIT_OBJECT_0+0:+returnCR_SHUTDOWN;++caseWAIT_OBJECT_0+1:+ResetEvent(lpo->hEvent);+returnCR_CONNECTED;++default:+returnCR_WAIT_ERROR;+}+}++/*+*Forwarddeclareourreplycallbackfunctionsothatanycompiler+*errorsarereportedwhenweactuallydefinethefunction(inaddition+*toanyerrorsreportedwhenwetrytopassthiscallbackfunctionas+*aparameterinafunctioncall).Theformerareeasiertounderstand.+*/+staticipc_server_reply_cbdo_io_reply_callback;++/*+*Relayapplication'sresponsemessagetotheclientprocess.+*(Wedonotflushatthispointbecauseweallowthecaller+*tochunkdatatotheclientthruus.)+*/+staticintdo_io_reply_callback(structipc_server_reply_data*reply_data,+constchar*response,size_tresponse_len)+{+structpacket_scratch_space*scratch=+&reply_data->server_thread_data->scratch_write_buffer;++if(reply_data->magic!=MAGIC_SERVER_REPLY_DATA)+BUG("reply_cb called with wrong instance data");++returnwrite_packetized_from_buf2(response,response_len,+reply_data->fd,0,scratch);+}++/*+*Receivetherequest/commandfromtheclientandpassittothe+*registeredrequest-callback.Therequest-callbackwillcompose+*aresponseandcallourreply-callbacktosendittotheclient.+*+*Simple-IPConlycontainsoneroundtrip,soweflushandclose+*hereaftertheresponse.+*/+staticintdo_io(structipc_server_thread_data*server_thread_data)+{+structstrbufbuf=STRBUF_INIT;+structipc_server_reply_datareply_data;+intret=0;++reply_data.magic=MAGIC_SERVER_REPLY_DATA;+reply_data.server_thread_data=server_thread_data;++reply_data.fd=dup_fd_from_pipe(server_thread_data->hPipe);+if(reply_data.fd<0)+returnerror(_("could not create fd from pipe for '%s'"),+server_thread_data->server_data->buf_path.buf);++ret=read_packetized_to_strbuf(reply_data.fd,&buf,+PACKET_READ_NEVER_DIE);+if(ret>=0){+ret=server_thread_data->server_data->application_cb(+server_thread_data->server_data->application_data,+buf.buf,do_io_reply_callback,&reply_data);++packet_flush_gently(reply_data.fd);++FlushFileBuffers((HANDLE)_get_osfhandle((reply_data.fd)));+}+else{+/*+*Theclientprobablydisconnected/shutdownbeforeit+*couldsendawell-formedmessage.Ignoreit.+*/+}++strbuf_release(&buf);+close(reply_data.fd);++returnret;+}++/*+*HandleIPCrequestandresponsewiththisconnectedclient.Andreset+*thepipetoprepareforthenextclient.+*/+staticintuse_connection(structipc_server_thread_data*server_thread_data)+{+intret;++ret=do_io(server_thread_data);++FlushFileBuffers(server_thread_data->hPipe);+DisconnectNamedPipe(server_thread_data->hPipe);++returnret;+}++/*+*ThreadprocforanIPCserverworkerthread.Ithandlesaseriesof+*connectionsfromclients.ItcleansandreusesthehPipebetweeneach+*client.+*/+staticvoid*server_thread_proc(void*_server_thread_data)+{+structipc_server_thread_data*server_thread_data=_server_thread_data;+HANDLEhEventConnected=INVALID_HANDLE_VALUE;+OVERLAPPEDoConnect;+enumconnect_resultcr;+intret;++assert(server_thread_data->hPipe!=INVALID_HANDLE_VALUE);++trace2_thread_start("ipc-server");+trace2_data_string("ipc-server",NULL,"pipe",+server_thread_data->server_data->buf_path.buf);++hEventConnected=CreateEventW(NULL,TRUE,FALSE,NULL);++memset(&oConnect,0,sizeof(oConnect));+oConnect.hEvent=hEventConnected;++for(;;){+cr=wait_for_connection(server_thread_data,&oConnect);++switch(cr){+caseCR_SHUTDOWN:+gotofinished;++caseCR_CONNECTED:+ret=use_connection(server_thread_data);+if(ret==SIMPLE_IPC_QUIT){+ipc_server_stop_async(+server_thread_data->server_data);+gotofinished;+}+if(ret>0){+/*+*Ignore(transient)IOerrorswiththis+*clientandresetforthenextclient.+*/+}+break;++caseCR_CONNECT_PENDING:+/* By construction, this should not happen. */+BUG("ipc-server[%s]: unexpeced CR_CONNECT_PENDING",+server_thread_data->server_data->buf_path.buf);++caseCR_CONNECT_ERROR:+caseCR_WAIT_ERROR:+/*+*Ignorethesetheoreticalerrors.+*/+DisconnectNamedPipe(server_thread_data->hPipe);+break;++default:+BUG("unandled case after wait_for_connection");+}+}++finished:+CloseHandle(server_thread_data->hPipe);+CloseHandle(hEventConnected);++trace2_thread_exit();+returnNULL;+}++staticHANDLEcreate_new_pipe(wchar_t*wpath,intis_first)+{+HANDLEhPipe;+DWORDdwOpenMode,dwPipeMode;+LPSECURITY_ATTRIBUTESlpsa=NULL;++dwOpenMode=PIPE_ACCESS_INBOUND|PIPE_ACCESS_OUTBOUND|+FILE_FLAG_OVERLAPPED;++dwPipeMode=PIPE_TYPE_MESSAGE|PIPE_READMODE_BYTE|PIPE_WAIT|+PIPE_REJECT_REMOTE_CLIENTS;++if(is_first){+dwOpenMode|=FILE_FLAG_FIRST_PIPE_INSTANCE;++/*+*OnWindows,thefirstserverpipeinstancegetsto+*settheACL/SecurityAttributesonthenamed+*pipe;subsequentinstancesinheritandcannot+*changethem.+*+*TODOShouldweallowtheapplicationlayerto+*specifysecurityattributes,suchas`LocalService`+*or`LocalSystem`,whenwecreatethenamedpipe?+*Thisquestionisprobablynotimportantwhenthe+*daemonisstartedbyaforegrounduserprocessand+*onlyneedstotalktothecurrentuser,butmaybe+*ifthedaemonisrunviatheControlPanelasa+*SystemService.+*/+}++hPipe=CreateNamedPipeW(wpath,dwOpenMode,dwPipeMode,+PIPE_UNLIMITED_INSTANCES,1024,1024,0,lpsa);++returnhPipe;+}++intipc_server_run_async(structipc_server_data**returned_server_data,+constchar*path,conststructipc_server_opts*opts,+ipc_server_application_cb*application_cb,+void*application_data)+{+structipc_server_data*server_data;+wchar_twpath[MAX_PATH];+HANDLEhPipeFirst=INVALID_HANDLE_VALUE;+intk;+intret=0;+intnr_threads=opts->nr_threads;++*returned_server_data=NULL;++ret=initialize_pipe_name(path,wpath,ARRAY_SIZE(wpath));+if(ret<0)+returnerror(+_("could not create normalized wchar_t path for '%s'"),+path);++hPipeFirst=create_new_pipe(wpath,1);+if(hPipeFirst==INVALID_HANDLE_VALUE)+returnerror(_("IPC server already running on '%s'"),path);++server_data=xcalloc(1,sizeof(*server_data));+server_data->magic=MAGIC_SERVER_DATA;+server_data->application_cb=application_cb;+server_data->application_data=application_data;+server_data->hEventStopRequested=CreateEvent(NULL,TRUE,FALSE,NULL);+strbuf_init(&server_data->buf_path,0);+strbuf_addstr(&server_data->buf_path,path);+wcscpy(server_data->wpath,wpath);++if(nr_threads<1)+nr_threads=1;++for(k=0;k<nr_threads;k++){+structipc_server_thread_data*std;++std=xcalloc(1,sizeof(*std));+std->magic=MAGIC_SERVER_THREAD_DATA;+std->server_data=server_data;+std->hPipe=INVALID_HANDLE_VALUE;++std->hPipe=(k==0)+?hPipeFirst+:create_new_pipe(server_data->wpath,0);++if(std->hPipe==INVALID_HANDLE_VALUE){+/*+*Ifwe'vereachedapipeinstancelimitfor+*thispath,justusefewerthreads.+*/+free(std);+break;+}++if(pthread_create(&std->pthread_id,NULL,+server_thread_proc,std)){+/*+*Likewise,ifwe'reoutofthreads,justuse+*fewerthreadsthanrequested.+*+*However,wejustgiveupifwecan'tevenget+*onethread.Thisshouldnothappen.+*/+if(k==0)+die(_("could not start thread[0] for '%s'"),+path);++CloseHandle(std->hPipe);+free(std);+break;+}++std->next_thread=server_data->thread_list;+server_data->thread_list=std;+}++*returned_server_data=server_data;+return0;+}++intipc_server_stop_async(structipc_server_data*server_data)+{+if(!server_data)+return0;++/*+*Gentlytellalloftheipc_serverthreadstoshutdown.+*Thiswillbeseenthenexttimetheyareidle(andwaiting+*foraconnection).+*+*WeDONOTattempttoforcethemtodropanactiveconnection.+*/+SetEvent(server_data->hEventStopRequested);+return0;+}++intipc_server_await(structipc_server_data*server_data)+{+DWORDdwWaitResult;++if(!server_data)+return0;++dwWaitResult=WaitForSingleObject(server_data->hEventStopRequested,INFINITE);+if(dwWaitResult!=WAIT_OBJECT_0)+returnerror(_("wait for hEvent failed for '%s'"),+server_data->buf_path.buf);++while(server_data->thread_list){+structipc_server_thread_data*std=server_data->thread_list;++pthread_join(std->pthread_id,NULL);++server_data->thread_list=std->next_thread;+free(std);+}++server_data->is_stopped=1;++return0;+}++voidipc_server_free(structipc_server_data*server_data)+{+if(!server_data)+return;++if(!server_data->is_stopped)+BUG("cannot free ipc-server while running for '%s'",+server_data->buf_path.buf);++strbuf_release(&server_data->buf_path);++if(server_data->hEventStopRequested!=INVALID_HANDLE_VALUE)+CloseHandle(server_data->hEventStopRequested);++while(server_data->thread_list){+structipc_server_thread_data*std=server_data->thread_list;++server_data->thread_list=std->next_thread;+free(std);+}++free(server_data);+}
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:49:21
From: Jeff Hostetler <redacted>
The static helper function `unix_stream_socket()` calls `die()`. This is not
appropriate for all callers. Eliminate the wrapper function and move the
existing error handling to the callers in preparation for adapting specific
callers.
Signed-off-by: Jeff Hostetler <redacted>
---
unix-socket.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
From: Jeff King <hidden> Date: 2021-02-02 09:55:47
On Mon, Feb 01, 2021 at 07:45:43PM +0000, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <redacted>
The static helper function `unix_stream_socket()` calls `die()`. This is not
appropriate for all callers. Eliminate the wrapper function and move the
existing error handling to the callers in preparation for adapting specific
callers.
Thanks, this looks good.
-static int unix_stream_socket(void)
-{
- int fd = socket(AF_UNIX, SOCK_STREAM, 0);
- if (fd < 0)
- die_errno("unable to create socket");
- return fd;
-}
This could become a one-liner:
return socket(AF_UNIX, SOCK_STREAM, 0);
to keep the details abstracted. But it's local to this file, the callers
are already necessarily full of bsd-socket arcana, and it's not like the
magic words there have ever changed in 30+ years. Putting it inline
seems quite reasonable. :)
-Peff
From: Jeff King <hidden> Date: 2021-02-02 09:59:29
On Mon, Feb 01, 2021 at 07:45:43PM +0000, Jeff Hostetler via GitGitGadget wrote:
quoted hunk
static int chdir_len(const char *orig, int len)
{
char *path = xmemdupz(orig, len);
@@ -79,7 +71,10 @@ int unix_stream_connect(const char *path) if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1;- fd = unix_stream_socket();+ fd = socket(AF_UNIX, SOCK_STREAM, 0);+ if (fd < 0)+ die_errno("unable to create socket");+
Reading the next patch, I suddenly realized that these are die calls,
and not just passing along the error (which you then fix in the next
patch). It seems like that should be happening here in this patch.
Callers must already be ready to handle an error (we return -1 in the
context above).
quoted hunk
@@ -103,7 +98,9 @@ int unix_stream_listen(const char *path) if (unix_sockaddr_init(&sa, path, &ctx) < 0) return -1;- fd = unix_stream_socket();+ fd = socket(AF_UNIX, SOCK_STREAM, 0);+ if (fd < 0)+ die_errno("unable to create socket");
@@ -0,0 +1,485 @@+/*+*test-simple-ipc.c:verifythattheInter-ProcessCommunicationworks.+*/++#include"test-tool.h"+#include"cache.h"+#include"strbuf.h"+#include"simple-ipc.h"+#include"parse-options.h"+#include"thread-utils.h"++#ifndef SUPPORTS_SIMPLE_IPC+intcmd__simple_ipc(intargc,constchar**argv)+{+die("simple IPC not available on this platform");+}+#else++/*+*Thetestdaemondefinesan"application callback"thatsupportsa+*seriesofcommands(see`test_app_cb()`).+*+*Unknowncommandsarecaughthereandwesendanerrormessageback+*totheclientprocess.+*/+staticintapp__unhandled_command(constchar*command,+ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+intret;++strbuf_addf(&buf,"unhandled command: %s",command);+ret=reply_cb(reply_data,buf.buf,buf.len);+strbuf_release(&buf);++returnret;+}++/*+*Replywithasingleverylargebuffer.Thisistoensurethat+*longresponseareproperlyhandled--whetherthechunkingoccurs+*inthekernelorinthe(probablypkt-line)layer.+*/+#define BIG_ROWS (10000)+staticintapp__big_command(ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+introw;+intret;++for(row=0;row<BIG_ROWS;row++)+strbuf_addf(&buf,"big: %.75d\n",row);++ret=reply_cb(reply_data,buf.buf,buf.len);+strbuf_release(&buf);++returnret;+}++/*+*Replywithaseriesoflines.Thisistoensurethatwecanincrementally+*computetheresponseandchunkittotheclient.+*/+#define CHUNK_ROWS (10000)+staticintapp__chunk_command(ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+introw;+intret;++for(row=0;row<CHUNK_ROWS;row++){+strbuf_setlen(&buf,0);+strbuf_addf(&buf,"big: %.75d\n",row);+ret=reply_cb(reply_data,buf.buf,buf.len);+}++strbuf_release(&buf);++returnret;+}++/*+*Slowlyreplywithaseriesoflines.Thisistomodelanexpensiveto+*computechunkedresponse(whichmighthappenifthiscallbackisrunning+*inathreadandisfightingforalockwithotherthreads).+*/+#define SLOW_ROWS (1000)+#define SLOW_DELAY_MS (10)+staticintapp__slow_command(ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf=STRBUF_INIT;+introw;+intret;++for(row=0;row<SLOW_ROWS;row++){+strbuf_setlen(&buf,0);+strbuf_addf(&buf,"big: %.75d\n",row);+ret=reply_cb(reply_data,buf.buf,buf.len);+sleep_millisec(SLOW_DELAY_MS);+}++strbuf_release(&buf);++returnret;+}++/*+*Theclientsentacommandfollowedbya(possiblyvery)largebuffer.+*/+staticintapp__sendbytes_command(constchar*received,+ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+structstrbufbuf_resp=STRBUF_INIT;+constchar*p="?";+intlen_ballast=0;+intk;+interrs=0;+intret;++if(skip_prefix(received,"sendbytes ",&p))+len_ballast=strlen(p);++/*+*Verifythattheballastisncopiesofasingleletter.+*Andthatthemulti-threadedIOlayerdidn'tcrossthestreams.+*/+for(k=1;k<len_ballast;k++)+if(p[k]!=p[0])+errs++;++if(errs)+strbuf_addf(&buf_resp,"errs:%d\n",errs);+else+strbuf_addf(&buf_resp,"rcvd:%c%08d\n",p[0],len_ballast);++ret=reply_cb(reply_data,buf_resp.buf,buf_resp.len);++strbuf_release(&buf_resp);++returnret;+}++/*+*Anarbitraryfixedaddresstoverifythattheapplicationinstance+*dataishandledproperly.+*/+staticintmy_app_data=42;++staticipc_server_application_cbtest_app_cb;++/*+*Thisis"application callback"thatsitsontopofthe"ipc-server".+*Itcompletelydefinesthesetofcommandverbssupportedbythis+*application.+*/+staticinttest_app_cb(void*application_data,+constchar*command,+ipc_server_reply_cb*reply_cb,+structipc_server_reply_data*reply_data)+{+/*+*Verifythatwereceivedtheapplication-datathatwepassed+*whenwestartedtheipc-server.(Wehaveseverallayersof+*callbackscallingcallbacksandit'seasytogetthingsmixed+*up(especiallywhensomeare"void*").)+*/+if(application_data!=(void*)&my_app_data)+BUG("application_cb: application_data pointer wrong");++if(!strcmp(command,"quit")){+/*+*Tellipc-servertohangupwithanemptyreply.+*/+returnSIMPLE_IPC_QUIT;+}++if(!strcmp(command,"ping")){+constchar*answer="pong";+returnreply_cb(reply_data,answer,strlen(answer));+}++if(!strcmp(command,"big"))+returnapp__big_command(reply_cb,reply_data);++if(!strcmp(command,"chunk"))+returnapp__chunk_command(reply_cb,reply_data);++if(!strcmp(command,"slow"))+returnapp__slow_command(reply_cb,reply_data);++if(starts_with(command,"sendbytes "))+returnapp__sendbytes_command(command,reply_cb,reply_data);++returnapp__unhandled_command(command,reply_cb,reply_data);+}++/*+*Thisprocesswillrunasasimple-ipcserverandlistenforIPCcommands+*fromclientprocesses.+*/+staticintdaemon__run_server(constchar*path,intargc,constchar**argv)+{+structipc_server_optsopts={+.nr_threads=5+};++constchar*constdaemon_usage[]={+N_("test-helper simple-ipc daemon [<options>"),+NULL+};+structoptiondaemon_options[]={+OPT_INTEGER(0,"threads",&opts.nr_threads,+N_("number of threads in server thread pool")),+OPT_END()+};++argc=parse_options(argc,argv,NULL,daemon_options,daemon_usage,0);++if(opts.nr_threads<1)+opts.nr_threads=1;++/*+*Synchronouslyruntheipc-server.Wedon'tneedanyapplication+*instancedata,sopassanarbitrarypointer(thatwe'lllater+*verifymadetheroundtrip).+*/+returnipc_server_run(path,&opts,test_app_cb,(void*)&my_app_data);+}++/*+*Thisprocesswillrunaquickprobetoseeifasimple-ipcserver+*isactiveonthispath.+*+*Returns0iftheserverisalive.+*/+staticintclient__probe_server(constchar*path)+{+enumipc_active_states;++s=ipc_get_active_state(path);+switch(s){+caseIPC_STATE__LISTENING:+return0;++caseIPC_STATE__NOT_LISTENING:+returnerror("no server listening at '%s'",path);++caseIPC_STATE__PATH_NOT_FOUND:+returnerror("path not found '%s'",path);++caseIPC_STATE__INVALID_PATH:+returnerror("invalid pipe/socket name '%s'",path);++caseIPC_STATE__OTHER_ERROR:+default:+returnerror("other error for '%s'",path);+}+}++/*+*SendanIPCcommandtoanalready-runningserverdaemonandprintthe+*response.+*+*argv[2]containsasimple(1word)commandverbthat`test_app_cb()`+*(inthedaemonprocess)willunderstand.+*/+staticintclient__send_ipc(intargc,constchar**argv,constchar*path)+{+constchar*command=argc>2?argv[2]:"(no command)";+structstrbufbuf=STRBUF_INIT;+structipc_client_connect_optionsoptions+=IPC_CLIENT_CONNECT_OPTIONS_INIT;++options.wait_if_busy=1;+options.wait_if_not_found=0;++if(!ipc_client_send_command(path,&options,command,&buf)){+printf("%s\n",buf.buf);+fflush(stdout);+strbuf_release(&buf);++return0;+}++returnerror("failed to send '%s' to '%s'",command,path);+}++/*+*SendanIPCcommandfollowedbyballasttoconfirmthatalarge+*messagecanbesentandthatthekernelorpkt-linelayerswill+*properlychunkitandthatthedaemonreceivestheentiremessage.+*/+staticintdo_sendbytes(intbytecount,charbyte,constchar*path)+{+structstrbufbuf_send=STRBUF_INIT;+structstrbufbuf_resp=STRBUF_INIT;+structipc_client_connect_optionsoptions+=IPC_CLIENT_CONNECT_OPTIONS_INIT;++options.wait_if_busy=1;+options.wait_if_not_found=0;++strbuf_addstr(&buf_send,"sendbytes ");+strbuf_addchars(&buf_send,byte,bytecount);++if(!ipc_client_send_command(path,&options,buf_send.buf,&buf_resp)){+strbuf_rtrim(&buf_resp);+printf("sent:%c%08d %s\n",byte,bytecount,buf_resp.buf);+fflush(stdout);+strbuf_release(&buf_send);+strbuf_release(&buf_resp);++return0;+}++returnerror("client failed to sendbytes(%d, '%c') to '%s'",+bytecount,byte,path);+}++/*+*SendanIPCcommandwithballasttoanalready-runningserverdaemon.+*/+staticintclient__sendbytes(intargc,constchar**argv,constchar*path)+{+intbytecount=1024;+char*string="x";+constchar*constsendbytes_usage[]={+N_("test-helper simple-ipc sendbytes [<options>]"),+NULL+};+structoptionsendbytes_options[]={+OPT_INTEGER(0,"bytecount",&bytecount,N_("number of bytes")),+OPT_STRING(0,"byte",&string,N_("byte"),N_("ballast")),+OPT_END()+};++argc=parse_options(argc,argv,NULL,sendbytes_options,sendbytes_usage,0);++returndo_sendbytes(bytecount,string[0],path);+}++structmultiple_thread_data{+pthread_tpthread_id;+structmultiple_thread_data*next;+constchar*path;+intbytecount;+intbatchsize;+intsum_errors;+intsum_good;+charletter;+};++staticvoid*multiple_thread_proc(void*_multiple_thread_data)+{+structmultiple_thread_data*d=_multiple_thread_data;+intk;++trace2_thread_start("multiple");++for(k=0;k<d->batchsize;k++){+if(do_sendbytes(d->bytecount+k,d->letter,d->path))+d->sum_errors++;+else+d->sum_good++;+}++trace2_thread_exit();+returnNULL;+}++/*+*Startaclient-sidethreadpool.Eachthreadsendsaseriesof+*IPCrequests.Eachrequestisonanewconnectiontotheserver.+*/+staticintclient__multiple(intargc,constchar**argv,constchar*path)+{+structmultiple_thread_data*list=NULL;+intk;+intnr_threads=5;+intbytecount=1;+intbatchsize=10;+intsum_join_errors=0;+intsum_thread_errors=0;+intsum_good=0;++constchar*constmultiple_usage[]={+N_("test-helper simple-ipc multiple [<options>]"),+NULL+};+structoptionmultiple_options[]={+OPT_INTEGER(0,"bytecount",&bytecount,N_("number of bytes")),+OPT_INTEGER(0,"threads",&nr_threads,N_("number of threads")),+OPT_INTEGER(0,"batchsize",&batchsize,N_("number of requests per thread")),+OPT_END()+};++argc=parse_options(argc,argv,NULL,multiple_options,multiple_usage,0);++if(bytecount<1)+bytecount=1;+if(nr_threads<1)+nr_threads=1;+if(batchsize<1)+batchsize=1;++for(k=0;k<nr_threads;k++){+structmultiple_thread_data*d=xcalloc(1,sizeof(*d));+d->next=list;+d->path=path;+d->bytecount=bytecount+batchsize*(k/26);+d->batchsize=batchsize;+d->sum_errors=0;+d->sum_good=0;+d->letter='A'+(k%26);++if(pthread_create(&d->pthread_id,NULL,multiple_thread_proc,d)){+warning("failed to create thread[%d] skipping remainder",k);+free(d);+break;+}++list=d;+}++while(list){+structmultiple_thread_data*d=list;++if(pthread_join(d->pthread_id,NULL))+sum_join_errors++;++sum_thread_errors+=d->sum_errors;+sum_good+=d->sum_good;++list=d->next;+free(d);+}++printf("client (good %d) (join %d), (errors %d)\n",+sum_good,sum_join_errors,sum_thread_errors);++return(sum_join_errors+sum_thread_errors)?1:0;+}++intcmd__simple_ipc(intargc,constchar**argv)+{+constchar*path="ipc-test";++if(argc==2&&!strcmp(argv[1],"SUPPORTS_SIMPLE_IPC"))+return0;++/* Use '!!' on all dispatch functions to map from `error()` style+*(returns-1)styleto`test_must_fail`style(expects1)and+*getlessconfusingshellerrormessages.+*/++if(argc==2&&!strcmp(argv[1],"is-active"))+return!!client__probe_server(path);++if(argc>=2&&!strcmp(argv[1],"daemon"))+return!!daemon__run_server(path,argc,argv);++/*+*Clientcommandsfollow.Ensureaserverisrunningbefore+*goinganyfurther.+*/+if(client__probe_server(path))+return1;++if((argc==2||argc==3)&&!strcmp(argv[1],"send"))+return!!client__send_ipc(argc,argv,path);++if(argc>=2&&!strcmp(argv[1],"sendbytes"))+return!!client__sendbytes(argc,argv,path);++if(argc>=2&&!strcmp(argv[1],"multiple"))+return!!client__multiple(argc,argv,path);++die("Unhandled argv[1]: '%s'",argv[1]);+}+#endif
@@ -0,0 +1,129 @@+#!/bin/sh++test_description='simple command server'++../test-lib.sh++test-toolsimple-ipcSUPPORTS_SIMPLE_IPC||{+skip_all='simple IPC not supported on this platform'+test_done+}++stop_simple_IPC_server(){+test-n"$SIMPLE_IPC_PID"||return0++kill"$SIMPLE_IPC_PID"&&+SIMPLE_IPC_PID=+}++test_expect_success'start simple command server''+{test-toolsimple-ipcdaemon--threads=8&}&&+SIMPLE_IPC_PID=$!&&+test_atexitstop_simple_IPC_server&&++sleep1&&++test-toolsimple-ipcis-active+'++test_expect_success'simple command server''+test-toolsimple-ipcsendping>actual&&+echopong>expect&&+test_cmpexpectactual+'++test_expect_success'servers cannot share the same path''+test_must_failtest-toolsimple-ipcdaemon&&+test-toolsimple-ipcis-active+'++test_expect_success'big response''+test-toolsimple-ipcsendbig>actual&&+test_line_count-ge10000actual&&+grep-q"big: [0]*9999\$"actual+'++test_expect_success'chunk response''+test-toolsimple-ipcsendchunk>actual&&+test_line_count-ge10000actual&&+grep-q"big: [0]*9999\$"actual+'++test_expect_success'slow response''+test-toolsimple-ipcsendslow>actual&&+test_line_count-ge100actual&&+grep-q"big: [0]*99\$"actual+'++# Send an IPC with n=100,000 bytes of ballast. This should be large enough+# to force both the kernel and the pkt-line layer to chunk the message to the+# daemon and for the daemon to receive it in chunks.+#+test_expect_success'sendbytes''+test-toolsimple-ipcsendbytes--bytecount=100000--byte=A>actual&&+grep"sent:A00100000 rcvd:A00100000"actual+'++# Start a series of <threads> client threads that each make <batchsize>+# IPC requests to the server. Each (<threads> * <batchsize>) request+# will open a new connection to the server and randomly bind to a server+# thread. Each client thread exits after completing its batch. So the+# total number of live client threads will be smaller than the total.+# Each request will send a message containing at least <bytecount> bytes+# of ballast. (Responses are small.)+#+# The purpose here is to test threading in the server and responding to+# many concurrent client requests (regardless of whether they come from+# 1 client process or many). And to test that the server side of the+# named pipe/socket is stable. (On Windows this means that the server+# pipe is properly recycled.)+#+# On Windows it also lets us adjust the connection timeout in the+# `ipc_client_send_command()`.+#+# Note it is easy to drive the system into failure by requesting an+# insane number of threads on client or server and/or increasing the+# per-thread batchsize or the per-request bytecount (ballast).+# On Windows these failures look like "pipe is busy" errors.+# So I've chosen fairly conservative values for now.+#+# We expect output of the form "sent:<letter><length> ..."+# With terms (7, 19, 13) we expect:+# <letter> in [A-G]+# <length> in [19+0 .. 19+(13-1)]+# and (7 * 13) successful responses.+#+test_expect_success'stress test threads''+test-toolsimple-ipcmultiple\+--threads=7\+--bytecount=19\+--batchsize=13\+>actual&&+test_line_count=92actual&&+grep"good 91"actual&&+grep"sent:A"<actual>actual_a&&+cat>expect_a<<-EOF&&+sent:A00000019rcvd:A00000019+sent:A00000020rcvd:A00000020+sent:A00000021rcvd:A00000021+sent:A00000022rcvd:A00000022+sent:A00000023rcvd:A00000023+sent:A00000024rcvd:A00000024+sent:A00000025rcvd:A00000025+sent:A00000026rcvd:A00000026+sent:A00000027rcvd:A00000027+sent:A00000028rcvd:A00000028+sent:A00000029rcvd:A00000029+sent:A00000030rcvd:A00000030+sent:A00000031rcvd:A00000031+EOF+test_cmpexpect_aactual_a+'++test_expect_success'`quit` works''+test-toolsimple-ipcsendquit&&+test_must_failtest-toolsimple-ipcis-active&&+test_must_failtest-toolsimple-ipcsendping+'++test_done
@@ -0,0 +1,129 @@+#!/bin/sh++test_description='simple command server'++../test-lib.sh++test-toolsimple-ipcSUPPORTS_SIMPLE_IPC||{+skip_all='simple IPC not supported on this platform'+test_done+}++stop_simple_IPC_server(){+test-n"$SIMPLE_IPC_PID"||return0++kill"$SIMPLE_IPC_PID"&&+SIMPLE_IPC_PID=+}++test_expect_success'start simple command server''+{test-toolsimple-ipcdaemon--threads=8&}&&+SIMPLE_IPC_PID=$!&&+test_atexitstop_simple_IPC_server&&++sleep1&&
This will certainly lead to occasional failures when the daemon takes
longer than that mere 1 second delay under heavy load or in CI jobs.
This will certainly lead to occasional failures when the daemon takes
longer than that mere 1 second delay under heavy load or in CI jobs.
Yeah. The robust thing is to have the server indicate when it's ready to
receive requests. There's some prior art in t/lib-git-daemon.sh using a
fifo to get a line to the caller. It's ugly, but AFAIK pretty
bulletproof.
-Peff
@@ -0,0 +1,129 @@+#!/bin/sh++test_description='simple command server'++../test-lib.sh++test-toolsimple-ipcSUPPORTS_SIMPLE_IPC||{+skip_all='simple IPC not supported on this platform'+test_done+}++stop_simple_IPC_server(){+test-n"$SIMPLE_IPC_PID"||return0++kill"$SIMPLE_IPC_PID"&&+SIMPLE_IPC_PID=+}++test_expect_success'start simple command server''+{test-toolsimple-ipcdaemon--threads=8&}&&+SIMPLE_IPC_PID=$!&&+test_atexitstop_simple_IPC_server&&++sleep1&&
This will certainly lead to occasional failures when the daemon takes
longer than that mere 1 second delay under heavy load or in CI jobs.
This test is flaky as well, and it did actually fail in CI:
expecting success of 0052.9 '`quit` works':
test-tool simple-ipc send quit &&
test_must_fail test-tool simple-ipc is-active &&
test_must_fail test-tool simple-ipc send ping
+test-tool simple-ipc send quit
+test_must_fail test-tool simple-ipc is-active
test_must_fail: command succeeded: test-tool simple-ipc is-active
error: last command exited with $?=1
not ok 9 - `quit` works
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:49:45
From: Jeff Hostetler <redacted>
Calls to `chdir()` are dangerous in a multi-threaded context. If
`unix_stream_listen()` is given a socket pathname that is too big to
fit in a `sockaddr_un` structure, it will `chdir()` to the parent
directory of the requested socket pathname, create the socket using a
relative pathname, and then `chdir()` back. This is not thread-safe.
Add `disallow_chdir` flag to `struct unix_sockaddr_context` and change
all callers to pass an initialized context structure.
Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag
is set.
Signed-off-by: Jeff Hostetler <redacted>
---
unix-socket.c | 19 ++++++++++++++++---
unix-socket.h | 2 ++
2 files changed, 18 insertions(+), 3 deletions(-)
From: Jeff King <hidden> Date: 2021-02-02 10:27:52
On Mon, Feb 01, 2021 at 07:45:45PM +0000, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <redacted>
Calls to `chdir()` are dangerous in a multi-threaded context. If
`unix_stream_listen()` is given a socket pathname that is too big to
fit in a `sockaddr_un` structure, it will `chdir()` to the parent
directory of the requested socket pathname, create the socket using a
relative pathname, and then `chdir()` back. This is not thread-safe.
Add `disallow_chdir` flag to `struct unix_sockaddr_context` and change
all callers to pass an initialized context structure.
Teach `unix_sockaddr_init()` to not allow calls to `chdir()` when flag
is set.
Makes sense, and it fits nicely into the options pattern you set up in
the earlier patch.
It is really just zero-initializing, so "{ 0 }" would be OK (I think we
are relaxed about allowing 0 as NULL in initializers). But I don't mind
it being written out (but do mind whitespace around the "=").
However, the point of unix_sockaddr_init() is that it's supposed to
initialize the struct. And I don't think we need to carry disallow_chdir
around; the cleanup function knows from orig_dir whether it's supposed
to do any cleanup, so only the init function has to care. So would:
make it more obvious? There are only two callers, and this is all
file-local, so I don't mind adding the extra parameter there. And you
would not need an initializer at all.
I don't know if we care, but some options are positive "do this unlink"
and some are negative "do not do this chdir". Those could be made
consistent (and flip the initializer value to keep the same defaults).
There is actually value in making struct defaults generally "0" unless
we have reason not to, because callers sometimes zero-initialize without
thinking about it. I doubt that would happen for this particular struct,
and I'm deep into bike-shedding anyway, so I'm OK either way. But
something like:
struct unix_stream_listen_opts_init {
int listen_backlog_size;
int disallow_unlink;
int disallow_chdir;
};
would work with just a "{ 0 }" zero-initializer. :)
-Peff
From: Jeff Hostetler via GitGitGadget <hidden> Date: 2021-02-01 19:49:47
From: Jeff Hostetler <redacted>
Create version of `write_packetized_from_buf()` that takes a scratch buffer
argument rather than assuming a static buffer. This will be used later as
we make packet-line writing more thread-safe.
Signed-off-by: Jeff Hostetler <redacted>
---
pkt-line.c | 9 ++++++++-
pkt-line.h | 2 ++
2 files changed, 10 insertions(+), 1 deletion(-)
@@ -278,6 +278,13 @@ int write_packetized_from_fd(int fd_in, int fd_out)intwrite_packetized_from_buf(constchar*src_in,size_tlen,intfd_out){staticstructpacket_scratch_spacescratch;++returnwrite_packetized_from_buf2(src_in,len,fd_out,&scratch);+}++intwrite_packetized_from_buf2(constchar*src_in,size_tlen,intfd_out,+structpacket_scratch_space*scratch)+{interr=0;size_tbytes_written=0;size_tbytes_to_write;
@@ -289,7 +296,7 @@ int write_packetized_from_buf(const char *src_in, size_t len, int fd_out)bytes_to_write=len-bytes_written;if(bytes_to_write==0)break;-err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write,&scratch);+err=packet_write_gently(fd_out,src_in+bytes_written,bytes_to_write,scratch);bytes_written+=bytes_to_write;}if(!err)
From: Jeff King <hidden> Date: 2021-02-02 09:45:50
On Mon, Feb 01, 2021 at 07:45:36PM +0000, Jeff Hostetler via GitGitGadget wrote:
From: Jeff Hostetler <redacted>
Create version of `write_packetized_from_buf()` that takes a scratch buffer
argument rather than assuming a static buffer. This will be used later as
we make packet-line writing more thread-safe.
OK, this is extending the changes from the first patch...
Oof, that name. I know we are guilty of a lot of "foo_1()" helpers for
foo(), but they are usually internal static functions that don't get
spread around. This one is a public function.
Something like "_with_scratch" might be a bit more descriptive. Though
given that there is exactly one caller of the original currently, I'd be
tempted to say that it should just learn the scratch-space argument.
(All of this is moot, of course, if you follow either of my suggestions
from the earlier patch to drop the need for this scratch space
entirely).
-Peff