| Message ID | pull.1040.v2.git.1632152178.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Builtin FSMonitor Part 1 | expand |
On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote: > Here is V2 of Part 1 of my Builtin FSMonitor series. > > Changes since V1 include: > > * Drop the Trace2 memory leak. > * Added a new "child_ready" event to Trace2 as an alternative to the > "child_exit" event for background processes. > * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use > the new "child_ready" event. > * Various minor code and documentation cleanups. I see 7/7 still has a pattern you included only to make a compiler error better. I noted in https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it make the error worse, on at least clang. You didn't note which compiler you were massaging, presumably MSVC. I think that's a relatively small matter, but it *is* one I know about, and there's no reply there, mention of it being unaddressed here or in the commit message. I haven't gone back & re-read v1 and seen if there's more unaddresed feedback from others, instead I wanted to encourage you to provide such a summary. It really helps when a series is re-rolled to aid review both for newcomers and returning reviewers. I really don't care about "getting my way" on such a minor thing. But it is frustrating to have the state of a re-roll be observably indistinguishable from one's E-Mail not having been received, when that's the case you've got to go back and re-read the thread, scour the range-diff, and generalyl do a lot of work that the person doing the re-roll has done, but either didn't keep notes, or didn't share them. Personally I'm in the habit of "flagging" (starring in GMail terms) E-Mails with outstanding unaddresed comments I get on my own topics, then when I re-roll them I look at the thread, and "unwind the stack" as it were by removing flags on E-Mails that I've either addressed via updated commit messages, or added a note to a WIP cover letter. E.g. here (just an example that includes Taylor, since he reviewed v1 here) is a case where Taylor suggested something that I didn't go for, but i'd like to think noting it helped him catch up: https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com/ All the best, just trying to make the reviewer & re-rolling process better for everyone.
On 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote: > >> Here is V2 of Part 1 of my Builtin FSMonitor series. >> >> Changes since V1 include: >> >> * Drop the Trace2 memory leak. >> * Added a new "child_ready" event to Trace2 as an alternative to the >> "child_exit" event for background processes. >> * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use >> the new "child_ready" event. >> * Various minor code and documentation cleanups. > > I see 7/7 still has a pattern you included only to make a compiler error > better. I noted in > https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it > make the error worse, on at least clang. You didn't note which compiler > you were massaging, presumably MSVC. > I've been holding my tongue for days on this issue and hoping a third party would step in an render an opinion one way or another. Too me, a forward declaration seemed like no big deal and it does have value as I tried to explain. And frankly, it felt a little bit like bike-shedding and was trying to avoid that again. The error message I quoted was from Clang v11.0.3. My forward declaration of the function prior to the actual definition of the function causes the compiler to stop at the function definition and complain with a short message saying that the function itself is incorrectly defined and doesn't match the typedef that it is associated with. When I use MSVC I get a similar error at the function definition. When I use GCC I get error messages at both the function definition and the usage in the call. Additionally, the forward declaration states that the function is associated with that typedef (something that is otherwise implicit and may be hard to discover (more on that in a minute)). And it doesn't require a reference to the function pointer (either on the right side of an assignment, a vtable initialization, or passing it in a function call) to flag the error. We always get the error at the point of the definition. The error message in your example is, I feel, worse than mine. It splats 2 different function signatures -- only one of which has the typedef name -- in a large, poorly wrapped brick of text. Yes, your error message does print corresponding arg in the function prototype of "start_bg_command()" that doesn't agree with the symbol used at the call-site, but that is much later than where the actual error occurred. And if the forward declaration were present, you'd already know that back up at the definition, right. Let's look at this from another point of view. Suppose for example we have two function prototypes with the same signature. Perhaps they describe groups of functions with different semantics -- the fact that they have the same argument list and return type is just a coincidence. typedef int(t_fn_1)(int); typedef int(t_fn_2)(int); And then declare one or more instances of functions in those groups: int foo_a(int x) { ... } int foo_b(int x) { ... } int foo_c(int x) { ... } int foo_d(int x) { ... } int foo_e(int x) { ... } int foo_f(int x) { ... } int foo_g(int x) { ... } Which of those functions should be associated with "t_fn_1" and which with "t_fn_2"? Again, they all have the same signature, but different semantics. The author knows when they wrote the code, but it may be hard to automatically determine later. If I then have a function like start_bg_command() that receives a function pointer: int test(..., t_fn_1 *fn, ...) { ... } In C -- even with the use of forward function declarations -- the compiler won't complain if you pass test() a pointer of type t_fn_2 -- as long a t_fn_1 and t_fn_2 have the same signature. But it does give the human a chance to catch the error. Of if we later change the function signature in the t_fn_1 typedef, we will automatically get a list of which of those foo_x functions do and do not need to be updated. Anyway, I've soapboxed on this enough. I think it is a worthy feature for the price. Jeff
On Thu, Sep 23, 2021 at 04:33:20PM +0200, Ævar Arnfjörð Bjarmason wrote: > E.g. here (just an example that includes Taylor, since he reviewed v1 > here) is a case where Taylor suggested something that I didn't go for, > but i'd like to think noting it helped him catch up: > https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com/ It did, but to be honest I would have been totally fine with you mentioning the changes you did incorporate into the rerolled version, and omitting mention of any insignificant suggestions you decided to ignore. In other words, when I got to the same spot in the rerolled version, I would have either thought "looks like Ævar didn't take my suggestion, OK" or not have remembered it in the first place. Either way, the point was trivial enough that I didn't bother to pursue it further in that thread. And I think that's what is happening here, too. Yes, I find the forward declaration useless on GCC, though it appears to be helpful on MSVC and hurtful on clang. Even if it does produce a strictly worse error message on clang, do we really care? It may cause some mild inconvenience for a developer later on, but I find it highly unlikely that it would allow us to ship a bug that wouldn't have been caught one way or another during development. So I was a little disappointed to see such a back-and-forth about this quite trivial point. I realize that I'm piling on here by adding my two-cents, but I think it's worth it to ask ourselves more often what points we're willing to concede and which are worth advocating more strongly for. Thanks, Taylor
On Thu, Sep 23 2021, Jeff Hostetler wrote: > On 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote: >> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote: >> >>> Here is V2 of Part 1 of my Builtin FSMonitor series. >>> >>> Changes since V1 include: >>> >>> * Drop the Trace2 memory leak. >>> * Added a new "child_ready" event to Trace2 as an alternative to the >>> "child_exit" event for background processes. >>> * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use >>> the new "child_ready" event. >>> * Various minor code and documentation cleanups. >> I see 7/7 still has a pattern you included only to make a compiler >> error >> better. I noted in >> https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it >> make the error worse, on at least clang. You didn't note which compiler >> you were massaging, presumably MSVC. >> > > I've been holding my tongue for days on this issue and hoping a third > party would step in an render an opinion one way or another. > > Too me, a forward declaration seemed like no big deal and it does > have value as I tried to explain. And frankly, it felt a little bit > like bike-shedding and was trying to avoid that again. I agree with you that it's no big deal in the end, I thought I made it clear in <87v92r49mt.fsf@evledraar.gmail.com> but the main thing I'm commenting on is not that I or anyone else suggested Y over X, and you said nah and went for X in the end. That's fine, I mean, depending on the comment/issue etc. it's something other reviewers & Junio can draw their own conclusions about. What I am saying that it's much better for review of iterations of patches in general, and especially of a complex multi-part series if reviewers don't have to read the cover letter of vX and wonder what's omitted/unaddressed in the V(X-1) comments, and then go and re-read the discussion themselves. It's not the "nah", but that the "nah" is implicit and only apparent when sending an E-Mail like this. Of course that's never perfect, you can't summarize every point etc. Personally I try to do this, but I've sometimes noticed after the fact that I've gotten it wrong etc. In the end I and I think anyone else offering their time to review things is trying to move the relevant topic forward in one way or another. I'd much rather spend my time on a vX discussing new things & getting the thing closer to merge-able state, than re-reading all of v(X-1) & effectively coming up with my own cover letter summary in my head or in my own notes as I read along. Anyway, sorry about the bikeshedding getting out of hand, and what seems to have been at least partially a misunderstanding in the last couple of E-Mails between us, but the above is all I was going for. > The error message I quoted was from Clang v11.0.3. My forward > declaration of the function prior to the actual definition of > the function causes the compiler to stop at the function definition > and complain with a short message saying that the function itself > is incorrectly defined and doesn't match the typedef that it is > associated with. > > When I use MSVC I get a similar error at the function definition. > > When I use GCC I get error messages at both the function definition > and the usage in the call. > > > Additionally, the forward declaration states that the function is > associated with that typedef (something that is otherwise implicit > and may be hard to discover (more on that in a minute)). > > And it doesn't require a reference to the function pointer (either > on the right side of an assignment, a vtable initialization, or passing > it in a function call) to flag the error. We always get the error > at the point of the definition. > > > The error message in your example is, I feel, worse than mine. > It splats 2 different function signatures -- only one of which has > the typedef name -- in a large, poorly wrapped brick of text. For what it's worth any poor wrapping is my fault, I have a relatively wide terminal and re-wrapped this when composing the E-Mail. I think both GCC & Clang (and most other mature compilers) would give the person getting the error sane wrapping based on their $COLUMNS. > Yes, your error message does print corresponding arg in the function > prototype of "start_bg_command()" that doesn't agree with the symbol > used at the call-site, but that is much later than where the actual > error occurred. And if the forward declaration were present, you'd > already know that back up at the definition, right. > > > Let's look at this from another point of view. > > Suppose for example we have two function prototypes with the same > signature. Perhaps they describe groups of functions with different > semantics -- the fact that they have the same argument list and return > type is just a coincidence. > > typedef int(t_fn_1)(int); > typedef int(t_fn_2)(int); > > And then declare one or more instances of functions in those groups: > > int foo_a(int x) { ... } > int foo_b(int x) { ... } > int foo_c(int x) { ... } > int foo_d(int x) { ... } > int foo_e(int x) { ... } > int foo_f(int x) { ... } > int foo_g(int x) { ... } > > Which of those functions should be associated with "t_fn_1" and which > with "t_fn_2"? Again, they all have the same signature, but different > semantics. The author knows when they wrote the code, but it may be > hard to automatically determine later. > > If I then have a function like start_bg_command() that receives a > function pointer: > > int test(..., t_fn_1 *fn, ...) { ... } > > In C -- even with the use of forward function declarations -- the > compiler won't complain if you pass test() a pointer of type t_fn_2 > -- as long a t_fn_1 and t_fn_2 have the same signature. > > But it does give the human a chance to catch the error. Of if we > later change the function signature in the t_fn_1 typedef, we will > automatically get a list of which of those foo_x functions do and > do not need to be updated. > > > Anyway, I've soapboxed on this enough. I think it is a worthy > feature for the price. Code in git.git generally just declares say an "int foo(int)" and leaves it at passing the "foo", we're not concerned about that "foo" just so happening to be passed to some other interface that takes the same signature, certainly not something within a <1k line t/helper/* file. We all have habits we've picked up from other codebases prior to working on git.git. I'm not arguing that what you're describing is worse in some abtract sense, but that there's a larger value in following conventions within the codebase as Junio noted in his reply.
On 9/23/21 4:47 PM, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Sep 23 2021, Jeff Hostetler wrote: > >> On 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote: >>> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote: >>> >>>> Here is V2 of Part 1 of my Builtin FSMonitor series. >>>> >>>> Changes since V1 include: >>>> >>>> * Drop the Trace2 memory leak. >>>> * Added a new "child_ready" event to Trace2 as an alternative to the >>>> "child_exit" event for background processes. >>>> * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use >>>> the new "child_ready" event. >>>> * Various minor code and documentation cleanups. >>> I see 7/7 still has a pattern you included only to make a compiler >>> error >>> better. I noted in >>> https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it >>> make the error worse, on at least clang. You didn't note which compiler >>> you were massaging, presumably MSVC. >>> >> >> I've been holding my tongue for days on this issue and hoping a third >> party would step in an render an opinion one way or another. >> >> Too me, a forward declaration seemed like no big deal and it does >> have value as I tried to explain. And frankly, it felt a little bit >> like bike-shedding and was trying to avoid that again. > > I agree with you that it's no big deal in the end, > > I thought I made it clear in <87v92r49mt.fsf@evledraar.gmail.com> but > the main thing I'm commenting on is not that I or anyone else suggested > Y over X, and you said nah and went for X in the end. > > That's fine, I mean, depending on the comment/issue etc. it's something > other reviewers & Junio can draw their own conclusions about. > > What I am saying that it's much better for review of iterations of > patches in general, and especially of a complex multi-part series if > reviewers don't have to read the cover letter of vX and wonder what's > omitted/unaddressed in the V(X-1) comments, and then go and re-read the > discussion themselves. It's not the "nah", but that the "nah" is > implicit and only apparent when sending an E-Mail like this. > > Of course that's never perfect, you can't summarize every point > etc. Personally I try to do this, but I've sometimes noticed after the > fact that I've gotten it wrong etc. > > In the end I and I think anyone else offering their time to review > things is trying to move the relevant topic forward in one way or > another. I'd much rather spend my time on a vX discussing new things & > getting the thing closer to merge-able state, than re-reading all of > v(X-1) & effectively coming up with my own cover letter summary in my > head or in my own notes as I read along. > > Anyway, sorry about the bikeshedding getting out of hand, and what seems > to have been at least partially a misunderstanding in the last couple of > E-Mails between us, but the above is all I was going for. Thanks. Yeah, email is a terrible communication medium and prone to misunderstandings. It's easy to forget that at times, since we spend so much time in it. Drafting a v(X+1) cover letter is a bit of an art. It is easy to err on the less-is-better side when trying to decide how much to include to explain the new version vs not wanting to including every little typo or nit. I tend to drop / cross-off issues that I decide to ignore or not act upon rather than report them. However, you're right, I should have included a brief statement about not changing the stuff mentioned in 7/7, since there was a larger conversation around it. Sorry. Jeff
Here is V2 of Part 1 of my Builtin FSMonitor series. Changes since V1 include: * Drop the Trace2 memory leak. * Added a new "child_ready" event to Trace2 as an alternative to the "child_exit" event for background processes. * Convert the Trace2-related NEEDSWORK items in "start_bg_command()" to use the new "child_ready" event. * Various minor code and documentation cleanups. Jeff Hostetler (7): trace2: add trace2_child_ready() to report on background children simple-ipc: preparations for supporting binary messages. simple-ipc: move definition of ipc_active_state outside of ifdef simple-ipc/ipc-win32: add trace2 debugging simple-ipc/ipc-win32: add Windows ACL to named pipe run-command: create start_bg_command t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Documentation/technical/api-trace2.txt | 40 +++++ compat/simple-ipc/ipc-unix-socket.c | 14 +- compat/simple-ipc/ipc-win32.c | 179 +++++++++++++++++-- run-command.c | 129 ++++++++++++++ run-command.h | 57 ++++++ simple-ipc.h | 21 ++- t/helper/test-simple-ipc.c | 233 +++++++------------------ trace2.c | 31 ++++ trace2.h | 25 +++ trace2/tr2_tgt.h | 5 + trace2/tr2_tgt_event.c | 22 +++ trace2/tr2_tgt_normal.c | 14 ++ trace2/tr2_tgt_perf.c | 15 ++ 13 files changed, 587 insertions(+), 198 deletions(-) base-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1040%2Fjeffhostetler%2Fbuiltin-fsmonitor-part1-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1040/jeffhostetler/builtin-fsmonitor-part1-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1040 Range-diff vs v1: 1: 5f557caee00 < -: ----------- trace2: fix memory leak of thread name -: ----------- > 1: f88e9feff26 trace2: add trace2_child_ready() to report on background children 2: 7182419f6df ! 2: 258baa0df8c simple-ipc: preparations for supporting binary messages. @@ Commit message Add `command_len` argument to the Simple IPC API. - In my original Simple IPC API, I assumed that the request - would always be a null-terminated string of text characters. - The command arg was just a `const char *`. + In my original Simple IPC API, I assumed that the request would always + be a null-terminated string of text characters. The `command` + argument was just a `const char *`. - I found a caller that would like to pass a binary command - to the daemon, so I want to ammend the Simple IPC API to - take `const char *command, size_t command_len` and pass - that to the daemon. (Really, the first arg should just be - a `void *` or `const unsigned byte *` to make that clearer.) + I found a caller that would like to pass a binary command to the + daemon, so I am amending the Simple IPC API to receive `const char + *command, size_t command_len` arguments. - Note, the response side has always been a `struct strbuf` - which includes the buffer and length, so we already support - returning a binary answer. (Yes, it feels a little weird - returning a binary buffer in a `strbuf`, but it works.) + I considered changing the `command` argument to be a `void *`, but the + IPC layer simply passes it to the pkt-line layer which takes a `const + char *`, so to avoid confusion I left it as is. + + Note, the response side has always been a `struct strbuf` which + includes the buffer and length, so we already support returning a + binary answer. (Yes, it feels a little weird returning a binary + buffer in a `strbuf`, but it works.) Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> 3: 7de207828ca = 3: c94b4cbcbf2 simple-ipc: move definition of ipc_active_state outside of ifdef 4: 30b7bb247c3 ! 4: 82b6ce0dd6a simple-ipc/ipc-win32: add trace2 debugging @@ compat/simple-ipc/ipc-win32.c: static enum ipc_active_state get_active_state(wch } @@ compat/simple-ipc/ipc-win32.c: static enum ipc_active_state connect_to_server( - if (GetLastError() == ERROR_SEM_TIMEOUT) + t_start_ms = (DWORD)(getnanotime() / 1000000); + + if (!WaitNamedPipeW(wpath, timeout_ms)) { +- if (GetLastError() == ERROR_SEM_TIMEOUT) ++ DWORD gleWait = GetLastError(); ++ ++ if (gleWait == ERROR_SEM_TIMEOUT) return IPC_STATE__NOT_LISTENING; -+ gle = GetLastError(); + trace2_data_intmax("ipc-debug", NULL, + "connect/waitpipe/gle", -+ (intmax_t)gle); ++ (intmax_t)gleWait); + return IPC_STATE__OTHER_ERROR; } 5: 5eadf719295 = 5: faf6034848e simple-ipc/ipc-win32: add Windows ACL to named pipe 6: f97038a563d ! 6: 0822118c4b5 run-command: create start_bg_command @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char + time_t time_limit; + + /* -+ * Silently disallow child cleanup -- even if requested. -+ * The child process should persist in the background -+ * and possibly/probably after this process exits. That -+ * is, don't kill the child during our atexit routine. ++ * We do not allow clean-on-exit because the child process ++ * should persist in the background and possibly/probably ++ * after this process exits. So we don't want to kill the ++ * child during our atexit routine. + */ -+ cmd->clean_on_exit = 0; ++ if (cmd->clean_on_exit) ++ BUG("start_bg_command() does not allow non-zero clean_on_exit"); ++ ++ if (!cmd->trace2_child_class) ++ cmd->trace2_child_class = "background"; + + ret = start_command(cmd); + if (ret) { @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char +wait: + pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG); + -+ if (pid_seen == 0) { ++ if (!pid_seen) { + /* + * The child is currently running. Ask the callback + * if the child is ready to do work or whether we + * should keep waiting for it to boot up. + */ -+ ret = (*wait_cb)(cb_data, cmd); ++ ret = (*wait_cb)(cmd, cb_data); + if (!ret) { + /* + * The child is running and "ready". -+ * -+ * NEEDSWORK: As we prepare to orphan (release to -+ * the background) this child, it is not appropriate -+ * to emit a `trace2_child_exit()` event. Should we -+ * create a new event for this case? + */ ++ trace2_child_ready(cmd, "ready"); + sbgr = SBGR_READY; + goto done; + } else if (ret > 0) { ++ /* ++ * The callback said to give it more time to boot up ++ * (subject to our timeout limit). ++ */ + time_t now; + + time(&now); @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char + * Our timeout has expired. We don't try to + * kill the child, but rather let it continue + * (hopefully) trying to startup. -+ * -+ * NEEDSWORK: Like the "ready" case, should we -+ * log a custom child-something Trace2 event here? + */ ++ trace2_child_ready(cmd, "timeout"); + sbgr = SBGR_TIMEOUT; + goto done; + } else { + /* -+ * The cb gave up on this child. -+ * -+ * NEEDSWORK: Like above, should we log a custom -+ * Trace2 child-something event here? ++ * The cb gave up on this child. It is still running, ++ * but our cb got an error trying to probe it. + */ ++ trace2_child_ready(cmd, "error"); + sbgr = SBGR_CB_ERROR; + goto done; + } + } + -+ if (pid_seen == cmd->pid) { ++ else if (pid_seen == cmd->pid) { + int child_code = -1; + + /* @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char + * before becoming "ready". + * + * We try to match the behavior of `wait_or_whine()` ++ * WRT the handling of WIFSIGNALED() and WIFEXITED() + * and convert the child's status to a return code for + * tracing purposes and emit the `trace2_child_exit()` + * event. ++ * ++ * We do not want the wait_or_whine() error message ++ * because we will be called by client-side library ++ * routines. + */ + if (WIFEXITED(wait_status)) + child_code = WEXITSTATUS(wait_status); @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char + goto done; + } + -+ if (pid_seen < 0 && errno == EINTR) ++ else if (pid_seen < 0 && errno == EINTR) + goto wait; + + trace2_child_exit(cmd, -1); @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir); +/** -+ * Possible return values for `start_bg_command()`. ++ * Possible return values for start_bg_command(). + */ +enum start_bg_result { + /* child process is "ready" */ @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai +}; + +/** -+ * Callback used by `start_bg_command()` to ask whether the -+ * child process is ready or needs more time to become ready. ++ * Callback used by start_bg_command() to ask whether the ++ * child process is ready or needs more time to become "ready". ++ * ++ * The callback will receive the cmd and cb_data arguments given to ++ * start_bg_command(). + * + * Returns 1 is child needs more time (subject to the requested timeout). -+ * Returns 0 if child is ready. -+ * Returns -1 on any error and cause `start_bg_command()` to also error out. ++ * Returns 0 if child is "ready". ++ * Returns -1 on any error and cause start_bg_command() to also error out. + */ -+typedef int(start_bg_wait_cb)(void *cb_data, -+ const struct child_process *cmd); ++typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data); + +/** -+ * Start a command in the background. Wait long enough for the child to -+ * become "ready". Capture immediate errors (like failure to start) and -+ * any immediate exit status (such as a shutdown/signal before the child -+ * became "ready"). ++ * Start a command in the background. Wait long enough for the child ++ * to become "ready" (as defined by the provided callback). Capture ++ * immediate errors (like failure to start) and any immediate exit ++ * status (such as a shutdown/signal before the child became "ready") ++ * and return this like start_command(). ++ * ++ * We run a custom wait loop using the provided callback to wait for ++ * the child to start and become "ready". This is limited by the given ++ * timeout value. ++ * ++ * If the child does successfully start and become "ready", we orphan ++ * it into the background. + * -+ * This is a combination of `start_command()` and `finish_command()`, but -+ * with a custom `wait_or_whine()` that allows the caller to define when -+ * the child is "ready". ++ * The caller must not call finish_command(). + * -+ * The caller does not need to call `finish_command()`. ++ * The opaque cb_data argument will be forwarded to the callback for ++ * any instance data that it might require. This may be NULL. + */ +enum start_bg_result start_bg_command(struct child_process *cmd, + start_bg_wait_cb *wait_cb, 7: 57f29feaadb ! 7: 6b7a058284b t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command @@ Commit message Convert test helper to use `start_bg_command()` when spawning a server daemon in the background rather than blocks of platform-specific code. + Also, while here, remove _() translation around error messages since + this is a test helper and not Git code. + Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com> ## t/helper/test-simple-ipc.c ## @@ t/helper/test-simple-ipc.c #ifndef SUPPORTS_SIMPLE_IPC int cmd__simple_ipc(int argc, const char **argv) @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void) + */ + ret = ipc_server_run(cl_args.path, &opts, test_app_cb, (void*)&my_app_data); + if (ret == -2) +- error(_("socket/pipe already in use: '%s'"), cl_args.path); ++ error("socket/pipe already in use: '%s'", cl_args.path); + else if (ret == -1) +- error_errno(_("could not start server on: '%s'"), cl_args.path); ++ error_errno("could not start server on: '%s'", cl_args.path); + return ret; } @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void) - close(1); - close(2); - sanitize_stdfds(); -+static int bg_wait_cb(void *cb_data, const struct child_process *cp) ++static int bg_wait_cb(const struct child_process *cp, void *cb_data) +{ + int s = ipc_get_active_state(cl_args.path); @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void) /* * This process will run a quick probe to see if a simple-ipc server * is active on this path. +@@ t/helper/test-simple-ipc.c: static int client__stop_server(void) + + time(&now); + if (now > time_limit) +- return error(_("daemon has not shutdown yet")); ++ return error("daemon has not shutdown yet"); + } + } +