{"thread":{"id":"55255","subject":"[PATCH 0/8] Simple IPC Cleanups","startedAt":"2021-03-04T20:19:02Z","lastAt":"2021-03-08T14:15:37Z","messageCount":15,"participants":["Jeff Hostetler via GitGitGadget","Junio C Hamano","Jeff Hostetler","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"418246","messageId":"pull.893.git.1614889047.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":null,"subject":"[PATCH 0/8] Simple IPC Cleanups","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:19Z","receivedAt":"2021-03-04T20:19:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"This patch series adds a few final cleanup and refactoring commits onto the\njh/simple-ipc branch. These are in response to mailing list comments that I\nreceived on my V4 version after it was queued into next.\n\nhttps://lore.kernel.org/git/pull.766.v4.git.1613598529.gitgitgadget@gmail.com/T/#mbd1da5ff93ef273049090f697aeab68c74f698f1\n\nThere are a couple of large changes that I'll call out here.\n\nIn the third commit, I moved the new lock-aware code out of unix-socket.c\nand into its own source file. This creates a slightly misleading diff at\ntimes (in gitk) where it looks like 51% copy of unix-socket.c rather than a\nnew file. On the command line and on GitHub it looks better.\n\nIn the commits prefixed with test-simple-ipc: ... I refactored the options\nparsing to allow the name of the socket/named-pipe to be passed on the\ncommand line so that the Windows version could do so (since it needs to exec\na child rather than fork). This turned into a larger cleanup/refactoring\nthan I had expected, but I think the result is much better. I unified all of\nthe option parsing into the main cmd__simple_ipc() function and got rid of\nthe smaller parsers inside of each subcommand. With this, all of the\nsubcommands now allow an alternate socket path to be used. (Just fixing the\nunused arg on the Windows side would allow us to spawn a background daemon\non a different socket, but none of the client subcommands would be able to\ntalk to it.)\n\nJeff Hostetler (8):\n pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()\n unix-socket: simplify initialization of unix_stream_listen_opts\n unix-stream-server: create unix-stream-server.c\n simple-ipc: move error handling up a level\n unix-stream-server: add st_dev and st_mode to socket stolen checks\n test-simple-ipc: refactor command line option processing in helper\n test-simple-ipc: add --token= string option\n simple-ipc: update design documentation with more details\n\n Documentation/technical/api-simple-ipc.txt | 131 +++++++--\n Makefile | 1 +\n compat/simple-ipc/ipc-unix-socket.c | 49 ++--\n compat/simple-ipc/ipc-win32.c | 14 +-\n contrib/buildsystems/CMakeLists.txt | 2 +-\n convert.c | 7 +-\n pkt-line.c | 19 +-\n pkt-line.h | 6 +-\n simple-ipc.h | 4 +\n t/helper/test-simple-ipc.c | 326 +++++++++++----------\n t/t0052-simple-ipc.sh | 10 +-\n unix-socket.c | 117 +-------\n unix-socket.h | 33 +--\n unix-stream-server.c | 130 ++++++++\n unix-stream-server.h | 35 +++\n 15 files changed, 502 insertions(+), 382 deletions(-)\n create mode 100644 unix-stream-server.c\n create mode 100644 unix-stream-server.h\n\n\nbase-commit: edce16a37ab87513a3f0bc805e9bf372bdd02961\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-893%2Fjeffhostetler%2Fnext-simple-ipc-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-893/jeffhostetler/next-simple-ipc-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/893\n-- \ngitgitgadget\n"},{"id":"418247","messageId":"db0467a7e4193adf525bf15ee333ec0bbab55872.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 6/8] test-simple-ipc: refactor command line option processing in helper","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:25Z","receivedAt":"2021-03-04T20:19:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nRefactor command line option processing in simple-ipc helper under\nthe main \"simple-ipc\" command rather than in the individual subcommands.\n\nUpdate \"start-daemon\" subcommand to pass socket pathname to background\nprocess on both platforms.\n\nSigned-off-by: Jeff Hostetler \n---\n t/helper/test-simple-ipc.c | 307 ++++++++++++++++++-------------------\n 1 file changed, 153 insertions(+), 154 deletions(-)\n\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex d0e6127528a7..116c824d7555 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -213,43 +213,53 @@ static int test_app_cb(void *application_data,\n \treturn app__unhandled_command(command, reply_cb, reply_data);\n }\n \n+struct cl_args\n+{\n+\tconst char *subcommand;\n+\tconst char *path;\n+\n+\tint nr_threads;\n+\tint max_wait_sec;\n+\tint bytecount;\n+\tint batchsize;\n+\n+\tchar bytevalue;\n+};\n+\n+struct cl_args cl_args = {\n+\t.subcommand = NULL,\n+\t.path = \"ipc-test\",\n+\n+\t.nr_threads = 5,\n+\t.max_wait_sec = 60,\n+\t.bytecount = 1024,\n+\t.batchsize = 10,\n+\n+\t.bytevalue = 'x',\n+};\n+\n /*\n * This process will run as a simple-ipc server and listen for IPC commands\n * from client processes.\n */\n-static int daemon__run_server(const char *path, int argc, const char **argv)\n+static int daemon__run_server(void)\n {\n \tint ret;\n \n \tstruct ipc_server_opts opts = {\n-\t\t.nr_threads = 5\n-\t};\n-\n-\tconst char * const daemon_usage[] = {\n-\t\tN_(\"test-helper simple-ipc run-daemon [\"),\n-\t\tNULL\n+\t\t.nr_threads = cl_args.nr_threads,\n \t};\n-\tstruct option daemon_options[] = {\n-\t\tOPT_INTEGER(0, \"threads\", &opts.nr_threads,\n-\t\t\t N_(\"number of threads in server thread pool\")),\n-\t\tOPT_END()\n-\t};\n-\n-\targc = parse_options(argc, argv, NULL, daemon_options, daemon_usage, 0);\n-\n-\tif (opts.nr_threads < 1)\n-\t\topts.nr_threads = 1;\n \n \t/*\n \t * Synchronously run the ipc-server. We don't need any application\n \t * instance data, so pass an arbitrary pointer (that we'll later\n \t * verify made the round trip).\n \t */\n-\tret = ipc_server_run(path, &opts, test_app_cb, (void*)&my_app_data);\n+\tret = ipc_server_run(cl_args.path, &opts, test_app_cb, (void*)&my_app_data);\n \tif (ret == -2)\n-\t\terror(_(\"socket/pipe already in use: '%s'\"), path);\n+\t\terror(_(\"socket/pipe already in use: '%s'\"), cl_args.path);\n \telse if (ret == -1)\n-\t\terror_errno(_(\"could not start server on: '%s'\"), path);\n+\t\terror_errno(_(\"could not start server on: '%s'\"), cl_args.path);\n \n \treturn ret;\n }\n@@ -259,10 +269,12 @@ static int daemon__run_server(const char *path, int argc, const char **argv)\n * This is adapted from `daemonize()`. Use `fork()` to directly create and\n * run the daemon in a child process.\n */\n-static int spawn_server(const char *path,\n-\t\t\tconst struct ipc_server_opts *opts,\n-\t\t\tpid_t *pid)\n+static int spawn_server(pid_t *pid)\n {\n+\tstruct ipc_server_opts opts = {\n+\t\t.nr_threads = cl_args.nr_threads,\n+\t};\n+\n \t*pid = fork();\n \n \tswitch (*pid) {\n@@ -274,7 +286,8 @@ static int spawn_server(const char *path,\n \t\tclose(2);\n \t\tsanitize_stdfds();\n \n-\t\treturn ipc_server_run(path, opts, test_app_cb, (void*)&my_app_data);\n+\t\treturn ipc_server_run(cl_args.path, &opts, test_app_cb,\n+\t\t\t\t (void*)&my_app_data);\n \n \tcase -1:\n \t\treturn error_errno(_(\"could not spawn daemon in the background\"));\n@@ -289,9 +302,7 @@ static int spawn_server(const char *path,\n * have `fork(2)`. Spawn a normal Windows child process but without the\n * limitations of `start_command()` and `finish_command()`.\n */\n-static int spawn_server(const char *path,\n-\t\t\tconst struct ipc_server_opts *opts,\n-\t\t\tpid_t *pid)\n+static int spawn_server(pid_t *pid)\n {\n \tchar test_tool_exe[MAX_PATH];\n \tstruct strvec args = STRVEC_INIT;\n@@ -305,7 +316,8 @@ static int spawn_server(const char *path,\n \tstrvec_push(&args, test_tool_exe);\n \tstrvec_push(&args, \"simple-ipc\");\n \tstrvec_push(&args, \"run-daemon\");\n-\tstrvec_pushf(&args, \"--threads=%d\", opts->nr_threads);\n+\tstrvec_pushf(&args, \"--name=%s\", cl_args.path);\n+\tstrvec_pushf(&args, \"--threads=%d\", cl_args.nr_threads);\n \n \t*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);\n \tclose(in);\n@@ -325,8 +337,7 @@ static int spawn_server(const char *path,\n * let it get started and begin listening for requests on the socket\n * before reporting our success.\n */\n-static int wait_for_server_startup(const char * path, pid_t pid_child,\n-\t\t\t\t int max_wait_sec)\n+static int wait_for_server_startup(pid_t pid_child)\n {\n \tint status;\n \tpid_t pid_seen;\n@@ -334,7 +345,7 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,\n \ttime_t time_limit, now;\n \n \ttime(&time_limit);\n-\ttime_limit += max_wait_sec;\n+\ttime_limit += cl_args.max_wait_sec;\n \n \tfor (;;) {\n \t\tpid_seen = waitpid(pid_child, &status, WNOHANG);\n@@ -354,7 +365,7 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,\n \t\t\t * after a timeout on the lock), but we don't\n \t\t\t * care (who responds) if the socket is live.\n \t\t\t */\n-\t\t\ts = ipc_get_active_state(path);\n+\t\t\ts = ipc_get_active_state(cl_args.path);\n \t\t\tif (s == IPC_STATE__LISTENING)\n \t\t\t\treturn 0;\n \n@@ -377,7 +388,7 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,\n \t\t\t *\n \t\t\t * Again, we don't care who services the socket.\n \t\t\t */\n-\t\t\ts = ipc_get_active_state(path);\n+\t\t\ts = ipc_get_active_state(cl_args.path);\n \t\t\tif (s == IPC_STATE__LISTENING)\n \t\t\t\treturn 0;\n \n@@ -404,39 +415,15 @@ static int wait_for_server_startup(const char * path, pid_t pid_child,\n * more control and better error reporting (and makes it easier to write\n * unit tests).\n */\n-static int daemon__start_server(const char *path, int argc, const char **argv)\n+static int daemon__start_server(void)\n {\n \tpid_t pid_child;\n \tint ret;\n-\tint max_wait_sec = 60;\n-\tstruct ipc_server_opts opts = {\n-\t\t.nr_threads = 5\n-\t};\n-\n-\tconst char * const daemon_usage[] = {\n-\t\tN_(\"test-helper simple-ipc start-daemon [\"),\n-\t\tNULL\n-\t};\n-\n-\tstruct option daemon_options[] = {\n-\t\tOPT_INTEGER(0, \"max-wait\", &max_wait_sec,\n-\t\t\t N_(\"seconds to wait for daemon to startup\")),\n-\t\tOPT_INTEGER(0, \"threads\", &opts.nr_threads,\n-\t\t\t N_(\"number of threads in server thread pool\")),\n-\t\tOPT_END()\n-\t};\n-\n-\targc = parse_options(argc, argv, NULL, daemon_options, daemon_usage, 0);\n-\n-\tif (max_wait_sec < 0)\n-\t\tmax_wait_sec = 0;\n-\tif (opts.nr_threads < 1)\n-\t\topts.nr_threads = 1;\n \n \t/*\n \t * Run the actual daemon in a background process.\n \t */\n-\tret = spawn_server(path, &opts, &pid_child);\n+\tret = spawn_server(&pid_child);\n \tif (pid_child <= 0)\n \t\treturn ret;\n \n@@ -444,7 +431,7 @@ static int daemon__start_server(const char *path, int argc, const char **argv)\n \t * Let the parent wait for the child process to get started\n \t * and begin listening for requests on the socket.\n \t */\n-\tret = wait_for_server_startup(path, pid_child, max_wait_sec);\n+\tret = wait_for_server_startup(pid_child);\n \n \treturn ret;\n }\n@@ -455,27 +442,27 @@ static int daemon__start_server(const char *path, int argc, const char **argv)\n *\n * Returns 0 if the server is alive.\n */\n-static int client__probe_server(const char *path)\n+static int client__probe_server(void)\n {\n \tenum ipc_active_state s;\n \n-\ts = ipc_get_active_state(path);\n+\ts = ipc_get_active_state(cl_args.path);\n \tswitch (s) {\n \tcase IPC_STATE__LISTENING:\n \t\treturn 0;\n \n \tcase IPC_STATE__NOT_LISTENING:\n-\t\treturn error(\"no server listening at '%s'\", path);\n+\t\treturn error(\"no server listening at '%s'\", cl_args.path);\n \n \tcase IPC_STATE__PATH_NOT_FOUND:\n-\t\treturn error(\"path not found '%s'\", path);\n+\t\treturn error(\"path not found '%s'\", cl_args.path);\n \n \tcase IPC_STATE__INVALID_PATH:\n-\t\treturn error(\"invalid pipe/socket name '%s'\", path);\n+\t\treturn error(\"invalid pipe/socket name '%s'\", cl_args.path);\n \n \tcase IPC_STATE__OTHER_ERROR:\n \tdefault:\n-\t\treturn error(\"other error for '%s'\", path);\n+\t\treturn error(\"other error for '%s'\", cl_args.path);\n \t}\n }\n \n@@ -486,9 +473,9 @@ static int client__probe_server(const char *path)\n * argv[2] contains a simple (1 word) command that `test_app_cb()` (in\n * the daemon process) will understand.\n */\n-static int client__send_ipc(int argc, const char **argv, const char *path)\n+static int client__send_ipc(const char *send_token)\n {\n-\tconst char *command = argc > 2 ? argv[2] : \"(no command)\";\n+\tconst char *command = send_token ? send_token : \"(no-command)\";\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ipc_client_connect_options options\n \t\t= IPC_CLIENT_CONNECT_OPTIONS_INIT;\n@@ -496,7 +483,7 @@ static int client__send_ipc(int argc, const char **argv, const char *path)\n \toptions.wait_if_busy = 1;\n \toptions.wait_if_not_found = 0;\n \n-\tif (!ipc_client_send_command(path, &options, command, &buf)) {\n+\tif (!ipc_client_send_command(cl_args.path, &options, command, &buf)) {\n \t\tif (buf.len) {\n \t\t\tprintf(\"%s\\n\", buf.buf);\n \t\t\tfflush(stdout);\n@@ -506,7 +493,7 @@ static int client__send_ipc(int argc, const char **argv, const char *path)\n \t\treturn 0;\n \t}\n \n-\treturn error(\"failed to send '%s' to '%s'\", command, path);\n+\treturn error(\"failed to send '%s' to '%s'\", command, cl_args.path);\n }\n \n /*\n@@ -515,41 +502,23 @@ static int client__send_ipc(int argc, const char **argv, const char *path)\n * event in the server, so we spin and wait here for it to actually\n * shutdown to make the unit tests a little easier to write.\n */\n-static int client__stop_server(int argc, const char **argv, const char *path)\n+static int client__stop_server(void)\n {\n-\tconst char *send_quit[] = { argv[0], \"send\", \"quit\", NULL };\n-\tint max_wait_sec = 60;\n \tint ret;\n \ttime_t time_limit, now;\n \tenum ipc_active_state s;\n \n-\tconst char * const stop_usage[] = {\n-\t\tN_(\"test-helper simple-ipc stop-daemon []\"),\n-\t\tNULL\n-\t};\n-\n-\tstruct option stop_options[] = {\n-\t\tOPT_INTEGER(0, \"max-wait\", &max_wait_sec,\n-\t\t\t N_(\"seconds to wait for daemon to stop\")),\n-\t\tOPT_END()\n-\t};\n-\n-\targc = parse_options(argc, argv, NULL, stop_options, stop_usage, 0);\n-\n-\tif (max_wait_sec < 0)\n-\t\tmax_wait_sec = 0;\n-\n \ttime(&time_limit);\n-\ttime_limit += max_wait_sec;\n+\ttime_limit += cl_args.max_wait_sec;\n \n-\tret = client__send_ipc(3, send_quit, path);\n+\tret = client__send_ipc(\"quit\");\n \tif (ret)\n \t\treturn ret;\n \n \tfor (;;) {\n \t\tsleep_millisec(100);\n \n-\t\ts = ipc_get_active_state(path);\n+\t\ts = ipc_get_active_state(cl_args.path);\n \n \t\tif (s != IPC_STATE__LISTENING) {\n \t\t\t/*\n@@ -597,19 +566,8 @@ static int do_sendbytes(int bytecount, char byte, const char *path,\n /*\n * Send an IPC command with ballast to an already-running server daemon.\n */\n-static int client__sendbytes(int argc, const char **argv, const char *path)\n+static int client__sendbytes(void)\n {\n-\tint bytecount = 1024;\n-\tchar *string = \"x\";\n-\tconst char * const sendbytes_usage[] = {\n-\t\tN_(\"test-helper simple-ipc sendbytes []\"),\n-\t\tNULL\n-\t};\n-\tstruct option sendbytes_options[] = {\n-\t\tOPT_INTEGER(0, \"bytecount\", &bytecount, N_(\"number of bytes\")),\n-\t\tOPT_STRING(0, \"byte\", &string, N_(\"byte\"), N_(\"ballast\")),\n-\t\tOPT_END()\n-\t};\n \tstruct ipc_client_connect_options options\n \t\t= IPC_CLIENT_CONNECT_OPTIONS_INIT;\n \n@@ -617,9 +575,8 @@ static int client__sendbytes(int argc, const char **argv, const char *path)\n \toptions.wait_if_not_found = 0;\n \toptions.uds_disallow_chdir = 0;\n \n-\targc = parse_options(argc, argv, NULL, sendbytes_options, sendbytes_usage, 0);\n-\n-\treturn do_sendbytes(bytecount, string[0], path, &options);\n+\treturn do_sendbytes(cl_args.bytecount, cl_args.bytevalue, cl_args.path,\n+\t\t\t &options);\n }\n \n struct multiple_thread_data {\n@@ -666,43 +623,20 @@ static void *multiple_thread_proc(void *_multiple_thread_data)\n * Start a client-side thread pool. Each thread sends a series of\n * IPC requests. Each request is on a new connection to the server.\n */\n-static int client__multiple(int argc, const char **argv, const char *path)\n+static int client__multiple(void)\n {\n \tstruct multiple_thread_data *list = NULL;\n \tint k;\n-\tint nr_threads = 5;\n-\tint bytecount = 1;\n-\tint batchsize = 10;\n \tint sum_join_errors = 0;\n \tint sum_thread_errors = 0;\n \tint sum_good = 0;\n \n-\tconst char * const multiple_usage[] = {\n-\t\tN_(\"test-helper simple-ipc multiple []\"),\n-\t\tNULL\n-\t};\n-\tstruct option multiple_options[] = {\n-\t\tOPT_INTEGER(0, \"bytecount\", &bytecount, N_(\"number of bytes\")),\n-\t\tOPT_INTEGER(0, \"threads\", &nr_threads, N_(\"number of threads\")),\n-\t\tOPT_INTEGER(0, \"batchsize\", &batchsize, N_(\"number of requests per thread\")),\n-\t\tOPT_END()\n-\t};\n-\n-\targc = parse_options(argc, argv, NULL, multiple_options, multiple_usage, 0);\n-\n-\tif (bytecount < 1)\n-\t\tbytecount = 1;\n-\tif (nr_threads < 1)\n-\t\tnr_threads = 1;\n-\tif (batchsize < 1)\n-\t\tbatchsize = 1;\n-\n-\tfor (k = 0; k < nr_threads; k++) {\n+\tfor (k = 0; k < cl_args.nr_threads; k++) {\n \t\tstruct multiple_thread_data *d = xcalloc(1, sizeof(*d));\n \t\td->next = list;\n-\t\td->path = path;\n-\t\td->bytecount = bytecount + batchsize*(k/26);\n-\t\td->batchsize = batchsize;\n+\t\td->path = cl_args.path;\n+\t\td->bytecount = cl_args.bytecount + cl_args.batchsize*(k/26);\n+\t\td->batchsize = cl_args.batchsize;\n \t\td->sum_errors = 0;\n \t\td->sum_good = 0;\n \t\td->letter = 'A' + (k % 26);\n@@ -737,45 +671,110 @@ static int client__multiple(int argc, const char **argv, const char *path)\n \n int cmd__simple_ipc(int argc, const char **argv)\n {\n-\tconst char *path = \"ipc-test\";\n+\tconst char * const simple_ipc_usage[] = {\n+\t\tN_(\"test-helper simple-ipc is-active [] []\"),\n+\t\tN_(\"test-helper simple-ipc run-daemon [] []\"),\n+\t\tN_(\"test-helper simple-ipc start-daemon [] [] []\"),\n+\t\tN_(\"test-helper simple-ipc stop-daemon [] []\"),\n+\t\tN_(\"test-helper simple-ipc send [] []\"),\n+\t\tN_(\"test-helper simple-ipc sendbytes [] [] []\"),\n+\t\tN_(\"test-helper simple-ipc multiple [] [] [] []\"),\n+\t\tNULL\n+\t};\n+\n+\tconst char *bytevalue = NULL;\n+\n+\tstruct option options[] = {\n+#ifndef GIT_WINDOWS_NATIVE\n+\t\tOPT_STRING(0, \"name\", &cl_args.path, N_(\"name\"), N_(\"name or pathname of unix domain socket\")),\n+#else\n+\t\tOPT_STRING(0, \"name\", &cl_args.path, N_(\"name\"), N_(\"named-pipe name\")),\n+#endif\n+\t\tOPT_INTEGER(0, \"threads\", &cl_args.nr_threads, N_(\"number of threads in server thread pool\")),\n+\t\tOPT_INTEGER(0, \"max-wait\", &cl_args.max_wait_sec, N_(\"seconds to wait for daemon to start or stop\")),\n+\n+\t\tOPT_INTEGER(0, \"bytecount\", &cl_args.bytecount, N_(\"number of bytes\")),\n+\t\tOPT_INTEGER(0, \"batchsize\", &cl_args.batchsize, N_(\"number of requests per thread\")),\n+\n+\t\tOPT_STRING(0, \"byte\", &bytevalue, N_(\"byte\"), N_(\"ballast character\")),\n+\n+\t\tOPT_END()\n+\t};\n+\n+\tif (argc < 2)\n+\t\tusage_with_options(simple_ipc_usage, options);\n+\n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(simple_ipc_usage, options);\n \n \tif (argc == 2 && !strcmp(argv[1], \"SUPPORTS_SIMPLE_IPC\"))\n \t\treturn 0;\n \n+\tcl_args.subcommand = argv[1];\n+\n+\targc--;\n+\targv++;\n+\n+\targc = parse_options(argc, argv, NULL, options, simple_ipc_usage, 0);\n+\n+\tif (cl_args.nr_threads < 1)\n+\t\tcl_args.nr_threads = 1;\n+\tif (cl_args.max_wait_sec < 0)\n+\t\tcl_args.max_wait_sec = 0;\n+\tif (cl_args.bytecount < 1)\n+\t\tcl_args.bytecount = 1;\n+\tif (cl_args.batchsize < 1)\n+\t\tcl_args.batchsize = 1;\n+\n+\tif (bytevalue && *bytevalue)\n+\t\tcl_args.bytevalue = bytevalue[0];\n+\n \t/*\n \t * Use '!!' on all dispatch functions to map from `error()` style\n \t * (returns -1) style to `test_must_fail` style (expects 1). This\n \t * makes shell error messages less confusing.\n \t */\n \n-\tif (argc == 2 && !strcmp(argv[1], \"is-active\"))\n-\t\treturn !!client__probe_server(path);\n+\tif (!strcmp(cl_args.subcommand, \"is-active\"))\n+\t\treturn !!client__probe_server();\n \n-\tif (argc >= 2 && !strcmp(argv[1], \"run-daemon\"))\n-\t\treturn !!daemon__run_server(path, argc, argv);\n+\tif (!strcmp(cl_args.subcommand, \"run-daemon\"))\n+\t\treturn !!daemon__run_server();\n \n-\tif (argc >= 2 && !strcmp(argv[1], \"start-daemon\"))\n-\t\treturn !!daemon__start_server(path, argc, argv);\n+\tif (!strcmp(cl_args.subcommand, \"start-daemon\"))\n+\t\treturn !!daemon__start_server();\n \n \t/*\n \t * Client commands follow. Ensure a server is running before\n-\t * going any further.\n+\t * sending any data. This might be overkill, but then again\n+\t * this is a test harness.\n \t */\n-\tif (client__probe_server(path))\n-\t\treturn 1;\n \n-\tif (argc >= 2 && !strcmp(argv[1], \"stop-daemon\"))\n-\t\treturn !!client__stop_server(argc, argv, path);\n+\tif (!strcmp(cl_args.subcommand, \"stop-daemon\")) {\n+\t\tif (client__probe_server())\n+\t\t\treturn 1;\n+\t\treturn !!client__stop_server();\n+\t}\n \n-\tif ((argc == 2 || argc == 3) && !strcmp(argv[1], \"send\"))\n-\t\treturn !!client__send_ipc(argc, argv, path);\n+\tif (!strcmp(cl_args.subcommand, \"send\")) {\n+\t\tconst char *send_token = argv[0];\n+\t\tif (client__probe_server())\n+\t\t\treturn 1;\n+\t\treturn !!client__send_ipc(send_token);\n+\t}\n \n-\tif (argc >= 2 && !strcmp(argv[1], \"sendbytes\"))\n-\t\treturn !!client__sendbytes(argc, argv, path);\n+\tif (!strcmp(cl_args.subcommand, \"sendbytes\")) {\n+\t\tif (client__probe_server())\n+\t\t\treturn 1;\n+\t\treturn !!client__sendbytes();\n+\t}\n \n-\tif (argc >= 2 && !strcmp(argv[1], \"multiple\"))\n-\t\treturn !!client__multiple(argc, argv, path);\n+\tif (!strcmp(cl_args.subcommand, \"multiple\")) {\n+\t\tif (client__probe_server())\n+\t\t\treturn 1;\n+\t\treturn !!client__multiple();\n+\t}\n \n-\tdie(\"Unhandled argv[1]: '%s'\", argv[1]);\n+\tdie(\"Unhandled subcommand: '%s'\", cl_args.subcommand);\n }\n #endif\n-- \ngitgitgadget\n\n"},{"id":"418248","messageId":"1902c23b87dbf6d4cf44d5a51c10eef539bb1243.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 7/8] test-simple-ipc: add --token= string option","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:26Z","receivedAt":"2021-03-04T20:19:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nChange the `test-tool simple-ipc send` subcommand take a string\noption rather than assume the last unclaimed value of argv.\n\nThis makes it a little less awkward to use after the recent changes\nthat added the `--name=` option.\n\nSigned-off-by: Jeff Hostetler \n---\n t/helper/test-simple-ipc.c | 25 ++++++++++++++++---------\n t/t0052-simple-ipc.sh | 10 +++++-----\n 2 files changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 116c824d7555..4da63fd30c97 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -217,6 +217,7 @@ struct cl_args\n {\n \tconst char *subcommand;\n \tconst char *path;\n+\tconst char *token;\n \n \tint nr_threads;\n \tint max_wait_sec;\n@@ -229,6 +230,7 @@ struct cl_args\n struct cl_args cl_args = {\n \t.subcommand = NULL,\n \t.path = \"ipc-test\",\n+\t.token = NULL,\n \n \t.nr_threads = 5,\n \t.max_wait_sec = 60,\n@@ -467,19 +469,22 @@ static int client__probe_server(void)\n }\n \n /*\n- * Send an IPC command to an already-running server daemon and print the\n- * response.\n+ * Send an IPC command token to an already-running server daemon and\n+ * print the response.\n *\n- * argv[2] contains a simple (1 word) command that `test_app_cb()` (in\n- * the daemon process) will understand.\n+ * This is a simple 1 word command/token that `test_app_cb()` (in the\n+ * daemon process) will understand.\n */\n-static int client__send_ipc(const char *send_token)\n+static int client__send_ipc(void)\n {\n-\tconst char *command = send_token ? send_token : \"(no-command)\";\n+\tconst char *command = \"(no-command)\";\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct ipc_client_connect_options options\n \t\t= IPC_CLIENT_CONNECT_OPTIONS_INIT;\n \n+\tif (cl_args.token && *cl_args.token)\n+\t\tcommand = cl_args.token;\n+\n \toptions.wait_if_busy = 1;\n \toptions.wait_if_not_found = 0;\n \n@@ -511,7 +516,9 @@ static int client__stop_server(void)\n \ttime(&time_limit);\n \ttime_limit += cl_args.max_wait_sec;\n \n-\tret = client__send_ipc(\"quit\");\n+\tcl_args.token = \"quit\";\n+\n+\tret = client__send_ipc();\n \tif (ret)\n \t\treturn ret;\n \n@@ -697,6 +704,7 @@ int cmd__simple_ipc(int argc, const char **argv)\n \t\tOPT_INTEGER(0, \"batchsize\", &cl_args.batchsize, N_(\"number of requests per thread\")),\n \n \t\tOPT_STRING(0, \"byte\", &bytevalue, N_(\"byte\"), N_(\"ballast character\")),\n+\t\tOPT_STRING(0, \"token\", &cl_args.token, N_(\"token\"), N_(\"command token to send to the server\")),\n \n \t\tOPT_END()\n \t};\n@@ -757,10 +765,9 @@ int cmd__simple_ipc(int argc, const char **argv)\n \t}\n \n \tif (!strcmp(cl_args.subcommand, \"send\")) {\n-\t\tconst char *send_token = argv[0];\n \t\tif (client__probe_server())\n \t\t\treturn 1;\n-\t\treturn !!client__send_ipc(send_token);\n+\t\treturn !!client__send_ipc();\n \t}\n \n \tif (!strcmp(cl_args.subcommand, \"sendbytes\")) {\ndiff --git a/t/t0052-simple-ipc.sh b/t/t0052-simple-ipc.sh\nindex 18dcc8130728..ff98be31a51b 100755\n--- a/t/t0052-simple-ipc.sh\n+++ b/t/t0052-simple-ipc.sh\n@@ -20,7 +20,7 @@ test_expect_success 'start simple command server' '\n '\n \n test_expect_success 'simple command server' '\n-\ttest-tool simple-ipc send ping >actual &&\n+\ttest-tool simple-ipc send --token=ping >actual &&\n \techo pong >expect &&\n \ttest_cmp expect actual\n '\n@@ -31,19 +31,19 @@ test_expect_success 'servers cannot share the same path' '\n '\n \n test_expect_success 'big response' '\n-\ttest-tool simple-ipc send big >actual &&\n+\ttest-tool simple-ipc send --token=big >actual &&\n \ttest_line_count -ge 10000 actual &&\n \tgrep -q \"big: [0]*9999\\$\" actual\n '\n \n test_expect_success 'chunk response' '\n-\ttest-tool simple-ipc send chunk >actual &&\n+\ttest-tool simple-ipc send --token=chunk >actual &&\n \ttest_line_count -ge 10000 actual &&\n \tgrep -q \"big: [0]*9999\\$\" actual\n '\n \n test_expect_success 'slow response' '\n-\ttest-tool simple-ipc send slow >actual &&\n+\ttest-tool simple-ipc send --token=slow >actual &&\n \ttest_line_count -ge 100 actual &&\n \tgrep -q \"big: [0]*99\\$\" actual\n '\n@@ -116,7 +116,7 @@ test_expect_success 'stress test threads' '\n test_expect_success 'stop-daemon works' '\n \ttest-tool simple-ipc stop-daemon &&\n \ttest_must_fail test-tool simple-ipc is-active &&\n-\ttest_must_fail test-tool simple-ipc send ping\n+\ttest_must_fail test-tool simple-ipc send --token=ping\n '\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"418249","messageId":"7be460771da6d0f4de91780d5bb4d20b3b856f6b.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 4/8] simple-ipc: move error handling up a level","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:23Z","receivedAt":"2021-03-04T20:19:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nMove error reporting for socket/named pipe creation out of platform\nspecific and simple-ipc layer code and rely on returned error value\nand `errno`.\n\nUpdate t/helper/test-simple-ipc.c to print errors.\n\nSimplify testing for another server in `is_another_server_is_alive()`.\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-unix-socket.c | 48 ++++++++++++----------\n compat/simple-ipc/ipc-win32.c | 14 ++++---\n simple-ipc.h | 4 ++\n t/helper/test-simple-ipc.c | 10 ++++-\n unix-stream-server.c | 63 +++++++++++++++--------------\n unix-stream-server.h | 7 +++-\n 6 files changed, 86 insertions(+), 60 deletions(-)\n\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c\nindex b0264ca6cdc6..55cbb346328a 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -729,44 +729,51 @@ static void *accept_thread_proc(void *_accept_thread_data)\n */\n #define LISTEN_BACKLOG (50)\n \n-static struct unix_stream_server_socket *create_listener_socket(\n+static int create_listener_socket(\n \tconst char *path,\n-\tconst struct ipc_server_opts *ipc_opts)\n+\tconst struct ipc_server_opts *ipc_opts,\n+\tstruct unix_stream_server_socket **new_server_socket)\n {\n \tstruct unix_stream_server_socket *server_socket = NULL;\n \tstruct unix_stream_listen_opts uslg_opts = UNIX_STREAM_LISTEN_OPTS_INIT;\n+\tint ret;\n \n \tuslg_opts.listen_backlog_size = LISTEN_BACKLOG;\n \tuslg_opts.disallow_chdir = ipc_opts->uds_disallow_chdir;\n \n-\tserver_socket = unix_stream_server__listen_with_lock(path, &uslg_opts);\n-\tif (!server_socket)\n-\t\treturn NULL;\n+\tret = unix_stream_server__create(path, &uslg_opts, &server_socket);\n+\tif (ret)\n+\t\treturn ret;\n \n \tif (set_socket_blocking_flag(server_socket->fd_socket, 1)) {\n \t\tint saved_errno = errno;\n-\t\terror_errno(_(\"could not set listener socket nonblocking '%s'\"),\n-\t\t\t path);\n \t\tunix_stream_server__free(server_socket);\n \t\terrno = saved_errno;\n-\t\treturn NULL;\n+\t\treturn -1;\n \t}\n \n+\t*new_server_socket = server_socket;\n+\n \ttrace2_data_string(\"ipc-server\", NULL, \"listen-with-lock\", path);\n-\treturn server_socket;\n+\treturn 0;\n }\n \n-static struct unix_stream_server_socket *setup_listener_socket(\n+static int setup_listener_socket(\n \tconst char *path,\n-\tconst struct ipc_server_opts *ipc_opts)\n+\tconst struct ipc_server_opts *ipc_opts,\n+\tstruct unix_stream_server_socket **new_server_socket)\n {\n-\tstruct unix_stream_server_socket *server_socket;\n+\tint ret, saved_errno;\n \n \ttrace2_region_enter(\"ipc-server\", \"create-listener_socket\", NULL);\n-\tserver_socket = create_listener_socket(path, ipc_opts);\n+\n+\tret = create_listener_socket(path, ipc_opts, new_server_socket);\n+\n+\tsaved_errno = errno;\n \ttrace2_region_leave(\"ipc-server\", \"create-listener_socket\", NULL);\n+\terrno = saved_errno;\n \n-\treturn server_socket;\n+\treturn ret;\n }\n \n /*\n@@ -781,6 +788,7 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \tstruct ipc_server_data *server_data;\n \tint sv[2];\n \tint k;\n+\tint ret;\n \tint nr_threads = opts->nr_threads;\n \n \t*returned_server_data = NULL;\n@@ -792,25 +800,23 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \t * connection or a shutdown request without spinning.\n \t */\n \tif (socketpair(AF_UNIX, SOCK_STREAM, 0, sv) < 0)\n-\t\treturn error_errno(_(\"could not create socketpair for '%s'\"),\n-\t\t\t\t path);\n+\t\treturn -1;\n \n \tif (set_socket_blocking_flag(sv[1], 1)) {\n \t\tint saved_errno = errno;\n \t\tclose(sv[0]);\n \t\tclose(sv[1]);\n \t\terrno = saved_errno;\n-\t\treturn error_errno(_(\"making socketpair nonblocking '%s'\"),\n-\t\t\t\t path);\n+\t\treturn -1;\n \t}\n \n-\tserver_socket = setup_listener_socket(path, opts);\n-\tif (!server_socket) {\n+\tret = setup_listener_socket(path, opts, &server_socket);\n+\tif (ret) {\n \t\tint saved_errno = errno;\n \t\tclose(sv[0]);\n \t\tclose(sv[1]);\n \t\terrno = saved_errno;\n-\t\treturn -1;\n+\t\treturn ret;\n \t}\n \n \tserver_data = xcalloc(1, sizeof(*server_data));\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex f0cfbf9d15c3..5fd5960a1328 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -614,14 +614,16 @@ int ipc_server_run_async(struct ipc_server_data **returned_server_data,\n \t*returned_server_data = NULL;\n \n \tret = initialize_pipe_name(path, wpath, ARRAY_SIZE(wpath));\n-\tif (ret < 0)\n-\t\treturn error(\n-\t\t\t_(\"could not create normalized wchar_t path for '%s'\"),\n-\t\t\tpath);\n+\tif (ret < 0) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n \n \thPipeFirst = create_new_pipe(wpath, 1);\n-\tif (hPipeFirst == INVALID_HANDLE_VALUE)\n-\t\treturn error(_(\"IPC server already running on '%s'\"), path);\n+\tif (hPipeFirst == INVALID_HANDLE_VALUE) {\n+\t\terrno = EADDRINUSE;\n+\t\treturn -2;\n+\t}\n \n \tserver_data = xcalloc(1, sizeof(*server_data));\n \tserver_data->magic = MAGIC_SERVER_DATA;\ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex f7e72e966f9a..dc3606e30bd6 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -178,6 +178,8 @@ struct ipc_server_opts\n *\n * Returns 0 if the asynchronous server pool was started successfully.\n * Returns -1 if not.\n+ * Returns -2 if we could not startup because another server is using\n+ * the socket or named pipe.\n *\n * When a client IPC message is received, the `application_cb` will be\n * called (possibly on a random thread) to handle the message and\n@@ -217,6 +219,8 @@ void ipc_server_free(struct ipc_server_data *server_data);\n *\n * Returns 0 after the server has completed successfully.\n * Returns -1 if the server cannot be started.\n+ * Returns -2 if we could not startup because another server is using\n+ * the socket or named pipe.\n *\n * When a client IPC message is received, the `application_cb` will be\n * called (possibly on a random thread) to handle the message and\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex d67eaa9a6ecc..d0e6127528a7 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -219,6 +219,8 @@ static int test_app_cb(void *application_data,\n */\n static int daemon__run_server(const char *path, int argc, const char **argv)\n {\n+\tint ret;\n+\n \tstruct ipc_server_opts opts = {\n \t\t.nr_threads = 5\n \t};\n@@ -243,7 +245,13 @@ static int daemon__run_server(const char *path, int argc, const char **argv)\n \t * instance data, so pass an arbitrary pointer (that we'll later\n \t * verify made the round trip).\n \t */\n-\treturn ipc_server_run(path, &opts, test_app_cb, (void*)&my_app_data);\n+\tret = ipc_server_run(path, &opts, test_app_cb, (void*)&my_app_data);\n+\tif (ret == -2)\n+\t\terror(_(\"socket/pipe already in use: '%s'\"), path);\n+\telse if (ret == -1)\n+\t\terror_errno(_(\"could not start server on: '%s'\"), path);\n+\n+\treturn ret;\n }\n \n #ifndef GIT_WINDOWS_NATIVE\ndiff --git a/unix-stream-server.c b/unix-stream-server.c\nindex ad175ba731ca..f00298ca7ec3 100644\n--- a/unix-stream-server.c\n+++ b/unix-stream-server.c\n@@ -5,41 +5,44 @@\n \n #define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)\n \n+/*\n+ * Try to connect to a unix domain socket at `path` (if it exists) and\n+ * see if there is a server listening.\n+ *\n+ * We don't know if the socket exists, whether a server died and\n+ * failed to cleanup, or whether we have a live server listening, so\n+ * we \"poke\" it.\n+ *\n+ * We immediately hangup without sending/receiving any data because we\n+ * don't know anything about the protocol spoken and don't want to\n+ * block while writing/reading data. It is sufficient to just know\n+ * that someone is listening.\n+ */\n static int is_another_server_alive(const char *path,\n \t\t\t\t const struct unix_stream_listen_opts *opts)\n {\n-\tstruct stat st;\n-\tint fd;\n-\n-\tif (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {\n-\t\t/*\n-\t\t * A socket-inode exists on disk at `path`, but we\n-\t\t * don't know whether it belongs to an active server\n-\t\t * or whether the last server died without cleaning\n-\t\t * up.\n-\t\t *\n-\t\t * Poke it with a trivial connection to try to find\n-\t\t * out.\n-\t\t */\n-\t\tfd = unix_stream_connect(path, opts->disallow_chdir);\n-\t\tif (fd >= 0) {\n-\t\t\tclose(fd);\n-\t\t\treturn 1;\n-\t\t}\n+\tint fd = unix_stream_connect(path, opts->disallow_chdir);\n+\n+\tif (fd >= 0) {\n+\t\tclose(fd);\n+\t\treturn 1;\n \t}\n \n \treturn 0;\n }\n \n-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n+int unix_stream_server__create(\n \tconst char *path,\n-\tconst struct unix_stream_listen_opts *opts)\n+\tconst struct unix_stream_listen_opts *opts,\n+\tstruct unix_stream_server_socket **new_server_socket)\n {\n \tstruct lock_file lock = LOCK_INIT;\n \tlong timeout;\n \tint fd_socket;\n \tstruct unix_stream_server_socket *server_socket;\n \n+\t*new_server_socket = NULL;\n+\n \ttimeout = opts->timeout_ms;\n \tif (opts->timeout_ms <= 0)\n \t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n@@ -47,20 +50,17 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n \t/*\n \t * Create a lock at \".lock\" if we can.\n \t */\n-\tif (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {\n-\t\terror_errno(_(\"could not lock listener socket '%s'\"), path);\n-\t\treturn NULL;\n-\t}\n+\tif (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0)\n+\t\treturn -1;\n \n \t/*\n \t * If another server is listening on \"\" give up. We do not\n \t * want to create a socket and steal future connections from them.\n \t */\n \tif (is_another_server_alive(path, opts)) {\n-\t\terrno = EADDRINUSE;\n-\t\terror_errno(_(\"listener socket already in use '%s'\"), path);\n \t\trollback_lock_file(&lock);\n-\t\treturn NULL;\n+\t\terrno = EADDRINUSE;\n+\t\treturn -2;\n \t}\n \n \t/*\n@@ -68,9 +68,10 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n \t */\n \tfd_socket = unix_stream_listen(path, opts);\n \tif (fd_socket < 0) {\n-\t\terror_errno(_(\"could not create listener socket '%s'\"), path);\n+\t\tint saved_errno = errno;\n \t\trollback_lock_file(&lock);\n-\t\treturn NULL;\n+\t\terrno = saved_errno;\n+\t\treturn -1;\n \t}\n \n \tserver_socket = xcalloc(1, sizeof(*server_socket));\n@@ -78,6 +79,8 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n \tserver_socket->fd_socket = fd_socket;\n \tlstat(path, &server_socket->st_socket);\n \n+\t*new_server_socket = server_socket;\n+\n \t/*\n \t * Always rollback (just delete) \".lock\" because we already created\n \t * \"\" as a socket and do not want to commit_lock to do the atomic\n@@ -85,7 +88,7 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n \t */\n \trollback_lock_file(&lock);\n \n-\treturn server_socket;\n+\treturn 0;\n }\n \n void unix_stream_server__free(\ndiff --git a/unix-stream-server.h b/unix-stream-server.h\nindex 745849e31c30..f7e76dec59ac 100644\n--- a/unix-stream-server.h\n+++ b/unix-stream-server.h\n@@ -12,10 +12,13 @@ struct unix_stream_server_socket {\n /*\n * Create a Unix Domain Socket at the given path under the protection\n * of a '.lock' lockfile.\n+ *\n+ * Returns 0 on success, -1 on error, -2 if socket is in use.\n */\n-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n+int unix_stream_server__create(\n \tconst char *path,\n-\tconst struct unix_stream_listen_opts *opts);\n+\tconst struct unix_stream_listen_opts *opts,\n+\tstruct unix_stream_server_socket **server_socket);\n \n /*\n * Close and delete the socket.\n-- \ngitgitgadget\n\n"},{"id":"418251","messageId":"6ef867bf37d366071d5f0f101e7430d859f529b5.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 2/8] unix-socket: simplify initialization of unix_stream_listen_opts","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:21Z","receivedAt":"2021-03-04T20:19:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nChange the public initialization of `struct unix_stream_listen_opts`\nto be all zeroes. Hide the default values for the timeout and backlog\nvalues inside `unix-socket.c`.\n\nSigned-off-by: Jeff Hostetler \n---\n unix-socket.c | 11 +++++++++--\n unix-socket.h | 7 ++-----\n 2 files changed, 11 insertions(+), 7 deletions(-)\n\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 647bbde37f97..c9ea1de43bd2 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -2,6 +2,9 @@\n #include \"lockfile.h\"\n #include \"unix-socket.h\"\n \n+#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)\n+#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)\n+\n static int chdir_len(const char *orig, int len)\n {\n \tchar *path = xmemdupz(orig, len);\n@@ -165,14 +168,18 @@ struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n \tconst struct unix_stream_listen_opts *opts)\n {\n \tstruct lock_file lock = LOCK_INIT;\n+\tlong timeout;\n \tint fd_socket;\n \tstruct unix_stream_server_socket *server_socket;\n \n+\ttimeout = opts->timeout_ms;\n+\tif (opts->timeout_ms <= 0)\n+\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n+\n \t/*\n \t * Create a lock at \".lock\" if we can.\n \t */\n-\tif (hold_lock_file_for_update_timeout(&lock, path, 0,\n-\t\t\t\t\t opts->timeout_ms) < 0) {\n+\tif (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {\n \t\terror_errno(_(\"could not lock listener socket '%s'\"), path);\n \t\treturn NULL;\n \t}\ndiff --git a/unix-socket.h b/unix-socket.h\nindex 8faf5b692f90..bec925ee0213 100644\n--- a/unix-socket.h\n+++ b/unix-socket.h\n@@ -7,13 +7,10 @@ struct unix_stream_listen_opts {\n \tunsigned int disallow_chdir:1;\n };\n \n-#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)\n-#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)\n-\n #define UNIX_STREAM_LISTEN_OPTS_INIT \\\n { \\\n-\t.timeout_ms = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT, \\\n-\t.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \\\n+\t.timeout_ms = 0, \\\n+\t.listen_backlog_size = 0, \\\n \t.disallow_chdir = 0, \\\n }\n \n-- \ngitgitgadget\n\n"},{"id":"418252","messageId":"4ae4429a70e408a1ab52a59370a4e6d10ca9a0fd.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 3/8] unix-stream-server: create unix-stream-server.c","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:22Z","receivedAt":"2021-03-04T20:19:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nMove unix_stream_server__listen_with_lock() and associated routines\nand data types out of unix-socket.[ch] and into their own source file.\n\nWe added code to safely create a Unix domain socket using a lockfile in:\n9406d12e14 (unix-socket: create `unix_stream_server__listen_with_lock()`,\n2021-02-13). This code conceptually exists at a higher-level than the\ncore unix_stream_connect() and unix_stream_listen() routines that it\nconsumes and should be isolated for clarity.\n\nSigned-off-by: Jeff Hostetler \n---\n Makefile | 1 +\n compat/simple-ipc/ipc-unix-socket.c | 1 +\n contrib/buildsystems/CMakeLists.txt | 2 +-\n unix-socket.c | 120 ---------------------------\n unix-socket.h | 26 ------\n unix-stream-server.c | 124 ++++++++++++++++++++++++++++\n unix-stream-server.h | 32 +++++++\n 7 files changed, 159 insertions(+), 147 deletions(-)\n create mode 100644 unix-stream-server.c\n create mode 100644 unix-stream-server.h\n\ndiff --git a/Makefile b/Makefile\nindex 93f2e7ca9e1f..dfc37b0480e4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1678,6 +1678,7 @@ ifdef NO_UNIX_SOCKETS\n \tBASIC_CFLAGS += -DNO_UNIX_SOCKETS\n else\n \tLIB_OBJS += unix-socket.o\n+\tLIB_OBJS += unix-stream-server.o\n \tLIB_OBJS += compat/simple-ipc/ipc-shared.o\n \tLIB_OBJS += compat/simple-ipc/ipc-unix-socket.o\n endif\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c\nindex b7fd0b34329e..b0264ca6cdc6 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -4,6 +4,7 @@\n #include \"pkt-line.h\"\n #include \"thread-utils.h\"\n #include \"unix-socket.h\"\n+#include \"unix-stream-server.h\"\n \n #ifdef NO_UNIX_SOCKETS\n #error compat/simple-ipc/ipc-unix-socket.c requires Unix sockets\ndiff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt\nindex 4c27a373414a..629a1817459b 100644\n--- a/contrib/buildsystems/CMakeLists.txt\n+++ b/contrib/buildsystems/CMakeLists.txt\n@@ -243,7 +243,7 @@ if(CMAKE_SYSTEM_NAME STREQUAL \"Windows\")\n \n elseif(CMAKE_SYSTEM_NAME STREQUAL \"Linux\")\n \tadd_compile_definitions(PROCFS_EXECUTABLE_PATH=\"/proc/self/exe\" HAVE_DEV_TTY )\n-\tlist(APPEND compat_SOURCES unix-socket.c)\n+\tlist(APPEND compat_SOURCES unix-socket.c unix-stream-server.c)\n endif()\n \n if(CMAKE_SYSTEM_NAME STREQUAL \"Windows\")\ndiff --git a/unix-socket.c b/unix-socket.c\nindex c9ea1de43bd2..e0be1badb58d 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -1,8 +1,6 @@\n #include \"cache.h\"\n-#include \"lockfile.h\"\n #include \"unix-socket.h\"\n \n-#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)\n #define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)\n \n static int chdir_len(const char *orig, int len)\n@@ -136,121 +134,3 @@ int unix_stream_listen(const char *path,\n \terrno = saved_errno;\n \treturn -1;\n }\n-\n-static int is_another_server_alive(const char *path,\n-\t\t\t\t const struct unix_stream_listen_opts *opts)\n-{\n-\tstruct stat st;\n-\tint fd;\n-\n-\tif (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {\n-\t\t/*\n-\t\t * A socket-inode exists on disk at `path`, but we\n-\t\t * don't know whether it belongs to an active server\n-\t\t * or whether the last server died without cleaning\n-\t\t * up.\n-\t\t *\n-\t\t * Poke it with a trivial connection to try to find\n-\t\t * out.\n-\t\t */\n-\t\tfd = unix_stream_connect(path, opts->disallow_chdir);\n-\t\tif (fd >= 0) {\n-\t\t\tclose(fd);\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n-\n-\treturn 0;\n-}\n-\n-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n-\tconst char *path,\n-\tconst struct unix_stream_listen_opts *opts)\n-{\n-\tstruct lock_file lock = LOCK_INIT;\n-\tlong timeout;\n-\tint fd_socket;\n-\tstruct unix_stream_server_socket *server_socket;\n-\n-\ttimeout = opts->timeout_ms;\n-\tif (opts->timeout_ms <= 0)\n-\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n-\n-\t/*\n-\t * Create a lock at \".lock\" if we can.\n-\t */\n-\tif (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {\n-\t\terror_errno(_(\"could not lock listener socket '%s'\"), path);\n-\t\treturn NULL;\n-\t}\n-\n-\t/*\n-\t * If another server is listening on \"\" give up. We do not\n-\t * want to create a socket and steal future connections from them.\n-\t */\n-\tif (is_another_server_alive(path, opts)) {\n-\t\terrno = EADDRINUSE;\n-\t\terror_errno(_(\"listener socket already in use '%s'\"), path);\n-\t\trollback_lock_file(&lock);\n-\t\treturn NULL;\n-\t}\n-\n-\t/*\n-\t * Create and bind to a Unix domain socket at \"\".\n-\t */\n-\tfd_socket = unix_stream_listen(path, opts);\n-\tif (fd_socket < 0) {\n-\t\terror_errno(_(\"could not create listener socket '%s'\"), path);\n-\t\trollback_lock_file(&lock);\n-\t\treturn NULL;\n-\t}\n-\n-\tserver_socket = xcalloc(1, sizeof(*server_socket));\n-\tserver_socket->path_socket = strdup(path);\n-\tserver_socket->fd_socket = fd_socket;\n-\tlstat(path, &server_socket->st_socket);\n-\n-\t/*\n-\t * Always rollback (just delete) \".lock\" because we already created\n-\t * \"\" as a socket and do not want to commit_lock to do the atomic\n-\t * rename trick.\n-\t */\n-\trollback_lock_file(&lock);\n-\n-\treturn server_socket;\n-}\n-\n-void unix_stream_server__free(\n-\tstruct unix_stream_server_socket *server_socket)\n-{\n-\tif (!server_socket)\n-\t\treturn;\n-\n-\tif (server_socket->fd_socket >= 0) {\n-\t\tif (!unix_stream_server__was_stolen(server_socket))\n-\t\t\tunlink(server_socket->path_socket);\n-\t\tclose(server_socket->fd_socket);\n-\t}\n-\n-\tfree(server_socket->path_socket);\n-\tfree(server_socket);\n-}\n-\n-int unix_stream_server__was_stolen(\n-\tstruct unix_stream_server_socket *server_socket)\n-{\n-\tstruct stat st_now;\n-\n-\tif (!server_socket)\n-\t\treturn 0;\n-\n-\tif (lstat(server_socket->path_socket, &st_now) == -1)\n-\t\treturn 1;\n-\n-\tif (st_now.st_ino != server_socket->st_socket.st_ino)\n-\t\treturn 1;\n-\n-\t/* We might also consider the ctime on some platforms. */\n-\n-\treturn 0;\n-}\ndiff --git a/unix-socket.h b/unix-socket.h\nindex bec925ee0213..b079e79cb3e1 100644\n--- a/unix-socket.h\n+++ b/unix-socket.h\n@@ -18,30 +18,4 @@ int unix_stream_connect(const char *path, int disallow_chdir);\n int unix_stream_listen(const char *path,\n \t\t const struct unix_stream_listen_opts *opts);\n \n-struct unix_stream_server_socket {\n-\tchar *path_socket;\n-\tstruct stat st_socket;\n-\tint fd_socket;\n-};\n-\n-/*\n- * Create a Unix Domain Socket at the given path under the protection\n- * of a '.lock' lockfile.\n- */\n-struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n-\tconst char *path,\n-\tconst struct unix_stream_listen_opts *opts);\n-\n-/*\n- * Close and delete the socket.\n- */\n-void unix_stream_server__free(\n-\tstruct unix_stream_server_socket *server_socket);\n-\n-/*\n- * Return 1 if the inode of the pathname to our socket changes.\n- */\n-int unix_stream_server__was_stolen(\n-\tstruct unix_stream_server_socket *server_socket);\n-\n #endif /* UNIX_SOCKET_H */\ndiff --git a/unix-stream-server.c b/unix-stream-server.c\nnew file mode 100644\nindex 000000000000..ad175ba731ca\n--- /dev/null\n+++ b/unix-stream-server.c\n@@ -0,0 +1,124 @@\n+#include \"cache.h\"\n+#include \"lockfile.h\"\n+#include \"unix-socket.h\"\n+#include \"unix-stream-server.h\"\n+\n+#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)\n+\n+static int is_another_server_alive(const char *path,\n+\t\t\t\t const struct unix_stream_listen_opts *opts)\n+{\n+\tstruct stat st;\n+\tint fd;\n+\n+\tif (!lstat(path, &st) && S_ISSOCK(st.st_mode)) {\n+\t\t/*\n+\t\t * A socket-inode exists on disk at `path`, but we\n+\t\t * don't know whether it belongs to an active server\n+\t\t * or whether the last server died without cleaning\n+\t\t * up.\n+\t\t *\n+\t\t * Poke it with a trivial connection to try to find\n+\t\t * out.\n+\t\t */\n+\t\tfd = unix_stream_connect(path, opts->disallow_chdir);\n+\t\tif (fd >= 0) {\n+\t\t\tclose(fd);\n+\t\t\treturn 1;\n+\t\t}\n+\t}\n+\n+\treturn 0;\n+}\n+\n+struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n+\tconst char *path,\n+\tconst struct unix_stream_listen_opts *opts)\n+{\n+\tstruct lock_file lock = LOCK_INIT;\n+\tlong timeout;\n+\tint fd_socket;\n+\tstruct unix_stream_server_socket *server_socket;\n+\n+\ttimeout = opts->timeout_ms;\n+\tif (opts->timeout_ms <= 0)\n+\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n+\n+\t/*\n+\t * Create a lock at \".lock\" if we can.\n+\t */\n+\tif (hold_lock_file_for_update_timeout(&lock, path, 0, timeout) < 0) {\n+\t\terror_errno(_(\"could not lock listener socket '%s'\"), path);\n+\t\treturn NULL;\n+\t}\n+\n+\t/*\n+\t * If another server is listening on \"\" give up. We do not\n+\t * want to create a socket and steal future connections from them.\n+\t */\n+\tif (is_another_server_alive(path, opts)) {\n+\t\terrno = EADDRINUSE;\n+\t\terror_errno(_(\"listener socket already in use '%s'\"), path);\n+\t\trollback_lock_file(&lock);\n+\t\treturn NULL;\n+\t}\n+\n+\t/*\n+\t * Create and bind to a Unix domain socket at \"\".\n+\t */\n+\tfd_socket = unix_stream_listen(path, opts);\n+\tif (fd_socket < 0) {\n+\t\terror_errno(_(\"could not create listener socket '%s'\"), path);\n+\t\trollback_lock_file(&lock);\n+\t\treturn NULL;\n+\t}\n+\n+\tserver_socket = xcalloc(1, sizeof(*server_socket));\n+\tserver_socket->path_socket = strdup(path);\n+\tserver_socket->fd_socket = fd_socket;\n+\tlstat(path, &server_socket->st_socket);\n+\n+\t/*\n+\t * Always rollback (just delete) \".lock\" because we already created\n+\t * \"\" as a socket and do not want to commit_lock to do the atomic\n+\t * rename trick.\n+\t */\n+\trollback_lock_file(&lock);\n+\n+\treturn server_socket;\n+}\n+\n+void unix_stream_server__free(\n+\tstruct unix_stream_server_socket *server_socket)\n+{\n+\tif (!server_socket)\n+\t\treturn;\n+\n+\tif (server_socket->fd_socket >= 0) {\n+\t\tif (!unix_stream_server__was_stolen(server_socket))\n+\t\t\tunlink(server_socket->path_socket);\n+\t\tclose(server_socket->fd_socket);\n+\t}\n+\n+\tfree(server_socket->path_socket);\n+\tfree(server_socket);\n+}\n+\n+int unix_stream_server__was_stolen(\n+\tstruct unix_stream_server_socket *server_socket)\n+{\n+\tstruct stat st_now;\n+\n+\tif (!server_socket)\n+\t\treturn 0;\n+\n+\tif (lstat(server_socket->path_socket, &st_now) == -1)\n+\t\treturn 1;\n+\n+\tif (st_now.st_ino != server_socket->st_socket.st_ino)\n+\t\treturn 1;\n+\n+\t/* We might also consider the ctime on some platforms. */\n+\n+\treturn 0;\n+}\ndiff --git a/unix-stream-server.h b/unix-stream-server.h\nnew file mode 100644\nindex 000000000000..745849e31c30\n--- /dev/null\n+++ b/unix-stream-server.h\n@@ -0,0 +1,32 @@\n+#ifndef UNIX_STREAM_SERVER_H\n+#define UNIX_STREAM_SERVER_H\n+\n+#include \"unix-socket.h\"\n+\n+struct unix_stream_server_socket {\n+\tchar *path_socket;\n+\tstruct stat st_socket;\n+\tint fd_socket;\n+};\n+\n+/*\n+ * Create a Unix Domain Socket at the given path under the protection\n+ * of a '.lock' lockfile.\n+ */\n+struct unix_stream_server_socket *unix_stream_server__listen_with_lock(\n+\tconst char *path,\n+\tconst struct unix_stream_listen_opts *opts);\n+\n+/*\n+ * Close and delete the socket.\n+ */\n+void unix_stream_server__free(\n+\tstruct unix_stream_server_socket *server_socket);\n+\n+/*\n+ * Return 1 if the inode of the pathname to our socket changes.\n+ */\n+int unix_stream_server__was_stolen(\n+\tstruct unix_stream_server_socket *server_socket);\n+\n+#endif /* UNIX_STREAM_SERVER_H */\n-- \ngitgitgadget\n\n"},{"id":"418250","messageId":"89100959528c6a3c16622cf86e6920273f5ed2d3.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 5/8] unix-stream-server: add st_dev and st_mode to socket stolen checks","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:24Z","receivedAt":"2021-03-04T20:19:03Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nWhen checking to see if our unix domain socket was stolen, also check\nwhether the st.st_dev and st.st_mode fields changed (in addition to\nthe st.st_ino field).\n\nThe inode by itself is not unique; it is only unique on a given\ndevice.\n\nSigned-off-by: Jeff Hostetler \n---\n unix-stream-server.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/unix-stream-server.c b/unix-stream-server.c\nindex f00298ca7ec3..366ece69306b 100644\n--- a/unix-stream-server.c\n+++ b/unix-stream-server.c\n@@ -120,8 +120,11 @@ int unix_stream_server__was_stolen(\n \n \tif (st_now.st_ino != server_socket->st_socket.st_ino)\n \t\treturn 1;\n+\tif (st_now.st_dev != server_socket->st_socket.st_dev)\n+\t\treturn 1;\n \n-\t/* We might also consider the ctime on some platforms. */\n+\tif (!S_ISSOCK(st_now.st_mode))\n+\t\treturn 1;\n \n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"418253","messageId":"d6ea7688dfb2536312b627f7ed47ff7f42091f60.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 1/8] pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:20Z","receivedAt":"2021-03-04T20:19:03Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nRemove the scratch buffer argument from `write_packetized_from_fd_no_flush()`\nand allocate a temporary buffer within the function.\n\nIn 3e4447f1ea (pkt-line: eliminate the need for static buffer in\npacket_write_gently(), 2021-02-13) we added the argument in order to\nget rid of a static buffer to make the routine safe in multi-threaded\ncontexts and to avoid putting a very large buffer on the stack. This\ndelegated the problem to the caller who has more knowledge of the use\ncase and can best decide the most efficient way to provide such a\nbuffer.\n\nHowever, in practice, the (currently, only) caller just created the\nbuffer on the stack, so we might as well just allocate it within the\nfunction and restore the original API.\n\nSigned-off-by: Jeff Hostetler \n---\n convert.c | 7 +++----\n pkt-line.c | 19 +++++++++++--------\n pkt-line.h | 6 +-----\n 3 files changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 9f44f00d841f..516f1095b06e 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -883,10 +883,9 @@ static int apply_multi_file_filter(const char *path, const char *src, size_t len\n \tif (err)\n \t\tgoto done;\n \n-\tif (fd >= 0) {\n-\t\tstruct packet_scratch_space scratch;\n-\t\terr = write_packetized_from_fd_no_flush(fd, process->in, &scratch);\n-\t} else\n+\tif (fd >= 0)\n+\t\terr = write_packetized_from_fd_no_flush(fd, process->in);\n+\telse\n \t\terr = write_packetized_from_buf_no_flush(src, len, process->in);\n \tif (err)\n \t\tgoto done;\ndiff --git a/pkt-line.c b/pkt-line.c\nindex 18ecad65e08c..c933bb95586d 100644\n--- a/pkt-line.c\n+++ b/pkt-line.c\n@@ -250,22 +250,25 @@ void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len)\n \tpacket_trace(data, len, 1);\n }\n \n-int write_packetized_from_fd_no_flush(int fd_in, int fd_out,\n-\t\t\t\t struct packet_scratch_space *scratch)\n+int write_packetized_from_fd_no_flush(int fd_in, int fd_out)\n {\n \tint err = 0;\n \tssize_t bytes_to_write;\n+\tchar *buf = xmalloc(LARGE_PACKET_DATA_MAX);\n \n \twhile (!err) {\n-\t\tbytes_to_write = xread(fd_in, scratch->buffer,\n-\t\t\t\t sizeof(scratch->buffer));\n-\t\tif (bytes_to_write < 0)\n-\t\t\treturn COPY_READ_ERROR;\n+\t\tbytes_to_write = xread(fd_in, buf, LARGE_PACKET_DATA_MAX);\n+\t\tif (bytes_to_write < 0) {\n+\t\t\terr = COPY_READ_ERROR;\n+\t\t\tbreak;\n+\t\t}\n \t\tif (bytes_to_write == 0)\n \t\t\tbreak;\n-\t\terr = packet_write_gently(fd_out, scratch->buffer,\n-\t\t\t\t\t bytes_to_write);\n+\t\terr = packet_write_gently(fd_out, buf, bytes_to_write);\n \t}\n+\n+\tfree(buf);\n+\n \treturn err;\n }\n \ndiff --git a/pkt-line.h b/pkt-line.h\nindex e347fe46832a..54eec72dc0af 100644\n--- a/pkt-line.h\n+++ b/pkt-line.h\n@@ -8,10 +8,6 @@\n #define LARGE_PACKET_MAX 65520\n #define LARGE_PACKET_DATA_MAX (LARGE_PACKET_MAX - 4)\n \n-struct packet_scratch_space {\n-\tchar buffer[LARGE_PACKET_DATA_MAX]; /* does not include header bytes */\n-};\n-\n /*\n * Write a packetized stream, where each line is preceded by\n * its length (including the header) as a 4-byte hex number.\n@@ -39,7 +35,7 @@ void packet_buf_write(struct strbuf *buf, const char *fmt, ...) __attribute__((f\n void packet_buf_write_len(struct strbuf *buf, const char *data, size_t len);\n int packet_flush_gently(int fd);\n int packet_write_fmt_gently(int fd, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n-int write_packetized_from_fd_no_flush(int fd_in, int fd_out, struct packet_scratch_space *scratch);\n+int write_packetized_from_fd_no_flush(int fd_in, int fd_out);\n int write_packetized_from_buf_no_flush(const char *src_in, size_t len, int fd_out);\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"418254","messageId":"eadde22c735e45e114776bc6e2e3affecb14cdcb.1614889047.git.gitgitgadget@gmail.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"[PATCH 8/8] simple-ipc: update design documentation with more details","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-03-04T20:17:27Z","receivedAt":"2021-03-04T20:19:48Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nSigned-off-by: Jeff Hostetler \n---\n Documentation/technical/api-simple-ipc.txt | 131 ++++++++++++++++-----\n 1 file changed, 101 insertions(+), 30 deletions(-)\n\ndiff --git a/Documentation/technical/api-simple-ipc.txt b/Documentation/technical/api-simple-ipc.txt\nindex 670a5c163e39..d79ad323e675 100644\n--- a/Documentation/technical/api-simple-ipc.txt\n+++ b/Documentation/technical/api-simple-ipc.txt\n@@ -1,34 +1,105 @@\n-simple-ipc API\n+Simple-IPC API\n ==============\n \n-The simple-ipc API is used to send an IPC message and response between\n-a (presumably) foreground Git client process to a background server or\n-daemon process. The server process must already be running. Multiple\n-client processes can simultaneously communicate with the server\n-process.\n+The Simple-IPC API is a collection of `ipc_` prefixed library routines\n+and a basic communication protocol that allow an IPC-client process to\n+send an application-specific IPC-request message to an IPC-server\n+process and receive an application-specific IPC-response message.\n \n Communication occurs over a named pipe on Windows and a Unix domain\n-socket on other platforms. Clients and the server rendezvous at a\n-previously agreed-to application-specific pathname (which is outside\n-the scope of this design).\n-\n-This IPC mechanism differs from the existing `sub-process.c` model\n-(Documentation/technical/long-running-process-protocol.txt) and used\n-by applications like Git-LFS. In the simple-ipc model the server is\n-assumed to be a very long-running system service. In contrast, in the\n-LFS-style sub-process model the helper is started with the foreground\n-process and exits when the foreground process terminates.\n-\n-How the simple-ipc server is started is also outside the scope of the\n-IPC mechanism. For example, the server might be started during\n-maintenance operations.\n-\n-The IPC protocol consists of a single request message from the client and\n-an optional request message from the server. For simplicity, pkt-line\n-routines are used to hide chunking and buffering concerns. Each side\n-terminates their message with a flush packet.\n-(Documentation/technical/protocol-common.txt)\n-\n-The actual format of the client and server messages is application\n-specific. The IPC layer transmits and receives an opaque buffer without\n-any concern for the content within.\n+socket on other platforms. IPC-clients and IPC-servers rendezvous at\n+a previously agreed-to application-specific pathname (which is outside\n+the scope of this design) that is local to the computer system.\n+\n+The IPC-server routines within the server application process create a\n+thread pool to listen for connections and receive request messages\n+from multiple concurrent IPC-clients. When received, these messages\n+are dispatched up to the server application callbacks for handling.\n+IPC-server routines then incrementally relay responses back to the\n+IPC-client.\n+\n+The IPC-client routines within a client application process connect\n+to the IPC-server and send a request message and wait for a response.\n+When received, the response is returned back the caller.\n+\n+For example, the `fsmonitor--daemon` feature will be built as a server\n+application on top of the IPC-server library routines. It will have\n+threads watching for file system events and a thread pool waiting for\n+client connections. Clients, such as `git status` will request a list\n+of file system events since a point in time and the server will\n+respond with a list of changed files and directories. The formats of\n+the request and response are application-specific; the IPC-client and\n+IPC-server routines treat them as opaque byte streams.\n+\n+\n+Comparison with sub-process model\n+---------------------------------\n+\n+The Simple-IPC mechanism differs from the existing `sub-process.c`\n+model (Documentation/technical/long-running-process-protocol.txt) and\n+used by applications like Git-LFS. In the LFS-style sub-process model\n+the helper is started by the foreground process, communication happens\n+via a pair of file descriptors bound to the stdin/stdout of the\n+sub-process, the sub-process only serves the current foreground\n+process, and the sub-process exits when the foreground process\n+terminates.\n+\n+In the Simple-IPC model the server is a very long-running service. It\n+can service many clients at the same time and has a private socket or\n+named pipe connection to each active client. It might be started\n+(on-demand) by the current client process or it might have been\n+started by a previous client or by the OS at boot time. The server\n+process is not associated with a terminal and it persists after\n+clients terminate. Clients do not have access to the stdin/stdout of\n+the server process and therefore must communicate over sockets or\n+named pipes.\n+\n+\n+Server startup and shutdown\n+---------------------------\n+\n+How an application server based upon IPC-server is started is also\n+outside the scope of the Simple-IPC design and is a property of the\n+application using it. For example, the server might be started or\n+restarted during routine maintenance operations, or it might be\n+started as a system service during the system boot-up sequence, or it\n+might be started on-demand by a foreground Git command when needed.\n+\n+Similarly, server shutdown is a property of the application using\n+the simple-ipc routines. For example, the server might decide to\n+shutdown when idle or only upon explicit request.\n+\n+\n+Simple-IPC protocol\n+-------------------\n+\n+The Simple-IPC protocol consists of a single request message from the\n+client and an optional response message from the server. Both the\n+client and server messages are unlimited in length and are terminated\n+with a flush packet.\n+\n+The pkt-line routines (Documentation/technical/protocol-common.txt)\n+are used to simplify buffer management during message generation,\n+transmission, and reception. A flush packet is used to mark the end\n+of the message. This allows the sender to incrementally generate and\n+transmit the message. It allows the receiver to incrementally receive\n+the message in chunks and to know when they have received the entire\n+message.\n+\n+The actual byte format of the client request and server response\n+messages are application specific. The IPC layer transmits and\n+receives them as opaque byte buffers without any concern for the\n+content within. It is the job of the calling application layer to\n+understand the contents of the request and response messages.\n+\n+\n+Summary\n+-------\n+\n+Conceptually, the Simple-IPC protocol is similar to an HTTP REST\n+request. Clients connect, make an application-specific and\n+stateless request, receive an application-specific\n+response, and disconnect. It is a one round trip facility for\n+querying the server. The Simple-IPC routines hide the socket,\n+named pipe, and thread pool details and allow the application\n+layer to focus on the application at hand.\n-- \ngitgitgadget\n"},{"id":"418270","messageId":"xmqqh7lqcywi.fsf@gitster.c.googlers.com","threadId":"55255","inReplyTo":"d6ea7688dfb2536312b627f7ed47ff7f42091f60.1614889047.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/8] pkt-line: remove buffer arg from write_packetized_from_fd_no_flush()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T22:55:09Z","receivedAt":"2021-03-04T22:55:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" writes:\n\n> From: Jeff Hostetler \n>\n> Remove the scratch buffer argument from `write_packetized_from_fd_no_flush()`\n> and allocate a temporary buffer within the function.\n>\n> In 3e4447f1ea (pkt-line: eliminate the need for static buffer in\n> packet_write_gently(), 2021-02-13) we added the argument in order to\n> get rid of a static buffer to make the routine safe in multi-threaded\n> contexts and to avoid putting a very large buffer on the stack. This\n> delegated the problem to the caller who has more knowledge of the use\n> case and can best decide the most efficient way to provide such a\n> buffer.\n>\n> However, in practice, the (currently, only) caller just created the\n> buffer on the stack, so we might as well just allocate it within the\n> function and restore the original API.\n\nHmph, I would have expected, since we already have changed the\ncallchain to pass the buffer down, that we'd keep the structure and\nupdate the caller to heap-allocate, but I think I like the result of\nthis patch better. It's not like the caller can allocate and reuse\na single buffer with repeated calls to the function; rather, the\ncaller makes a single call that results in relaying many bytes by\nthe helper, all done in a loop, and it is sufficient for the helper\nto allocate/deallocate outside of its loop to reuse the buffer.\n\n"},{"id":"418271","messageId":"xmqqa6ricy2v.fsf@gitster.c.googlers.com","threadId":"55255","inReplyTo":"6ef867bf37d366071d5f0f101e7430d859f529b5.1614889047.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/8] unix-socket: simplify initialization of unix_stream_listen_opts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-04T23:12:56Z","receivedAt":"2021-03-04T23:13:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" writes:\n\n> \tstruct lock_file lock = LOCK_INIT;\n> +\tlong timeout;\n> \tint fd_socket;\n> \tstruct unix_stream_server_socket *server_socket;\n> \n> +\ttimeout = opts->timeout_ms;\n> +\tif (opts->timeout_ms <= 0)\n> +\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n\nIf we have 0 as a special value to tell this API to use the default\nvalue, do we need to treat negative values the same way?\n\nDo we see any value in being able to say \"no timeout---if we can do\nso immediately fine, otherwise return me a failure\"? Deep in the\ncallchain, lockfile.c::lock_file_timeout(), which is the workhorse\nof the feature, notices timeout_ms==0 and makes a direct call to\nlock_file() after all, so we are prepared for such a caller already.\n\nAnd if this is such a caller that may benefit from being able to say\n\"fail if we cannot immediately lock\", perhaps we might want to allow\n0 to be used as a real value and use something else as a signal to\nuse the timeout value determined by the helper as the default.\n\nIOW, I would find the above iffy and prefer any of the following\nover it:\n\n(0)\tif (!opts->timeout_ms)\n\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n\telse if (opts->timeout_ms < 0)\n\t\tBUG(\"...\");\n\n(1)\tif (opts->timeout_ms < 0)\n\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n\n(2)\tif (opts->timeout_ms == -1)\n\t\ttimeout = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT;\n\telse if (opts->timeout_ms < 0)\n\t\tBUG(\"...\");\n\n> diff --git a/unix-socket.h b/unix-socket.h\n> index 8faf5b692f90..bec925ee0213 100644\n> --- a/unix-socket.h\n> +++ b/unix-socket.h\n> @@ -7,13 +7,10 @@ struct unix_stream_listen_opts {\n> \tunsigned int disallow_chdir:1;\n> };\n> \n> -#define DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT (100)\n> -#define DEFAULT_UNIX_STREAM_LISTEN_BACKLOG (5)\n> -\n> #define UNIX_STREAM_LISTEN_OPTS_INIT \\\n> { \\\n> -\t.timeout_ms = DEFAULT_UNIX_STREAM_LISTEN_TIMEOUT, \\\n> -\t.listen_backlog_size = DEFAULT_UNIX_STREAM_LISTEN_BACKLOG, \\\n> +\t.timeout_ms = 0, \\\n> +\t.listen_backlog_size = 0, \\\n> \t.disallow_chdir = 0, \\\n> }\n\nI thought the point of suggested fix was to allow 0-initialize the\nwhole structure, so that we do not have to have the C preprocessor\nmacro UNIX_STREAM_LISTEN_OPTS_INIT at all. I.e. it would allow us\nto do\n\n-\tstruct unix_stream_listen_opts opts = UNIX_STREAM_LISTEN_OPTS_INIT;\n+\tstruct unix_stream_listen_opts opts = { 0 };\n\nin builtin/credential-cache--daemon.c::serve_cache().\n\nIf we cannot use 0 as a special value, however, for the timeout,\nthen we cannot get rid of UNIX_STREAM_LISTEN_OPTS_INIT, but at least\nwe should be able to do\n\n\t#define UNIX_STREAM_LISTEN_OPTS_INIT { .timeout_ms = -1 }\n\nand leave everything else 0-initialized.\n\nThanks.\n"},{"id":"418278","messageId":"xmqqlfb2bg6c.fsf@gitster.c.googlers.com","threadId":"55255","inReplyTo":"pull.893.git.1614889047.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/8] Simple IPC Cleanups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-05T00:24:59Z","receivedAt":"2021-03-05T00:25:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"sparse failed seen, so I tentatively added this on top of your\nseries.\n\nThanks.\n\n t/helper/test-simple-ipc.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 4da63fd30c..42040ef81b 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -227,7 +227,7 @@ struct cl_args\n \tchar bytevalue;\n };\n \n-struct cl_args cl_args = {\n+static struct cl_args cl_args = {\n \t.subcommand = NULL,\n \t.path = \"ipc-test\",\n \t.token = NULL,\n"},{"id":"418342","messageId":"8ea6401c-6ee6-94cb-4e33-9dfffaf466e8@jeffhostetler.com","threadId":"55255","inReplyTo":"xmqqlfb2bg6c.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 0/8] Simple IPC Cleanups","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-03-05T21:34:35Z","receivedAt":"2021-03-05T21:35:15Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/4/21 7:24 PM, Junio C Hamano wrote:\n> sparse failed seen, so I tentatively added this on top of your\n> series.\n> \n> Thanks.\n> \n> t/helper/test-simple-ipc.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\n> index 4da63fd30c..42040ef81b 100644\n> --- a/t/helper/test-simple-ipc.c\n> +++ b/t/helper/test-simple-ipc.c\n> @@ -227,7 +227,7 @@ struct cl_args\n> \tchar bytevalue;\n> };\n> \n> -struct cl_args cl_args = {\n> +static struct cl_args cl_args = {\n> \t.subcommand = NULL,\n> \t.path = \"ipc-test\",\n> \t.token = NULL,\n> \n\nThanks!!\n\nI'll update my copy.\n\nSince things are settling down on the whole simple-ipc series and\nwe are waiting for the current release cycle to complete before\ngoing any further, I'm going to let this series rest for a bit\nin case there are any more comments.\n\nThen I'll combine the 2 parts and rebase/re-roll all of this into\na single series that we can re-eval for \"next\" post 2.31.\n\nJeff\n"},{"id":"418390","messageId":"05f4cb97-5d78-4698-795d-311197052e22@web.de","threadId":"55255","inReplyTo":"89100959528c6a3c16622cf86e6920273f5ed2d3.1614889047.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/8] unix-stream-server: add st_dev and st_mode to socket stolen checks","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-03-06T11:42:20Z","receivedAt":"2021-03-06T11:43:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 04.03.21 um 21:17 schrieb Jeff Hostetler via GitGitGadget:\n> From: Jeff Hostetler \n>\n> When checking to see if our unix domain socket was stolen, also check\n> whether the st.st_dev and st.st_mode fields changed (in addition to\n> the st.st_ino field).\n>\n> The inode by itself is not unique; it is only unique on a given\n> device.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> unix-stream-server.c | 5 ++++-\n> 1 file changed, 4 insertions(+), 1 deletion(-)\n>\n> diff --git a/unix-stream-server.c b/unix-stream-server.c\n> index f00298ca7ec3..366ece69306b 100644\n> --- a/unix-stream-server.c\n> +++ b/unix-stream-server.c\n> @@ -120,8 +120,11 @@ int unix_stream_server__was_stolen(\n>\n> \tif (st_now.st_ino != server_socket->st_socket.st_ino)\n> \t\treturn 1;\n> +\tif (st_now.st_dev != server_socket->st_socket.st_dev)\n> +\t\treturn 1;\n>\n> -\t/* We might also consider the ctime on some platforms. */\n\nWhy remove that comment? (This change is not mentioned in the commit\nmessage.)\n\n> +\tif (!S_ISSOCK(st_now.st_mode))\n> +\t\treturn 1;\n>\n> \treturn 0;\n> }\n>\n\n"},{"id":"418445","messageId":"f74c7b94-0378-edfc-6f34-18c896570a3e@jeffhostetler.com","threadId":"55255","inReplyTo":"05f4cb97-5d78-4698-795d-311197052e22@web.de","subject":"Re: [PATCH 5/8] unix-stream-server: add st_dev and st_mode to socket stolen checks","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-03-08T14:14:48Z","receivedAt":"2021-03-08T14:15:37Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/6/21 6:42 AM, René Scharfe wrote:\n> Am 04.03.21 um 21:17 schrieb Jeff Hostetler via GitGitGadget:\n>> From: Jeff Hostetler \n>>\n>> When checking to see if our unix domain socket was stolen, also check\n>> whether the st.st_dev and st.st_mode fields changed (in addition to\n>> the st.st_ino field).\n>>\n>> The inode by itself is not unique; it is only unique on a given\n>> device.\n>>\n>> Signed-off-by: Jeff Hostetler \n>> ---\n>> unix-stream-server.c | 5 ++++-\n>> 1 file changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/unix-stream-server.c b/unix-stream-server.c\n>> index f00298ca7ec3..366ece69306b 100644\n>> --- a/unix-stream-server.c\n>> +++ b/unix-stream-server.c\n>> @@ -120,8 +120,11 @@ int unix_stream_server__was_stolen(\n>>\n>> \tif (st_now.st_ino != server_socket->st_socket.st_ino)\n>> \t\treturn 1;\n>> +\tif (st_now.st_dev != server_socket->st_socket.st_dev)\n>> +\t\treturn 1;\n>>\n>> -\t/* We might also consider the ctime on some platforms. */\n> \n> Why remove that comment? (This change is not mentioned in the commit\n> message.)\n\nI added it as a TODO to myself thinking that it might give us\nadditional assurances on some platforms while I was originally\ngetting things working. In hindsight (and now that we have the\nlockfile helping us), I didn't think it was actually needed, so\nI removed it.\n\nI didn't think it warranted a mention in the commit message.\n\n> \n>> +\tif (!S_ISSOCK(st_now.st_mode))\n>> +\t\treturn 1;\n>>\n>> \treturn 0;\n>> }\n>>\n> \n"}]}