[PATCH 0/7] Builtin FSMonitor Part 1

[PATCH 0/7] Builtin FSMonitor Part 1

[Jeff Hostetler via GitGitGadget]

In July I sent the V3 version [3] of my Builtin FSMonitor series [1,2,3].
This has been in "seen" as branch jh/builtin-fsmonitor. At 34 commits, it
was already a little too big to easily review. With the feed back on [3] and
from people using the experimental version that we shipped with
git-for-windows v2.32 and v2.33, I've prepared a new V4 version. This can be
seen in [4].

However, this new version currently contains 58 commits and is way too big
to be submitted as is. So I would like to close or discard the original V3
branch and submit V4 in pieces to make it easier to review.

Here is Part 1 of (what would be V4 of) my Builtin FSMonitor series.

Part 1 contains:

 * A fix for a memory leak in the Trace2 code. (This was independently
   reported in last week in "ab/tr2-leaks-and-fixes".)
 * Various cleanups in the simple-ipc layer.
 * A new start_bg_command() function to launch a command into the background
   and wait for it to start. And a refactored consumer of it in
   test-simple-ipc. There was a large discussion on commit 14/34 in V3 [5
   thru 6] about the large ifdef'd blocks of platform specific code to spawn
   background commands, trace2 handling, and duplicated code in
   fsmonitor--daemon.c and in test-simple-ipc. That has all been addressed
   here.

[1]
https://lore.kernel.org/git/pull.923.git.1617291666.gitgitgadget@gmail.com/
[2]
https://lore.kernel.org/git/pull.923.v2.git.1621691828.gitgitgadget@gmail.com/
[3]
https://lore.kernel.org/git/pull.923.v3.git.1625150864.gitgitgadget@gmail.com/
[4] https://github.com/gitgitgadget/git/pull/923 [5]
https://lore.kernel.org/git/9fe902aad87f1192705fb69ea212a2d066d0286d.1625150864.git.gitgitgadget@gmail.com/
[6] https://lore.kernel.org/git/87tukovidd.fsf@evledraar.gmail.com/

Jeff Hostetler (7):
  trace2: fix memory leak of thread name
  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

 compat/simple-ipc/ipc-unix-socket.c |  14 +-
 compat/simple-ipc/ipc-win32.c       | 176 +++++++++++++++++++--
 run-command.c                       | 123 +++++++++++++++
 run-command.h                       |  48 ++++++
 simple-ipc.h                        |  21 +--
 t/helper/test-simple-ipc.c          | 227 ++++++++--------------------
 trace2/tr2_tls.c                    |   1 +
 7 files changed, 416 insertions(+), 194 deletions(-)


base-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1040%2Fjeffhostetler%2Fbuiltin-fsmonitor-part1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1040/jeffhostetler/builtin-fsmonitor-part1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1040
-- 
gitgitgadget

[PATCH 1/7] trace2: fix memory leak of thread name

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Do not leak the thread name (contained within the thread context) when
a thread terminates.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 trace2/tr2_tls.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
index 067c23755fb..7da94aba522 100644
--- a/trace2/tr2_tls.c
+++ b/trace2/tr2_tls.c
@@ -95,6 +95,7 @@ void tr2tls_unset_self(void)
 
 	pthread_setspecific(tr2tls_key, NULL);
 
+	strbuf_release(&ctx->thread_name);
 	free(ctx->array_us_start);
 	free(ctx);
 }
-- 
gitgitgadget

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Do not leak the thread name (contained within the thread context) when
> a thread terminates.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  trace2/tr2_tls.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
> index 067c23755fb..7da94aba522 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)
>  
>  	pthread_setspecific(tr2tls_key, NULL);
>  
> +	strbuf_release(&ctx->thread_name);
>  	free(ctx->array_us_start);
>  	free(ctx);
>  }

So the idea is create allocates a new instance, and unset is to
release the resource held by it?

This is not a problem in _this_ patch but more about the base code
that is being fixed here, but using strbuf as thread_name member
sends a strong signal that it is designed to be inexpensive to
change thread_name after a context is created by create_self.  If
it is not the case, the member should be "const char *", which may
be computed using a temporary strbuf in create_self() and taken from
the strbuf with strbuf_detach(), I would think.

Thanks.

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Taylor Blau]

On Wed, Sep 15, 2021 at 02:01:39PM -0700, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Jeff Hostetler <jeffhost@microsoft.com>
> >
> > Do not leak the thread name (contained within the thread context) when
> > a thread terminates.
> >
> > Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> > ---
> >  trace2/tr2_tls.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
> > index 067c23755fb..7da94aba522 100644
> > --- a/trace2/tr2_tls.c
> > +++ b/trace2/tr2_tls.c
> > @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)
> >
> >  	pthread_setspecific(tr2tls_key, NULL);
> >
> > +	strbuf_release(&ctx->thread_name);
> >  	free(ctx->array_us_start);
> >  	free(ctx);
> >  }
>
> So the idea is create allocates a new instance, and unset is to
> release the resource held by it?
>
> This is not a problem in _this_ patch but more about the base code
> that is being fixed here, but using strbuf as thread_name member
> sends a strong signal that it is designed to be inexpensive to
> change thread_name after a context is created by create_self.  If
> it is not the case, the member should be "const char *", which may
> be computed using a temporary strbuf in create_self() and taken from
> the strbuf with strbuf_detach(), I would think.

It looks like we do not change the contents of the thread_name buffer
anywhere. I assume that we store the strbuf in struct tr2tls_thread_ctx
because we do some string formatting in tr2tls_create_self().

But there we could easily say ctx->thread_name = strbuf_release(&buf).

More concerning to me is that we use signed integers to keep track of nr
and alloc of array_us_start. But both of these are separate issues and
don't need to be dealt with here.

Thanks,
Taylor

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Do not leak the thread name (contained within the thread context) when
> a thread terminates.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  trace2/tr2_tls.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c
> index 067c23755fb..7da94aba522 100644
> --- a/trace2/tr2_tls.c
> +++ b/trace2/tr2_tls.c
> @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)
>  
>  	pthread_setspecific(tr2tls_key, NULL);
>  
> +	strbuf_release(&ctx->thread_name);
>  	free(ctx->array_us_start);
>  	free(ctx);
>  }

As noted in your cover letter:

 * A fix for a memory leak in the Trace2 code. (This was independently
   reported in last week in "ab/tr2-leaks-and-fixes".)

So I think this patch can be dropped from this series, since it's exact
duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory,
2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked
for a merge with master.

When submitting a series that depends on another one it's best to rebase
it on top of it & indicate it as such in the cover letter, Junio can
queue such a series on top of another one.

In this case I'm still not sure why this fix is here, i.e. surely
nothing later in the series absolutely needs this stray memory leak
fix...

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Taylor Blau]

On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
> So I think this patch can be dropped from this series, since it's exact
> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory,
> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked
> for a merge with master.

I agree it can be dropped.

> When submitting a series that depends on another one it's best to rebase
> it on top of it & indicate it as such in the cover letter, Junio can
> queue such a series on top of another one.
>
> In this case I'm still not sure why this fix is here, i.e. surely
> nothing later in the series absolutely needs this stray memory leak
> fix...

But there's no need for Jeff to depend on your branch, since (as you
mentioned) this cleanup isn't relevant for anything else in this series,
which is a sort of grab-bag of miscellaneous clean-ups.

Thanks,
Taylor

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 16 2021, Taylor Blau wrote:

> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> So I think this patch can be dropped from this series, since it's exact
>> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory,
>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked
>> for a merge with master.
>
> I agree it can be dropped.
>
>> When submitting a series that depends on another one it's best to rebase
>> it on top of it & indicate it as such in the cover letter, Junio can
>> queue such a series on top of another one.
>>
>> In this case I'm still not sure why this fix is here, i.e. surely
>> nothing later in the series absolutely needs this stray memory leak
>> fix...
>
> But there's no need for Jeff to depend on your branch, since (as you
> mentioned) this cleanup isn't relevant for anything else in this series,
> which is a sort of grab-bag of miscellaneous clean-ups.

Indeed, to be clear it was just general advice about queue-on-top.

But to clarify what I was getting at here: If we just came up with the
same diff I'd have assumed Jeff just hadn't need the change in "next",
but since he clearly has I was confused by it being here.

I.e. it doesn't *seem* like anything in the rest of the series depends
on it, so why have it here at all since the bug is being fixed anyway?
Or if it does depend on it in some subtle way I've missed, perhaps it
does need to be queued on top of ab/tr2-leaks-and-fixes, and the
relevant commit/subtle dependency needs to be called out in a commit
message.

Or maybe Jeff had just come up with this independently, noticed it just
before submission and just updated the CL, not the patch or series
itself :)

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Jeff Hostetler]

On 9/16/21 4:01 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Sep 16 2021, Taylor Blau wrote:
> 
>> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>> So I think this patch can be dropped from this series, since it's exact
>>> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory,
>>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked
>>> for a merge with master.
>>
>> I agree it can be dropped.
>>
>>> When submitting a series that depends on another one it's best to rebase
>>> it on top of it & indicate it as such in the cover letter, Junio can
>>> queue such a series on top of another one.
>>>
>>> In this case I'm still not sure why this fix is here, i.e. surely
>>> nothing later in the series absolutely needs this stray memory leak
>>> fix...
>>
>> But there's no need for Jeff to depend on your branch, since (as you
>> mentioned) this cleanup isn't relevant for anything else in this series,
>> which is a sort of grab-bag of miscellaneous clean-ups.
> 
> Indeed, to be clear it was just general advice about queue-on-top.
> 
> But to clarify what I was getting at here: If we just came up with the
> same diff I'd have assumed Jeff just hadn't need the change in "next",
> but since he clearly has I was confused by it being here.
> 
> I.e. it doesn't *seem* like anything in the rest of the series depends
> on it, so why have it here at all since the bug is being fixed anyway?
> Or if it does depend on it in some subtle way I've missed, perhaps it
> does need to be queued on top of ab/tr2-leaks-and-fixes, and the
> relevant commit/subtle dependency needs to be called out in a commit
> message.
> 
> Or maybe Jeff had just come up with this independently, noticed it just
> before submission and just updated the CL, not the patch or series
> itself :)
> 

I'll drop this commit since your version is already queued up
and headed to master.  I've been carrying it in my dev branch
for a while and was using it to make leak reporting a little
quieter.

And yes, I just noticed that yours had advanced when I wrote the
cover letter and ACKd it rather than dropping it.

And no, nothing in the rest of the whole FSMonitor series depends
on this, so I can leave my series based upon master rather than
your branch.

Thanks
Jeff

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 16 2021, Jeff Hostetler wrote:

> On 9/16/21 4:01 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Thu, Sep 16 2021, Taylor Blau wrote:
>> 
>>> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:
>>>> So I think this patch can be dropped from this series, since it's exact
>>>> duplicate of my 48f68715b14 (tr2: stop leaking "thread_name" memory,
>>>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in "next" and marked
>>>> for a merge with master.
>>>
>>> I agree it can be dropped.
>>>
>>>> When submitting a series that depends on another one it's best to rebase
>>>> it on top of it & indicate it as such in the cover letter, Junio can
>>>> queue such a series on top of another one.
>>>>
>>>> In this case I'm still not sure why this fix is here, i.e. surely
>>>> nothing later in the series absolutely needs this stray memory leak
>>>> fix...
>>>
>>> But there's no need for Jeff to depend on your branch, since (as you
>>> mentioned) this cleanup isn't relevant for anything else in this series,
>>> which is a sort of grab-bag of miscellaneous clean-ups.
>> Indeed, to be clear it was just general advice about queue-on-top.
>> But to clarify what I was getting at here: If we just came up with
>> the
>> same diff I'd have assumed Jeff just hadn't need the change in "next",
>> but since he clearly has I was confused by it being here.
>> I.e. it doesn't *seem* like anything in the rest of the series
>> depends
>> on it, so why have it here at all since the bug is being fixed anyway?
>> Or if it does depend on it in some subtle way I've missed, perhaps it
>> does need to be queued on top of ab/tr2-leaks-and-fixes, and the
>> relevant commit/subtle dependency needs to be called out in a commit
>> message.
>> Or maybe Jeff had just come up with this independently, noticed it
>> just
>> before submission and just updated the CL, not the patch or series
>> itself :)
>> 
>
> I'll drop this commit since your version is already queued up
> and headed to master.  I've been carrying it in my dev branch
> for a while and was using it to make leak reporting a little
> quieter.
>
> And yes, I just noticed that yours had advanced when I wrote the
> cover letter and ACKd it rather than dropping it.
>
> And no, nothing in the rest of the whole FSMonitor series depends
> on this, so I can leave my series based upon master rather than
> your branch.
>
> Thanks
> Jeff

Sounds good, thanks for clarifying.

In any case by the time you'll re-roll this (or soon thereafter) Junio
will probably have merged it down anyway.

Re: [PATCH 1/7] trace2: fix memory leak of thread name

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Indeed, to be clear it was just general advice about queue-on-top.
>
> But to clarify what I was getting at here: If we just came up with the
> same diff I'd have assumed Jeff just hadn't need the change in "next",
> but since he clearly has I was confused by it being here.
>
> I.e. it doesn't *seem* like anything in the rest of the series depends
> on it, so why have it here at all since the bug is being fixed anyway?

I'd imagine that it was there just for the same reason series from
some people (yours included) tend to bloat, either over iterations
or from day one, by including "this is not necessary for the end
goal of this topic at all, but since I noticed it, I am including
this fix, which should be obvious enough" unrelated fix.

Here is a lesson to be learned.

[PATCH 2/7] simple-ipc: preparations for supporting binary messages.

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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 *`.

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.)

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>
---
 compat/simple-ipc/ipc-unix-socket.c | 14 +++++++-----
 compat/simple-ipc/ipc-win32.c       | 14 +++++++-----
 simple-ipc.h                        |  7 ++++--
 t/helper/test-simple-ipc.c          | 34 +++++++++++++++++++----------
 4 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
index 1927e6ef4bc..4e28857a0a1 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -168,7 +168,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)
 
 int ipc_client_send_command_to_connection(
 	struct ipc_client_connection *connection,
-	const char *message, struct strbuf *answer)
+	const char *message, size_t message_len,
+	struct strbuf *answer)
 {
 	int ret = 0;
 
@@ -176,7 +177,7 @@ int ipc_client_send_command_to_connection(
 
 	trace2_region_enter("ipc-client", "send-command", NULL);
 
-	if (write_packetized_from_buf_no_flush(message, strlen(message),
+	if (write_packetized_from_buf_no_flush(message, message_len,
 					       connection->fd) < 0 ||
 	    packet_flush_gently(connection->fd) < 0) {
 		ret = error(_("could not send IPC command"));
@@ -197,7 +198,8 @@ done:
 
 int ipc_client_send_command(const char *path,
 			    const struct ipc_client_connect_options *options,
-			    const char *message, struct strbuf *answer)
+			    const char *message, size_t message_len,
+			    struct strbuf *answer)
 {
 	int ret = -1;
 	enum ipc_active_state state;
@@ -208,7 +210,9 @@ int ipc_client_send_command(const char *path,
 	if (state != IPC_STATE__LISTENING)
 		return ret;
 
-	ret = ipc_client_send_command_to_connection(connection, message, answer);
+	ret = ipc_client_send_command_to_connection(connection,
+						    message, message_len,
+						    answer);
 
 	ipc_client_close_connection(connection);
 
@@ -503,7 +507,7 @@ static int worker_thread__do_io(
 	if (ret >= 0) {
 		ret = worker_thread_data->server_data->application_cb(
 			worker_thread_data->server_data->application_data,
-			buf.buf, do_io_reply_callback, &reply_data);
+			buf.buf, buf.len, do_io_reply_callback, &reply_data);
 
 		packet_flush_gently(reply_data.fd);
 	}
diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 8dc7bda087d..8e889d6a506 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -208,7 +208,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)
 
 int ipc_client_send_command_to_connection(
 	struct ipc_client_connection *connection,
-	const char *message, struct strbuf *answer)
+	const char *message, size_t message_len,
+	struct strbuf *answer)
 {
 	int ret = 0;
 
@@ -216,7 +217,7 @@ int ipc_client_send_command_to_connection(
 
 	trace2_region_enter("ipc-client", "send-command", NULL);
 
-	if (write_packetized_from_buf_no_flush(message, strlen(message),
+	if (write_packetized_from_buf_no_flush(message, message_len,
 					       connection->fd) < 0 ||
 	    packet_flush_gently(connection->fd) < 0) {
 		ret = error(_("could not send IPC command"));
@@ -239,7 +240,8 @@ done:
 
 int ipc_client_send_command(const char *path,
 			    const struct ipc_client_connect_options *options,
-			    const char *message, struct strbuf *response)
+			    const char *message, size_t message_len,
+			    struct strbuf *response)
 {
 	int ret = -1;
 	enum ipc_active_state state;
@@ -250,7 +252,9 @@ int ipc_client_send_command(const char *path,
 	if (state != IPC_STATE__LISTENING)
 		return ret;
 
-	ret = ipc_client_send_command_to_connection(connection, message, response);
+	ret = ipc_client_send_command_to_connection(connection,
+						    message, message_len,
+						    response);
 
 	ipc_client_close_connection(connection);
 
@@ -458,7 +462,7 @@ static int do_io(struct ipc_server_thread_data *server_thread_data)
 	if (ret >= 0) {
 		ret = server_thread_data->server_data->application_cb(
 			server_thread_data->server_data->application_data,
-			buf.buf, do_io_reply_callback, &reply_data);
+			buf.buf, buf.len, do_io_reply_callback, &reply_data);
 
 		packet_flush_gently(reply_data.fd);
 
diff --git a/simple-ipc.h b/simple-ipc.h
index 2c48a5ee004..9c7330fcda0 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -107,7 +107,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection);
  */
 int ipc_client_send_command_to_connection(
 	struct ipc_client_connection *connection,
-	const char *message, struct strbuf *answer);
+	const char *message, size_t message_len,
+	struct strbuf *answer);
 
 /*
  * Used by the client to synchronously connect and send and receive a
@@ -119,7 +120,8 @@ int ipc_client_send_command_to_connection(
  */
 int ipc_client_send_command(const char *path,
 			    const struct ipc_client_connect_options *options,
-			    const char *message, struct strbuf *answer);
+			    const char *message, size_t message_len,
+			    struct strbuf *answer);
 
 /*
  * Simple IPC Server Side API.
@@ -144,6 +146,7 @@ typedef int (ipc_server_reply_cb)(struct ipc_server_reply_data *,
  */
 typedef int (ipc_server_application_cb)(void *application_data,
 					const char *request,
+					size_t request_len,
 					ipc_server_reply_cb *reply_cb,
 					struct ipc_server_reply_data *reply_data);
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 42040ef81b1..91345180750 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -112,7 +112,7 @@ static int app__slow_command(ipc_server_reply_cb *reply_cb,
 /*
  * The client sent a command followed by a (possibly very) large buffer.
  */
-static int app__sendbytes_command(const char *received,
+static int app__sendbytes_command(const char *received, size_t received_len,
 				  ipc_server_reply_cb *reply_cb,
 				  struct ipc_server_reply_data *reply_data)
 {
@@ -123,6 +123,13 @@ static int app__sendbytes_command(const char *received,
 	int errs = 0;
 	int ret;
 
+	/*
+	 * The test is setup to send:
+	 *     "sendbytes" SP <n * char>
+	 */
+	if (received_len < strlen("sendbytes "))
+		BUG("received_len is short in app__sendbytes_command");
+
 	if (skip_prefix(received, "sendbytes ", &p))
 		len_ballast = strlen(p);
 
@@ -160,7 +167,7 @@ static ipc_server_application_cb test_app_cb;
  * by this application.
  */
 static int test_app_cb(void *application_data,
-		       const char *command,
+		       const char *command, size_t command_len,
 		       ipc_server_reply_cb *reply_cb,
 		       struct ipc_server_reply_data *reply_data)
 {
@@ -173,7 +180,7 @@ static int test_app_cb(void *application_data,
 	if (application_data != (void*)&my_app_data)
 		BUG("application_cb: application_data pointer wrong");
 
-	if (!strcmp(command, "quit")) {
+	if (command_len == 4 && !strncmp(command, "quit", 4)) {
 		/*
 		 * The client sent a "quit" command.  This is an async
 		 * request for the server to shutdown.
@@ -193,22 +200,23 @@ static int test_app_cb(void *application_data,
 		return SIMPLE_IPC_QUIT;
 	}
 
-	if (!strcmp(command, "ping")) {
+	if (command_len == 4 && !strncmp(command, "ping", 4)) {
 		const char *answer = "pong";
 		return reply_cb(reply_data, answer, strlen(answer));
 	}
 
-	if (!strcmp(command, "big"))
+	if (command_len == 3 && !strncmp(command, "big", 3))
 		return app__big_command(reply_cb, reply_data);
 
-	if (!strcmp(command, "chunk"))
+	if (command_len == 5 && !strncmp(command, "chunk", 5))
 		return app__chunk_command(reply_cb, reply_data);
 
-	if (!strcmp(command, "slow"))
+	if (command_len == 4 && !strncmp(command, "slow", 4))
 		return app__slow_command(reply_cb, reply_data);
 
-	if (starts_with(command, "sendbytes "))
-		return app__sendbytes_command(command, reply_cb, reply_data);
+	if (command_len >= 10 && starts_with(command, "sendbytes "))
+		return app__sendbytes_command(command, command_len,
+					      reply_cb, reply_data);
 
 	return app__unhandled_command(command, reply_cb, reply_data);
 }
@@ -488,7 +496,9 @@ static int client__send_ipc(void)
 	options.wait_if_busy = 1;
 	options.wait_if_not_found = 0;
 
-	if (!ipc_client_send_command(cl_args.path, &options, command, &buf)) {
+	if (!ipc_client_send_command(cl_args.path, &options,
+				     command, strlen(command),
+				     &buf)) {
 		if (buf.len) {
 			printf("%s\n", buf.buf);
 			fflush(stdout);
@@ -556,7 +566,9 @@ static int do_sendbytes(int bytecount, char byte, const char *path,
 	strbuf_addstr(&buf_send, "sendbytes ");
 	strbuf_addchars(&buf_send, byte, bytecount);
 
-	if (!ipc_client_send_command(path, options, buf_send.buf, &buf_resp)) {
+	if (!ipc_client_send_command(path, options,
+				     buf_send.buf, buf_send.len,
+				     &buf_resp)) {
 		strbuf_rtrim(&buf_resp);
 		printf("sent:%c%08d %s\n", byte, bytecount, buf_resp.buf);
 		fflush(stdout);
-- 
gitgitgadget

Re: [PATCH 2/7] simple-ipc: preparations for supporting binary messages.

[Eric Sunshine]

On Wed, Sep 15, 2021 at 4:36 PM Jeff Hostetler via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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 *`.
>
> 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

s/ammend/amend/

> 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.)

The reader is left wondering why you didn't also change it to `const
void *` (or one of the other choices) while at it.

> 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>

Re: [PATCH 2/7] simple-ipc: preparations for supporting binary messages.

[Jeff Hostetler]

On 9/15/21 4:43 PM, Eric Sunshine wrote:
> On Wed, Sep 15, 2021 at 4:36 PM Jeff Hostetler via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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 *`.
>>
>> 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
> 
> s/ammend/amend/
> 
>> 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.)
> 
> The reader is left wondering why you didn't also change it to `const
> void *` (or one of the other choices) while at it.
> 

The simple ipc layer just passes the buffer to the pkt-line layer
and it takes a "const char *", so to avoid confusion I just left
the type is it was.  If later we want to fix pkt-line, we can
investigate passing a "const unsigned byte *" value down the
call chain, but that is more than I want to do right now.

>> 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>

[PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Move the declartion of the `enum ipc_active_state` type outside of
the SUPPORTS_SIMPLE_IPC ifdef.

A later commit will introduce the `fsmonitor_ipc__*()` API and stub in
a "mock" implementation that requires this enum in some function
signatures.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 simple-ipc.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/simple-ipc.h b/simple-ipc.h
index 9c7330fcda0..b396293bdfc 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -5,13 +5,6 @@
  * See Documentation/technical/api-simple-ipc.txt
  */
 
-#ifdef SUPPORTS_SIMPLE_IPC
-#include "pkt-line.h"
-
-/*
- * Simple IPC Client Side API.
- */
-
 enum ipc_active_state {
 	/*
 	 * The pipe/socket exists and the daemon is waiting for connections.
@@ -43,6 +36,13 @@ enum ipc_active_state {
 	IPC_STATE__OTHER_ERROR,
 };
 
+#ifdef SUPPORTS_SIMPLE_IPC
+#include "pkt-line.h"
+
+/*
+ * Simple IPC Client Side API.
+ */
+
 struct ipc_client_connect_options {
 	/*
 	 * Spin under timeout if the server is running but can't
-- 
gitgitgadget

Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef

[Junio C Hamano]

"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>
> Move the declartion of the `enum ipc_active_state` type outside of
> the SUPPORTS_SIMPLE_IPC ifdef.

The second one is not an in-body header since there is already a
blank line that signals the end of in-body headers after the first
one.

This _may_ be a bug in GGG, perhaps?


> A later commit will introduce the `fsmonitor_ipc__*()` API and stub in
> a "mock" implementation that requires this enum in some function
> signatures.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  simple-ipc.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/simple-ipc.h b/simple-ipc.h
> index 9c7330fcda0..b396293bdfc 100644
> --- a/simple-ipc.h
> +++ b/simple-ipc.h
> @@ -5,13 +5,6 @@
>   * See Documentation/technical/api-simple-ipc.txt
>   */
>  
> -#ifdef SUPPORTS_SIMPLE_IPC
> -#include "pkt-line.h"
> -
> -/*
> - * Simple IPC Client Side API.
> - */
> -
>  enum ipc_active_state {
>  	/*
>  	 * The pipe/socket exists and the daemon is waiting for connections.
> @@ -43,6 +36,13 @@ enum ipc_active_state {
>  	IPC_STATE__OTHER_ERROR,
>  };
>  
> +#ifdef SUPPORTS_SIMPLE_IPC
> +#include "pkt-line.h"
> +
> +/*
> + * Simple IPC Client Side API.
> + */
> +
>  struct ipc_client_connect_options {
>  	/*
>  	 * Spin under timeout if the server is running but can't

Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef

[Jeff Hostetler]

On 9/15/21 5:06 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>>
>> Move the declartion of the `enum ipc_active_state` type outside of
>> the SUPPORTS_SIMPLE_IPC ifdef.
> 
> The second one is not an in-body header since there is already a
> blank line that signals the end of in-body headers after the first
> one.
> 
> This _may_ be a bug in GGG, perhaps?

Maybe.  I'll make a note to ask @dscho when he gets back from vacation.

Credit for the commit should go to Carlo.  I just added it to the series
with his "From:" line and it looks like GGG added an extra one before
it.

Jeff

Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef

[Carlo Arenas]

On Fri, Sep 17, 2021 at 11:39 PM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
> Maybe.  I'll make a note to ask @dscho when he gets back from vacation.

I am actually more worried about GGG introducing a typo in my flawless
commit message.

Carlo

Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> On 9/15/21 5:06 PM, Junio C Hamano wrote:
>> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>>
>>> From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>>>
>>> Move the declartion of the `enum ipc_active_state` type outside of
>>> the SUPPORTS_SIMPLE_IPC ifdef.
>> The second one is not an in-body header since there is already a
>> blank line that signals the end of in-body headers after the first
>> one.
>> This _may_ be a bug in GGG, perhaps?
>
> Maybe.  I'll make a note to ask @dscho when he gets back from vacation.
>
> Credit for the commit should go to Carlo.  I just added it to the series
> with his "From:" line and it looks like GGG added an extra one before
> it.

Thanks for a clarification.  I'll tweak the authorship when queuing.

[PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create "ipc-debug" category events to log unexpected errors
when creating Simple-IPC connections.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/simple-ipc/ipc-win32.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 8e889d6a506..6c8a308de13 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -49,6 +49,9 @@ static enum ipc_active_state get_active_state(wchar_t *pipe_path)
 	if (GetLastError() == ERROR_FILE_NOT_FOUND)
 		return IPC_STATE__PATH_NOT_FOUND;
 
+	trace2_data_intmax("ipc-debug", NULL, "getstate/waitpipe/gle",
+			   (intmax_t)GetLastError());
+
 	return IPC_STATE__OTHER_ERROR;
 }
 
@@ -112,6 +115,11 @@ static enum ipc_active_state connect_to_server(
 				if (GetLastError() == ERROR_SEM_TIMEOUT)
 					return IPC_STATE__NOT_LISTENING;
 
+				gle = GetLastError();
+				trace2_data_intmax("ipc-debug", NULL,
+						   "connect/waitpipe/gle",
+						   (intmax_t)gle);
+
 				return IPC_STATE__OTHER_ERROR;
 			}
 
@@ -133,17 +141,31 @@ static enum ipc_active_state connect_to_server(
 			break; /* try again */
 
 		default:
+			trace2_data_intmax("ipc-debug", NULL,
+					   "connect/createfile/gle",
+					   (intmax_t)gle);
+
 			return IPC_STATE__OTHER_ERROR;
 		}
 	}
 
 	if (!SetNamedPipeHandleState(hPipe, &mode, NULL, NULL)) {
+		gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL,
+				   "connect/setpipestate/gle",
+				   (intmax_t)gle);
+
 		CloseHandle(hPipe);
 		return IPC_STATE__OTHER_ERROR;
 	}
 
 	*pfd = _open_osfhandle((intptr_t)hPipe, O_RDWR|O_BINARY);
 	if (*pfd < 0) {
+		gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL,
+				   "connect/openosfhandle/gle",
+				   (intmax_t)gle);
+
 		CloseHandle(hPipe);
 		return IPC_STATE__OTHER_ERROR;
 	}
-- 
gitgitgadget

Re: [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:

> @@ -112,6 +115,11 @@ static enum ipc_active_state connect_to_server(
>  				if (GetLastError() == ERROR_SEM_TIMEOUT)
>  					return IPC_STATE__NOT_LISTENING;
>  
> +				gle = GetLastError();
> +				trace2_data_intmax("ipc-debug", NULL,
> +						   "connect/waitpipe/gle",
> +						   (intmax_t)gle);
> +
>  				return IPC_STATE__OTHER_ERROR;
>  			}
>  

I've never used this Win32 API (or well, any Win32 API) but I'm guessing
that GetLastError() isn't here to check an error in GetLastError() itself.

Earlier in this function added in your 59c7b88198a (simple-ipc: add
win32 implementation, 2021-03-15) we assign to "gle", I'd really expect...:

>  	if (!SetNamedPipeHandleState(hPipe, &mode, NULL, NULL)) {
> +		gle = GetLastError();
> +		trace2_data_intmax("ipc-debug", NULL,
> +				   "connect/setpipestate/gle",
> +				   (intmax_t)gle);
> +
>  		CloseHandle(hPipe);
>  		return IPC_STATE__OTHER_ERROR;
>  	}
>  
>  	*pfd = _open_osfhandle((intptr_t)hPipe, O_RDWR|O_BINARY);
>  	if (*pfd < 0) {
> +		gle = GetLastError();
> +		trace2_data_intmax("ipc-debug", NULL,
> +				   "connect/openosfhandle/gle",
> +				   (intmax_t)gle);
> +
>  		CloseHandle(hPipe);
>  		return IPC_STATE__OTHER_ERROR;
>  	}

...something that looks exactly like this. I.e. as shown by the below
hunk-at-the-end, as it is I'm either missing some subtlety that could
really use explaining. I.e. this reads like:

    int saved_errno = errno;
    if (syscall()) {
        if (errno)
            die("bad");
        saved_errno = errno;
        log_it("...%d", saved_errno);
    }

When surely we want either of:

    int saved_errno = errno;
    if (syscall()) {
        if (errno)
            die("bad");
        log_it("...%d", errno);
    }

Or better yet (and consistent with the rest of your code):

    int saved_errno = errno;
    if (syscall()) {
        saved_errno = errno;
        if (saved_errno)
            die("bad");
        log_it("...%d", saved_errno);
    }

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 8dc7bda087d..b0c422e4867 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -109,9 +109,12 @@ static enum ipc_active_state connect_to_server(
 			t_start_ms = (DWORD)(getnanotime() / 1000000);
 
 			if (!WaitNamedPipeW(wpath, timeout_ms)) {
-				if (GetLastError() == ERROR_SEM_TIMEOUT)
+				gle = GetLastError();
+				if (gle == ERROR_SEM_TIMEOUT)
 					return IPC_STATE__NOT_LISTENING;
 
+				/* ...rest of your patch */
+
 				return IPC_STATE__OTHER_ERROR;
 			}
 

Re: [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging

[Jeff Hostetler]

On 9/16/21 1:40 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> @@ -112,6 +115,11 @@ static enum ipc_active_state connect_to_server(
>>   				if (GetLastError() == ERROR_SEM_TIMEOUT)
>>   					return IPC_STATE__NOT_LISTENING;
>>   
>> +				gle = GetLastError();
>> +				trace2_data_intmax("ipc-debug", NULL,
>> +						   "connect/waitpipe/gle",
>> +						   (intmax_t)gle);
>> +
>>   				return IPC_STATE__OTHER_ERROR;
>>   			}
>>   
>... 
 >
> @@ -109,9 +109,12 @@ static enum ipc_active_state connect_to_server(
>   			t_start_ms = (DWORD)(getnanotime() / 1000000);
>   
>   			if (!WaitNamedPipeW(wpath, timeout_ms)) {
> -				if (GetLastError() == ERROR_SEM_TIMEOUT)
> +				gle = GetLastError();
> +				if (gle == ERROR_SEM_TIMEOUT)
>   					return IPC_STATE__NOT_LISTENING;
>   
> +				/* ...rest of your patch */
> +
>   				return IPC_STATE__OTHER_ERROR;
>   			}
>   
> 

Yeah, I was just trying to minimize the size of the diff
and at the time was considering those debug messages to be
temporary, but I kind of like having them so it makes sense
to clean up a bit as you've indicated.

Thanks
Jeff

[PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Set an ACL on the named pipe to allow the well-known group EVERYONE
to read and write to the IPC server's named pipe.  In the event that
the daemon was started with elevation, allow non-elevated clients
to communicate with the daemon.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/simple-ipc/ipc-win32.c | 140 +++++++++++++++++++++++++++++++---
 1 file changed, 129 insertions(+), 11 deletions(-)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 6c8a308de13..374ae2f81c7 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -3,6 +3,8 @@
 #include "strbuf.h"
 #include "pkt-line.h"
 #include "thread-utils.h"
+#include "accctrl.h"
+#include "aclapi.h"
 
 #ifndef SUPPORTS_SIMPLE_IPC
 /*
@@ -591,11 +593,132 @@ finished:
 	return NULL;
 }
 
+/*
+ * We need to build a Windows "SECURITY_ATTRIBUTES" object and use it
+ * to apply an ACL when we create the initial instance of the Named
+ * Pipe.  The construction is somewhat involved and consists of
+ * several sequential steps and intermediate objects.
+ *
+ * We use this structure to hold these intermediate pointers so that
+ * we can free them as a group.  (It is unclear from the docs whether
+ * some of these intermediate pointers can be freed before we are
+ * finished using the "lpSA" member.)
+ */
+struct my_sa_data
+{
+	PSID pEveryoneSID;
+	PACL pACL;
+	PSECURITY_DESCRIPTOR pSD;
+	LPSECURITY_ATTRIBUTES lpSA;
+};
+
+static void init_sa(struct my_sa_data *d)
+{
+	memset(d, 0, sizeof(*d));
+}
+
+static void release_sa(struct my_sa_data *d)
+{
+	if (d->pEveryoneSID)
+		FreeSid(d->pEveryoneSID);
+	if (d->pACL)
+		LocalFree(d->pACL);
+	if (d->pSD)
+		LocalFree(d->pSD);
+	if (d->lpSA)
+		LocalFree(d->lpSA);
+
+	memset(d, 0, sizeof(*d));
+}
+
+/*
+ * Create SECURITY_ATTRIBUTES to apply to the initial named pipe.  The
+ * creator of the first server instance gets to set the ACLs on it.
+ *
+ * We allow the well-known group `EVERYONE` to have read+write access
+ * to the named pipe so that clients can send queries to the daemon
+ * and receive the response.
+ *
+ * Normally, this is not necessary since the daemon is usually
+ * automatically started by a foreground command like `git status`,
+ * but in those cases where an elevated Git command started the daemon
+ * (such that the daemon itself runs with elevation), we need to add
+ * the ACL so that non-elevated commands can write to it.
+ *
+ * The following document was helpful:
+ * https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c--
+ *
+ * Returns d->lpSA set to a SA or NULL.
+ */
+static LPSECURITY_ATTRIBUTES get_sa(struct my_sa_data *d)
+{
+	SID_IDENTIFIER_AUTHORITY sid_auth_world = SECURITY_WORLD_SID_AUTHORITY;
+#define NR_EA (1)
+	EXPLICIT_ACCESS ea[NR_EA];
+	DWORD dwResult;
+
+	if (!AllocateAndInitializeSid(&sid_auth_world, 1,
+				      SECURITY_WORLD_RID, 0,0,0,0,0,0,0,
+				      &d->pEveryoneSID)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "alloc-world-sid/gle",
+				   (intmax_t)gle);
+		goto fail;
+	}
+
+	memset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS));
+
+	ea[0].grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
+	ea[0].grfAccessMode = SET_ACCESS;
+	ea[0].grfInheritance = NO_INHERITANCE;
+	ea[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
+	ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+	ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
+	ea[0].Trustee.ptstrName = (LPTSTR)d->pEveryoneSID;
+
+	dwResult = SetEntriesInAcl(NR_EA, ea, NULL, &d->pACL);
+	if (dwResult != ERROR_SUCCESS) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "set-acl-entry/gle",
+				   (intmax_t)gle);
+		trace2_data_intmax("ipc-debug", NULL, "set-acl-entry/dw",
+				   (intmax_t)dwResult);
+		goto fail;
+	}
+
+	d->pSD = (PSECURITY_DESCRIPTOR)LocalAlloc(
+		LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);
+	if (!InitializeSecurityDescriptor(d->pSD, SECURITY_DESCRIPTOR_REVISION)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "init-sd/gle", (intmax_t)gle);
+		goto fail;
+	}
+
+	if (!SetSecurityDescriptorDacl(d->pSD, TRUE, d->pACL, FALSE)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "set-sd-dacl/gle", (intmax_t)gle);
+		goto fail;
+	}
+
+	d->lpSA = (LPSECURITY_ATTRIBUTES)LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));
+	d->lpSA->nLength = sizeof(SECURITY_ATTRIBUTES);
+	d->lpSA->lpSecurityDescriptor = d->pSD;
+	d->lpSA->bInheritHandle = FALSE;
+
+	return d->lpSA;
+
+fail:
+	release_sa(d);
+	return NULL;
+}
+
 static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
 {
 	HANDLE hPipe;
 	DWORD dwOpenMode, dwPipeMode;
-	LPSECURITY_ATTRIBUTES lpsa = NULL;
+	struct my_sa_data my_sa_data;
+
+	init_sa(&my_sa_data);
 
 	dwOpenMode = PIPE_ACCESS_INBOUND | PIPE_ACCESS_OUTBOUND |
 		FILE_FLAG_OVERLAPPED;
@@ -611,20 +734,15 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
 		 * set the ACL / Security Attributes on the named
 		 * pipe; subsequent instances inherit and cannot
 		 * change them.
-		 *
-		 * TODO Should we allow the application layer to
-		 * specify security attributes, such as `LocalService`
-		 * or `LocalSystem`, when we create the named pipe?
-		 * This question is probably not important when the
-		 * daemon is started by a foreground user process and
-		 * only needs to talk to the current user, but may be
-		 * if the daemon is run via the Control Panel as a
-		 * System Service.
 		 */
+		get_sa(&my_sa_data);
 	}
 
 	hPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,
-				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);
+				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,
+				 my_sa_data.lpSA);
+
+	release_sa(&my_sa_data);
 
 	return hPipe;
 }
-- 
gitgitgadget

Re: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:

> +struct my_sa_data
> +{
> +	PSID pEveryoneSID;
> +	PACL pACL;
> +	PSECURITY_DESCRIPTOR pSD;
> +	LPSECURITY_ATTRIBUTES lpSA;
> +};
> +
> +static void init_sa(struct my_sa_data *d)
> +{
> +	memset(d, 0, sizeof(*d));
> +}
> +
> +static void release_sa(struct my_sa_data *d)
> +{
> +	if (d->pEveryoneSID)
> +		FreeSid(d->pEveryoneSID);
> +	if (d->pACL)
> +		LocalFree(d->pACL);
> +	if (d->pSD)
> +		LocalFree(d->pSD);
> +	if (d->lpSA)
> +		LocalFree(d->lpSA);
> +
> +	memset(d, 0, sizeof(*d));
> +}

[...]

> +fail:
> +	release_sa(d);
> +	return NULL;
> +}
> +
>  static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
>  {
>  	HANDLE hPipe;
>  	DWORD dwOpenMode, dwPipeMode;
> -	LPSECURITY_ATTRIBUTES lpsa = NULL;
> +	struct my_sa_data my_sa_data;
> +
> +	init_sa(&my_sa_data);
>  

[...]

>  	hPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,
> -				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);
> +				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,
> +				 my_sa_data.lpSA);
> +
> +	release_sa(&my_sa_data);
>  
>  	return hPipe;
>  }

Just a nit on the init pattern, since we always allocate this on the
stack (this as all the relevant "struct my_sa_data") I'd have thought to
see:

    #define INIT_MY_SA_DATA { 0 }
    [...]
    struct my_sa_data my_sa_data = INIT_MY_SA_DATA;

Which gets rid of the need for an init_sa() function.

Also having the release_sa() do a memset() is a bit odd, usually we have
a reset*() function do that if the intent is to re-use, but it doesn't
appear to be in this case, and we don't return this data anywhere, do
we?

Re: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe

[Jeff Hostetler]

On 9/16/21 1:47 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> +struct my_sa_data
>> +{
>> +	PSID pEveryoneSID;
>> +	PACL pACL;
>> +	PSECURITY_DESCRIPTOR pSD;
>> +	LPSECURITY_ATTRIBUTES lpSA;
>> +};
>> +
>> +static void init_sa(struct my_sa_data *d)
>> +{
>> +	memset(d, 0, sizeof(*d));
>> +}
>> +
>> +static void release_sa(struct my_sa_data *d)
>> +{
>> +	if (d->pEveryoneSID)
>> +		FreeSid(d->pEveryoneSID);
>> +	if (d->pACL)
>> +		LocalFree(d->pACL);
>> +	if (d->pSD)
>> +		LocalFree(d->pSD);
>> +	if (d->lpSA)
>> +		LocalFree(d->lpSA);
>> +
>> +	memset(d, 0, sizeof(*d));
>> +}
> 
> [...]
> 
>> +fail:
>> +	release_sa(d);
>> +	return NULL;
>> +}
>> +
>>   static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
>>   {
>>   	HANDLE hPipe;
>>   	DWORD dwOpenMode, dwPipeMode;
>> -	LPSECURITY_ATTRIBUTES lpsa = NULL;
>> +	struct my_sa_data my_sa_data;
>> +
>> +	init_sa(&my_sa_data);
>>   
> 
> [...]
> 
>>   	hPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,
>> -				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);
>> +				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,
>> +				 my_sa_data.lpSA);
>> +
>> +	release_sa(&my_sa_data);
>>   
>>   	return hPipe;
>>   }
> 
> Just a nit on the init pattern, since we always allocate this on the
> stack (this as all the relevant "struct my_sa_data") I'd have thought to
> see:
> 
>      #define INIT_MY_SA_DATA { 0 }
>      [...]
>      struct my_sa_data my_sa_data = INIT_MY_SA_DATA;
> 
> Which gets rid of the need for an init_sa() function.

The current "my_sa_data" is just a set of 4 pointers, so yes your
INIT_MY_SA_DATA macro and my init_sa function are equivalent.
(And assuming that mine and memset are inlined by the compiler, they
are both probably exactly equivalent.)   So it really doesn't matter
one way or the other.

> 
> Also having the release_sa() do a memset() is a bit odd, usually we have
> a reset*() function do that if the intent is to re-use, but it doesn't
> appear to be in this case, and we don't return this data anywhere, do
> we?
> 

I use the release_sa() function inside my "fail:" label in get_sa()
to cleanup if any of the component parts of the SA cannot be created.
So the return value of get_sa() is either fully constructed or contains
NULL pointers.

(The docs are jargon heavy and little obscure and circular and it
is not at all clear if/when we might fail to create some of these
sub-structures.)

So I allow the SA failure to be silently ignored and we create the named
pipe without an ACL.  Normally, this is fine since the daemon usually
isn't elevated.

In create_new_pipe we always call release_sa to free the pointers if
necessary.  (So in case of a failure in get_sa, we won't double free
the pointers when create_new_pipe calls release_sa.)

Another way to think of it is that in release_sa, I'd like to have
FREE_AND_NULL() variants for FreeSid() and LocalFree(), but a simple
memset is sufficient.


Jeff

Re: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 17 2021, Jeff Hostetler wrote:

> On 9/16/21 1:47 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:
>> 
>>> +struct my_sa_data
>>> +{
>>> +	PSID pEveryoneSID;
>>> +	PACL pACL;
>>> +	PSECURITY_DESCRIPTOR pSD;
>>> +	LPSECURITY_ATTRIBUTES lpSA;
>>> +};
>>> +
>>> +static void init_sa(struct my_sa_data *d)
>>> +{
>>> +	memset(d, 0, sizeof(*d));
>>> +}
>>> +
>>> +static void release_sa(struct my_sa_data *d)
>>> +{
>>> +	if (d->pEveryoneSID)
>>> +		FreeSid(d->pEveryoneSID);
>>> +	if (d->pACL)
>>> +		LocalFree(d->pACL);
>>> +	if (d->pSD)
>>> +		LocalFree(d->pSD);
>>> +	if (d->lpSA)
>>> +		LocalFree(d->lpSA);
>>> +
>>> +	memset(d, 0, sizeof(*d));
>>> +}
>> [...]
>> 
>>> +fail:
>>> +	release_sa(d);
>>> +	return NULL;
>>> +}
>>> +
>>>   static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
>>>   {
>>>   	HANDLE hPipe;
>>>   	DWORD dwOpenMode, dwPipeMode;
>>> -	LPSECURITY_ATTRIBUTES lpsa = NULL;
>>> +	struct my_sa_data my_sa_data;
>>> +
>>> +	init_sa(&my_sa_data);
>>>   
>> [...]
>> 
>>>   	hPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,
>>> -				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);
>>> +				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,
>>> +				 my_sa_data.lpSA);
>>> +
>>> +	release_sa(&my_sa_data);
>>>     	return hPipe;
>>>   }
>> Just a nit on the init pattern, since we always allocate this on the
>> stack (this as all the relevant "struct my_sa_data") I'd have thought to
>> see:
>>      #define INIT_MY_SA_DATA { 0 }
>>      [...]
>>      struct my_sa_data my_sa_data = INIT_MY_SA_DATA;
>> Which gets rid of the need for an init_sa() function.
>
> The current "my_sa_data" is just a set of 4 pointers, so yes your
> INIT_MY_SA_DATA macro and my init_sa function are equivalent.
> (And assuming that mine and memset are inlined by the compiler, they
> are both probably exactly equivalent.)   So it really doesn't matter
> one way or the other.

Yes it's the same to the compiler, see 5726a6b4012 (*.c *_init(): define
in terms of corresponding *_INIT macro, 2021-07-01).

But being the same to the compiler != the same to human readers. I was
going for the latter here, i.e. in git.git init with a macro tends to be
used if the init is simple enough to be run like that, and with a
function if it really does need to run some function (and then after
5726a6b4012 the init-via-function-via-macro for things that need init
after malloc() or whatever).

Whereas this one's just on the stack, and doesn't need anything special,
so the nit was about just preferring the simplest construct for the job.

>> Also having the release_sa() do a memset() is a bit odd, usually we
>> have
>> a reset*() function do that if the intent is to re-use, but it doesn't
>> appear to be in this case, and we don't return this data anywhere, do
>> we?
>> 
>
> I use the release_sa() function inside my "fail:" label in get_sa()
> to cleanup if any of the component parts of the SA cannot be created.
> So the return value of get_sa() is either fully constructed or contains
> NULL pointers.
>
> (The docs are jargon heavy and little obscure and circular and it
> is not at all clear if/when we might fail to create some of these
> sub-structures.)
>
> So I allow the SA failure to be silently ignored and we create the named
> pipe without an ACL.  Normally, this is fine since the daemon usually
> isn't elevated.
>
> In create_new_pipe we always call release_sa to free the pointers if
> necessary.  (So in case of a failure in get_sa, we won't double free
> the pointers when create_new_pipe calls release_sa.)
>
> Another way to think of it is that in release_sa, I'd like to have
> FREE_AND_NULL() variants for FreeSid() and LocalFree(), but a simple
> memset is sufficient.

*nod*, I meant if functions are doing that sort of intent-to-reuse we
 usually call them *_reset(), with a *_release() that's just a plain
 free() and forget.

[PATCH 6/7] run-command: create start_bg_command

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create a variation of `run_command()` and `start_command()` to launch a command
into the background and optionally wait for it to become "ready" before returning.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
 run-command.h |  48 ++++++++++++++++++++
 2 files changed, 171 insertions(+)

diff --git a/run-command.c b/run-command.c
index 3e4e082e94d..fe75fd08f74 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
 	}
 	strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
 }
+
+enum start_bg_result start_bg_command(struct child_process *cmd,
+				      start_bg_wait_cb *wait_cb,
+				      void *cb_data,
+				      unsigned int timeout_sec)
+{
+	enum start_bg_result sbgr = SBGR_ERROR;
+	int ret;
+	int wait_status;
+	pid_t pid_seen;
+	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.
+	 */
+	cmd->clean_on_exit = 0;
+
+	ret = start_command(cmd);
+	if (ret) {
+		/*
+		 * We assume that if `start_command()` fails, we
+		 * either get a complete `trace2_child_start() /
+		 * trace2_child_exit()` pair or it fails before the
+		 * `trace2_child_start()` is emitted, so we do not
+		 * need to worry about it here.
+		 *
+		 * We also assume that `start_command()` does not add
+		 * us to the cleanup list.  And that it calls
+		 * calls `child_process_clear()`.
+		 */
+		sbgr = SBGR_ERROR;
+		goto done;
+	}
+
+	time(&time_limit);
+	time_limit += timeout_sec;
+
+wait:
+	pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
+
+	if (pid_seen == 0) {
+		/*
+		 * 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);
+		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?
+			 */
+			sbgr = SBGR_READY;
+			goto done;
+		} else if (ret > 0) {
+			time_t now;
+
+			time(&now);
+			if (now < time_limit)
+				goto wait;
+
+			/*
+			 * 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?
+			 */
+			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?
+			 */
+			sbgr = SBGR_CB_ERROR;
+			goto done;
+		}
+	}
+
+	if (pid_seen == cmd->pid) {
+		int child_code = -1;
+
+		/*
+		 * The child started, but exited or was terminated
+		 * before becoming "ready".
+		 *
+		 * We try to match the behavior of `wait_or_whine()`
+		 * and convert the child's status to a return code for
+		 * tracing purposes and emit the `trace2_child_exit()`
+		 * event.
+		 */
+		if (WIFEXITED(wait_status))
+			child_code = WEXITSTATUS(wait_status);
+		else if (WIFSIGNALED(wait_status))
+			child_code = WTERMSIG(wait_status) + 128;
+		trace2_child_exit(cmd, child_code);
+
+		sbgr = SBGR_DIED;
+		goto done;
+	}
+
+	if (pid_seen < 0 && errno == EINTR)
+		goto wait;
+
+	trace2_child_exit(cmd, -1);
+	sbgr = SBGR_ERROR;
+
+done:
+	child_process_clear(cmd);
+	invalidate_lstat_cache();
+	return sbgr;
+}
diff --git a/run-command.h b/run-command.h
index af1296769f9..58065197d1b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -496,4 +496,52 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
  */
 void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);
 
+/**
+ * Possible return values for `start_bg_command()`.
+ */
+enum start_bg_result {
+	/* child process is "ready" */
+	SBGR_READY = 0,
+
+	/* child process could not be started */
+	SBGR_ERROR,
+
+	/* callback error when testing for "ready" */
+	SBGR_CB_ERROR,
+
+	/* timeout expired waiting for child to become "ready" */
+	SBGR_TIMEOUT,
+
+	/* child process exited or was signalled before becomming "ready" */
+	SBGR_DIED,
+};
+
+/**
+ * Callback used by `start_bg_command()` to ask whether the
+ * child process is ready or needs more time to become ready.
+ *
+ * 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.
+ */
+typedef int(start_bg_wait_cb)(void *cb_data,
+			      const struct child_process *cmd);
+
+/**
+ * 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").
+ *
+ * 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 does not need to call `finish_command()`.
+ */
+enum start_bg_result start_bg_command(struct child_process *cmd,
+				      start_bg_wait_cb *wait_cb,
+				      void *cb_data,
+				      unsigned int timeout_sec);
+
 #endif
-- 
gitgitgadget

Re: [PATCH 6/7] run-command: create start_bg_command

[Taylor Blau]

On Wed, Sep 15, 2021 at 08:36:16PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Create a variation of `run_command()` and `start_command()` to launch a command
> into the background and optionally wait for it to become "ready" before returning.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  run-command.h |  48 ++++++++++++++++++++
>  2 files changed, 171 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 3e4e082e94d..fe75fd08f74 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>  	}
>  	strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
>  }
> +
> +enum start_bg_result start_bg_command(struct child_process *cmd,
> +				      start_bg_wait_cb *wait_cb,
> +				      void *cb_data,
> +				      unsigned int timeout_sec)
> +{
> +	enum start_bg_result sbgr = SBGR_ERROR;
> +	int ret;
> +	int wait_status;
> +	pid_t pid_seen;
> +	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.
> +	 */
> +	cmd->clean_on_exit = 0;
> +
> +	ret = start_command(cmd);
> +	if (ret) {
> +		/*
> +		 * We assume that if `start_command()` fails, we
> +		 * either get a complete `trace2_child_start() /
> +		 * trace2_child_exit()` pair or it fails before the
> +		 * `trace2_child_start()` is emitted, so we do not
> +		 * need to worry about it here.
> +		 *
> +		 * We also assume that `start_command()` does not add
> +		 * us to the cleanup list.  And that it calls
> +		 * calls `child_process_clear()`.
> +		 */
> +		sbgr = SBGR_ERROR;
> +		goto done;
> +	}
> +
> +	time(&time_limit);
> +	time_limit += timeout_sec;

This jumped out to me as unsafe, since POSIX guarantees time_t to be an
integral value holding a number of seconds (so += timeout_sec is safe
there), but it isn't in the C standard.

But we have lots of other examples of adding a number of seconds
directly the value filled in by time(2), so I think this is fine.

> +
> +wait:
> +	pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
> +
> +	if (pid_seen == 0) {

Small nit, probably better to write this as if (!pid_seen), but not
worth a reroll alone.

> +		/*
> +		 * 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.
> +		 */

This comment is simple and informative, thank you!

> +		ret = (*wait_cb)(cb_data, cmd);
> +		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?

Probably. Maybe trace2_child_orphaned() or trace2_child_background()?

> +			 */
> +			sbgr = SBGR_READY;
> +			goto done;
> +		} else if (ret > 0) {
> +			time_t now;
> +
> +			time(&now);
> +			if (now < time_limit)
> +				goto wait;
> +
> +			/*
> +			 * 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?
> +			 */
> +			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?
> +			 */
> +			sbgr = SBGR_CB_ERROR;
> +			goto done;
> +		}

OK, so assuming that the child is running, then we ask wait_cb what to
do. Returning zero from the callback means to background it, a positive
value means to give it more time, and negative means to cause an error.
And those match the documentation below, good.

> +	if (pid_seen == cmd->pid) {

This could be an "else if", no?

> +		int child_code = -1;
> +
> +		/*
> +		 * The child started, but exited or was terminated
> +		 * before becoming "ready".
> +		 *
> +		 * We try to match the behavior of `wait_or_whine()`
> +		 * and convert the child's status to a return code for
> +		 * tracing purposes and emit the `trace2_child_exit()`
> +		 * event.
> +		 */
> +		if (WIFEXITED(wait_status))
> +			child_code = WEXITSTATUS(wait_status);
> +		else if (WIFSIGNALED(wait_status))
> +			child_code = WTERMSIG(wait_status) + 128;

Do we care about emitting the same error (when it was signaled with
something other than SIGINT/SIGQUIT/SIGPIPE) as is reported by
wait_or_whine()?

If we want that error here, too, we could probably share the same code
here from here and in wait_or_whine(). I would probably write something
like:

    static int handle_awaited_status(int status, int *code)
    {
      if (WIFSIGNALED(status)) {
        *code = WTERMSIG(status);
        if (*code != SIGINT && *code != SIGQUIT && *code != SIGPIPE)
               error("%s died of signal %d", argv0, *code);
        /*
         * This return value is chosen so that code & 0xff
         * mimics the exit code that a POSIX shell would report for
         * a program that died from this signal.
         */
        *code += 128;
        return 1;
      } else if (WIFEXITED(status)) {
        *code = WEXITSTATUS(status);
        return 1;
      }
      return 0;
    }

so that we could call it in wait_or_whine() like:

    } else if (!handle_awaited_status(status, &code)) {
      error("waitpid is confused (%s)", argv0);
    }

and similarly here in this new function. Alternatively, if we don't want
that error, then it may help future readers to add a short comment
explaining why not.

> +/**
> + * Callback used by `start_bg_command()` to ask whether the
> + * child process is ready or needs more time to become ready.
> + *
> + * 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.
> + */
> +typedef int(start_bg_wait_cb)(void *cb_data,
> +			      const struct child_process *cmd);

Nitpicking, but typically I would assume that the "extra" void pointer
is the last argument in a callback. It definitely does not matter,
though.

Thanks,
Taylor

Re: [PATCH 6/7] run-command: create start_bg_command

[Taylor Blau]

On Thu, Sep 16, 2021 at 12:53:07AM -0400, Taylor Blau wrote:
> > +/**
> > + * Callback used by `start_bg_command()` to ask whether the
> > + * child process is ready or needs more time to become ready.
> > + *
> > + * 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.
> > + */
> > +typedef int(start_bg_wait_cb)(void *cb_data,
> > +			      const struct child_process *cmd);
>
> Nitpicking, but typically I would assume that the "extra" void pointer
> is the last argument in a callback. It definitely does not matter,
> though.

Looking at the last patch (which adds the first implementation of one of
these callbacks) it appears that this cb_data pointer is unused. I
assume that it is used in later patches which aren't in this topic?

If so, then it may help future readers to indicate as much in the patch
message. Perhaps "the cb_data argument in the start_bg_wait_cb callback
is unused in this series, but will be useful in later patches".

Thanks,
Taylor

Re: [PATCH 6/7] run-command: create start_bg_command

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Create a variation of `run_command()` and `start_command()` to launch a command
> into the background and optionally wait for it to become "ready" before returning.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  run-command.h |  48 ++++++++++++++++++++
>  2 files changed, 171 insertions(+)
>
> diff --git a/run-command.c b/run-command.c
> index 3e4e082e94d..fe75fd08f74 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>  	}
>  	strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
>  }
> +
> +enum start_bg_result start_bg_command(struct child_process *cmd,
> +				      start_bg_wait_cb *wait_cb,
> +				      void *cb_data,
> +				      unsigned int timeout_sec)
> +{
> +	enum start_bg_result sbgr = SBGR_ERROR;
> +	int ret;
> +	int wait_status;
> +	pid_t pid_seen;
> +	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.
> +	 */
> +	cmd->clean_on_exit = 0;

Since we have no existing users, can we change this to:

if (cmd->clean_on_exit)
    BUG("start_bg_command() doesn't support non-zero clean_on_exit");

Just silently discarding what the caller asked for seems like the wrong
thing to do, why the silence?

> +
> +	ret = start_command(cmd);
> +	if (ret) {
> +		/*
> +		 * We assume that if `start_command()` fails, we
> +		 * either get a complete `trace2_child_start() /
> +		 * trace2_child_exit()` pair or it fails before the
> +		 * `trace2_child_start()` is emitted, so we do not
> +		 * need to worry about it here.
> +		 *
> +		 * We also assume that `start_command()` does not add
> +		 * us to the cleanup list.  And that it calls
> +		 * calls `child_process_clear()`.
> +		 */

These all look like sensible things to assume, but I think commentary /
writing on this would be much better just documenting that the
start_command() API does this in its comment in run-command.h, or
perhaps the comment starting with "The functions: child_process_init".

> +		sbgr = SBGR_ERROR;
> +		goto done;
> +	}
> +
> +	time(&time_limit);
> +	time_limit += timeout_sec;
> +
> +wait:
> +	pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
> +
> +	if (pid_seen == 0) {
> +		/*
> +		 * 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);
> +		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?
> +			 */
> +			sbgr = SBGR_READY;
> +			goto done;

Per api-trace2.txt:

    `"child_exit"`::
            This event is generated after the current process has returned
            from the waitpid() and collected the exit information from the
            child.

My (perhaps wrong) reading of that is that yes, we should do that, after
all if we've released the child are we otherwise going to hear from them
again in any way trace2 could log with a child_exit later?

Ditto the "commentary better elsewhere" whatever the result of this
discussion is, let's add a note to api-trace2.txt about how detached
children are handled by child_exit events.

> [...]
> +			 * NEEDSWORK: Like the "ready" case, should we
> +			 * log a custom child-something Trace2 event here?
> +			 */
> +			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?
> +			 */
> +			sbgr = SBGR_CB_ERROR;
> +			goto done;

*ditto*

> +		}
> +	}
> +
> +	if (pid_seen == cmd->pid) {
> +		int child_code = -1;
> +
> +		/*
> +		 * The child started, but exited or was terminated
> +		 * before becoming "ready".
> +		 *
> +		 * We try to match the behavior of `wait_or_whine()`
> +		 * and convert the child's status to a return code for
> +		 * tracing purposes and emit the `trace2_child_exit()`
> +		 * event.
> +		 */
> +		if (WIFEXITED(wait_status))
> +			child_code = WEXITSTATUS(wait_status);
> +		else if (WIFSIGNALED(wait_status))
> +			child_code = WTERMSIG(wait_status) + 128;
> +		trace2_child_exit(cmd, child_code);

Although with the above: Perhaps I've missed some subtleties...

> [...]
> diff --git a/run-command.h b/run-command.h
> index af1296769f9..58065197d1b 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -496,4 +496,52 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
>   */
>  void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);
>  
> +/**
> + * Possible return values for `start_bg_command()`.
> + */
> +enum start_bg_result {
> +	/* child process is "ready" */
> +	SBGR_READY = 0,

Clarity nit, whenever I see an explicit enum assignment I start hunting
for things that depend on the specific value, as we sometimes do, but
there appears to be nothing...

> +	/* child process could not be started */
> +	SBGR_ERROR,
> +
> +	/* callback error when testing for "ready" */
> +	SBGR_CB_ERROR,
> +
> +	/* timeout expired waiting for child to become "ready" */
> +	SBGR_TIMEOUT,
> +
> +	/* child process exited or was signalled before becomming "ready" */
> +	SBGR_DIED,

...I'd think if we were tweaking the value we'd want to put all the
error values at < 0, but I'm fine with this pattern of them being
positive, it encourages users to use switch/case & compiler checks, and
since it's all new users...

> + * This is a combination of `start_command()` and `finish_command()`, but

Doc nit: I think with whatever syntax standard we use for /** comments
that we prefer functions like_this() not quoted `like_this()`, that
being for values like `123` or whatever. But I'm just going by a quick
grep there & what Emacs is highlighting.

[PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Convert test helper to use `start_bg_command()` when spawning a server
daemon in the background rather than blocks of platform-specific code.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------
 1 file changed, 40 insertions(+), 153 deletions(-)

diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 91345180750..59a950f3b00 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -9,6 +9,7 @@
 #include "parse-options.h"
 #include "thread-utils.h"
 #include "strvec.h"
+#include "run-command.h"
 
 #ifndef SUPPORTS_SIMPLE_IPC
 int cmd__simple_ipc(int argc, const char **argv)
@@ -274,178 +275,64 @@ static int daemon__run_server(void)
 	return ret;
 }
 
-#ifndef GIT_WINDOWS_NATIVE
-/*
- * This is adapted from `daemonize()`.  Use `fork()` to directly create and
- * run the daemon in a child process.
- */
-static int spawn_server(pid_t *pid)
-{
-	struct ipc_server_opts opts = {
-		.nr_threads = cl_args.nr_threads,
-	};
+static start_bg_wait_cb bg_wait_cb;
 
-	*pid = fork();
-
-	switch (*pid) {
-	case 0:
-		if (setsid() == -1)
-			error_errno(_("setsid failed"));
-		close(0);
-		close(1);
-		close(2);
-		sanitize_stdfds();
+static int bg_wait_cb(void *cb_data, const struct child_process *cp)
+{
+	int s = ipc_get_active_state(cl_args.path);
 
-		return ipc_server_run(cl_args.path, &opts, test_app_cb,
-				      (void*)&my_app_data);
+	switch (s) {
+	case IPC_STATE__LISTENING:
+		/* child is "ready" */
+		return 0;
 
-	case -1:
-		return error_errno(_("could not spawn daemon in the background"));
+	case IPC_STATE__NOT_LISTENING:
+	case IPC_STATE__PATH_NOT_FOUND:
+		/* give child more time */
+		return 1;
 
 	default:
-		return 0;
+	case IPC_STATE__INVALID_PATH:
+	case IPC_STATE__OTHER_ERROR:
+		/* all the time in world won't help */
+		return -1;
 	}
 }
-#else
-/*
- * Conceptually like `daemonize()` but different because Windows does not
- * have `fork(2)`.  Spawn a normal Windows child process but without the
- * limitations of `start_command()` and `finish_command()`.
- */
-static int spawn_server(pid_t *pid)
-{
-	char test_tool_exe[MAX_PATH];
-	struct strvec args = STRVEC_INIT;
-	int in, out;
-
-	GetModuleFileNameA(NULL, test_tool_exe, MAX_PATH);
-
-	in = open("/dev/null", O_RDONLY);
-	out = open("/dev/null", O_WRONLY);
-
-	strvec_push(&args, test_tool_exe);
-	strvec_push(&args, "simple-ipc");
-	strvec_push(&args, "run-daemon");
-	strvec_pushf(&args, "--name=%s", cl_args.path);
-	strvec_pushf(&args, "--threads=%d", cl_args.nr_threads);
-
-	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
-	close(in);
-	close(out);
-
-	strvec_clear(&args);
 
-	if (*pid < 0)
-		return error(_("could not spawn daemon in the background"));
-
-	return 0;
-}
-#endif
-
-/*
- * This is adapted from `wait_or_whine()`.  Watch the child process and
- * let it get started and begin listening for requests on the socket
- * before reporting our success.
- */
-static int wait_for_server_startup(pid_t pid_child)
+static int daemon__start_server(void)
 {
-	int status;
-	pid_t pid_seen;
-	enum ipc_active_state s;
-	time_t time_limit, now;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	enum start_bg_result sbgr;
 
-	time(&time_limit);
-	time_limit += cl_args.max_wait_sec;
+	strvec_push(&cp.args, "test-tool");
+	strvec_push(&cp.args, "simple-ipc");
+	strvec_push(&cp.args, "run-daemon");
+	strvec_pushf(&cp.args, "--name=%s", cl_args.path);
+	strvec_pushf(&cp.args, "--threads=%d", cl_args.nr_threads);
 
-	for (;;) {
-		pid_seen = waitpid(pid_child, &status, WNOHANG);
+	cp.no_stdin = 1;
+	cp.no_stdout = 1;
+	cp.no_stderr = 1;
 
-		if (pid_seen == -1)
-			return error_errno(_("waitpid failed"));
+	sbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec);
 
-		else if (pid_seen == 0) {
-			/*
-			 * The child is still running (this should be
-			 * the normal case).  Try to connect to it on
-			 * the socket and see if it is ready for
-			 * business.
-			 *
-			 * If there is another daemon already running,
-			 * our child will fail to start (possibly
-			 * after a timeout on the lock), but we don't
-			 * care (who responds) if the socket is live.
-			 */
-			s = ipc_get_active_state(cl_args.path);
-			if (s == IPC_STATE__LISTENING)
-				return 0;
-
-			time(&now);
-			if (now > time_limit)
-				return error(_("daemon not online yet"));
-
-			continue;
-		}
+	switch (sbgr) {
+	case SBGR_READY:
+		return 0;
 
-		else if (pid_seen == pid_child) {
-			/*
-			 * The new child daemon process shutdown while
-			 * it was starting up, so it is not listening
-			 * on the socket.
-			 *
-			 * Try to ping the socket in the odd chance
-			 * that another daemon started (or was already
-			 * running) while our child was starting.
-			 *
-			 * Again, we don't care who services the socket.
-			 */
-			s = ipc_get_active_state(cl_args.path);
-			if (s == IPC_STATE__LISTENING)
-				return 0;
+	default:
+	case SBGR_ERROR:
+	case SBGR_CB_ERROR:
+		return error("daemon failed to start");
 
-			/*
-			 * We don't care about the WEXITSTATUS() nor
-			 * any of the WIF*(status) values because
-			 * `cmd__simple_ipc()` does the `!!result`
-			 * trick on all function return values.
-			 *
-			 * So it is sufficient to just report the
-			 * early shutdown as an error.
-			 */
-			return error(_("daemon failed to start"));
-		}
+	case SBGR_TIMEOUT:
+		return error("daemon not online yet");
 
-		else
-			return error(_("waitpid is confused"));
+	case SBGR_DIED:
+		return error("daemon terminated");
 	}
 }
 
-/*
- * This process will start a simple-ipc server in a background process and
- * wait for it to become ready.  This is like `daemonize()` but gives us
- * more control and better error reporting (and makes it easier to write
- * unit tests).
- */
-static int daemon__start_server(void)
-{
-	pid_t pid_child;
-	int ret;
-
-	/*
-	 * Run the actual daemon in a background process.
-	 */
-	ret = spawn_server(&pid_child);
-	if (pid_child <= 0)
-		return ret;
-
-	/*
-	 * Let the parent wait for the child process to get started
-	 * and begin listening for requests on the socket.
-	 */
-	ret = wait_for_server_startup(pid_child);
-
-	return ret;
-}
-
 /*
  * This process will run a quick probe to see if a simple-ipc server
  * is active on this path.
-- 
gitgitgadget

Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Taylor Blau]

On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> Convert test helper to use `start_bg_command()` when spawning a server
> daemon in the background rather than blocks of platform-specific code.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------
>  1 file changed, 40 insertions(+), 153 deletions(-)
>
> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
> index 91345180750..59a950f3b00 100644
> --- a/t/helper/test-simple-ipc.c
> +++ b/t/helper/test-simple-ipc.c
> @@ -9,6 +9,7 @@
>  #include "parse-options.h"
>  #include "thread-utils.h"
>  #include "strvec.h"
> +#include "run-command.h"
>
>  #ifndef SUPPORTS_SIMPLE_IPC
>  int cmd__simple_ipc(int argc, const char **argv)
> @@ -274,178 +275,64 @@ static int daemon__run_server(void)
>  	return ret;
>  }
>
> -#ifndef GIT_WINDOWS_NATIVE
> -/*
> - * This is adapted from `daemonize()`.  Use `fork()` to directly create and
> - * run the daemon in a child process.
> - */
> -static int spawn_server(pid_t *pid)
> -{
> -	struct ipc_server_opts opts = {
> -		.nr_threads = cl_args.nr_threads,
> -	};
> +static start_bg_wait_cb bg_wait_cb;

This whole patch is delightful to read, as the new implementation is so
much cleaner as a result of the earlier work in this series.

Am I correct in assuming that this is to encourage a compiler error if
bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I
think we are already getting that by trying to pass bg_wait_cb to
start_bg_command().

E.g., applying this (intentionally broken) diff on top:

--- 8< ---

diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 59a950f3b0..3aed787206 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -275,9 +275,7 @@ static int daemon__run_server(void)
 	return ret;
 }

-static start_bg_wait_cb bg_wait_cb;
-
-static int bg_wait_cb(void *cb_data, const struct child_process *cp)
+static int bg_wait_cb(const void *cb_data, const struct child_process *cp)
 {
 	int s = ipc_get_active_state(cl_args.path);

--- >8 ---

and then compiling still warns of a mismatched type when calling
start_bg_command().

> -	*pid = fork();
> -
> -	switch (*pid) {
> -	case 0:
> -		if (setsid() == -1)
> -			error_errno(_("setsid failed"));
> -		close(0);
> -		close(1);
> -		close(2);
> -		sanitize_stdfds();
> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)
> +{
> +	int s = ipc_get_active_state(cl_args.path);
>
> -		return ipc_server_run(cl_args.path, &opts, test_app_cb,
> -				      (void*)&my_app_data);
> +	switch (s) {
> +	case IPC_STATE__LISTENING:
> +		/* child is "ready" */
> +		return 0;
>
> -	case -1:
> -		return error_errno(_("could not spawn daemon in the background"));
> +	case IPC_STATE__NOT_LISTENING:
> +	case IPC_STATE__PATH_NOT_FOUND:
> +		/* give child more time */
> +		return 1;
>
>  	default:

I'm always a little hesitant to have default cases when switch over enum
types, since it suppresses the warning when there's a new value of that
type. But we already have a similar default in client__probe_server().

> -		else if (pid_seen == pid_child) {
> -			/*
> -			 * The new child daemon process shutdown while
> -			 * it was starting up, so it is not listening
> -			 * on the socket.
> -			 *
> -			 * Try to ping the socket in the odd chance
> -			 * that another daemon started (or was already
> -			 * running) while our child was starting.
> -			 *
> -			 * Again, we don't care who services the socket.
> -			 */
> -			s = ipc_get_active_state(cl_args.path);
> -			if (s == IPC_STATE__LISTENING)
> -				return 0;
> +	default:

Ditto.

Thanks,
Taylor

Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Jeff Hostetler]

On 9/16/21 1:06 AM, Taylor Blau wrote:
> On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Convert test helper to use `start_bg_command()` when spawning a server
>> daemon in the background rather than blocks of platform-specific code.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>>   t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------
>>   1 file changed, 40 insertions(+), 153 deletions(-)
>>
>> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
>> index 91345180750..59a950f3b00 100644
>> --- a/t/helper/test-simple-ipc.c
>> +++ b/t/helper/test-simple-ipc.c
>> @@ -9,6 +9,7 @@
>>   #include "parse-options.h"
>>   #include "thread-utils.h"
>>   #include "strvec.h"
>> +#include "run-command.h"
>>
>>   #ifndef SUPPORTS_SIMPLE_IPC
>>   int cmd__simple_ipc(int argc, const char **argv)
>> @@ -274,178 +275,64 @@ static int daemon__run_server(void)
>>   	return ret;
>>   }
>>
>> -#ifndef GIT_WINDOWS_NATIVE
>> -/*
>> - * This is adapted from `daemonize()`.  Use `fork()` to directly create and
>> - * run the daemon in a child process.
>> - */
>> -static int spawn_server(pid_t *pid)
>> -{
>> -	struct ipc_server_opts opts = {
>> -		.nr_threads = cl_args.nr_threads,
>> -	};
>> +static start_bg_wait_cb bg_wait_cb;
> 
> This whole patch is delightful to read, as the new implementation is so
> much cleaner as a result of the earlier work in this series.
> 
> Am I correct in assuming that this is to encourage a compiler error if
> bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I
> think we are already getting that by trying to pass bg_wait_cb to
> start_bg_command().

I use that trick to get the compiler to give me a compiler error at the
point of the function declaration.

For example, If I add an arg to the function that doesn't match what's
in the prototype definition, I get:

t/helper/test-simple-ipc.c:280:12: error: conflicting types for 'bg_wait_cb'
static int bg_wait_cb(const struct child_process *cp, void *cb_data, int 
foo)
            ^
t/helper/test-simple-ipc.c:278:25: note: previous declaration is here
static start_bg_wait_cb bg_wait_cb;
                         ^
1 error generated.

Yes, we may get an error when the function pointer is referenced in
start_bg_command() or if we're using it to initialize a vtable or
something, but those errors are further away from the actual error
(and sometimes they can be a little cryptic).

Also, it helps document that this function's signature is predefined
for a reason.

It's a quirky trick I know, but it has served me well over the years.
	
> 
> E.g., applying this (intentionally broken) diff on top:
> 
> --- 8< ---
> 
> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
> index 59a950f3b0..3aed787206 100644
> --- a/t/helper/test-simple-ipc.c
> +++ b/t/helper/test-simple-ipc.c
> @@ -275,9 +275,7 @@ static int daemon__run_server(void)
>   	return ret;
>   }
> 
> -static start_bg_wait_cb bg_wait_cb;
> -
> -static int bg_wait_cb(void *cb_data, const struct child_process *cp)
> +static int bg_wait_cb(const void *cb_data, const struct child_process *cp)
>   {
>   	int s = ipc_get_active_state(cl_args.path);
> 
> --- >8 ---
> 
> and then compiling still warns of a mismatched type when calling
> start_bg_command().
> 
>> -	*pid = fork();
>> -
>> -	switch (*pid) {
>> -	case 0:
>> -		if (setsid() == -1)
>> -			error_errno(_("setsid failed"));
>> -		close(0);
>> -		close(1);
>> -		close(2);
>> -		sanitize_stdfds();
>> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)
>> +{
>> +	int s = ipc_get_active_state(cl_args.path);
>>
>> -		return ipc_server_run(cl_args.path, &opts, test_app_cb,
>> -				      (void*)&my_app_data);
>> +	switch (s) {
>> +	case IPC_STATE__LISTENING:
>> +		/* child is "ready" */
>> +		return 0;
>>
>> -	case -1:
>> -		return error_errno(_("could not spawn daemon in the background"));
>> +	case IPC_STATE__NOT_LISTENING:
>> +	case IPC_STATE__PATH_NOT_FOUND:
>> +		/* give child more time */
>> +		return 1;
>>
>>   	default:
> 
> I'm always a little hesitant to have default cases when switch over enum
> types, since it suppresses the warning when there's a new value of that
> type. But we already have a similar default in client__probe_server().

Do all compilers now handle switching over an enum and detect unhandled
cases?  Once upon a time that wasn't the case IIRC.

>...
> 
> Ditto.
> 
> Thanks,
> Taylor
> 

Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ævar Arnfjörð Bjarmason]

On Fri, Sep 17 2021, Jeff Hostetler wrote:

> On 9/16/21 1:06 AM, Taylor Blau wrote:
>> On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:
>>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>>
>>> Convert test helper to use `start_bg_command()` when spawning a server
>>> daemon in the background rather than blocks of platform-specific code.
>>>
>>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>>> ---
>>>   t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------
>>>   1 file changed, 40 insertions(+), 153 deletions(-)
>>>
>>> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
>>> index 91345180750..59a950f3b00 100644
>>> --- a/t/helper/test-simple-ipc.c
>>> +++ b/t/helper/test-simple-ipc.c
>>> @@ -9,6 +9,7 @@
>>>   #include "parse-options.h"
>>>   #include "thread-utils.h"
>>>   #include "strvec.h"
>>> +#include "run-command.h"
>>>
>>>   #ifndef SUPPORTS_SIMPLE_IPC
>>>   int cmd__simple_ipc(int argc, const char **argv)
>>> @@ -274,178 +275,64 @@ static int daemon__run_server(void)
>>>   	return ret;
>>>   }
>>>
>>> -#ifndef GIT_WINDOWS_NATIVE
>>> -/*
>>> - * This is adapted from `daemonize()`.  Use `fork()` to directly create and
>>> - * run the daemon in a child process.
>>> - */
>>> -static int spawn_server(pid_t *pid)
>>> -{
>>> -	struct ipc_server_opts opts = {
>>> -		.nr_threads = cl_args.nr_threads,
>>> -	};
>>> +static start_bg_wait_cb bg_wait_cb;
>> This whole patch is delightful to read, as the new implementation is
>> so
>> much cleaner as a result of the earlier work in this series.
>> Am I correct in assuming that this is to encourage a compiler error
>> if
>> bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I
>> think we are already getting that by trying to pass bg_wait_cb to
>> start_bg_command().
>
> I use that trick to get the compiler to give me a compiler error at the
> point of the function declaration.
>
> For example, If I add an arg to the function that doesn't match what's
> in the prototype definition, I get:
>
> t/helper/test-simple-ipc.c:280:12: error: conflicting types for 'bg_wait_cb'
> static int bg_wait_cb(const struct child_process *cp, void *cb_data,
> int foo)
>            ^
> t/helper/test-simple-ipc.c:278:25: note: previous declaration is here
> static start_bg_wait_cb bg_wait_cb;
>                         ^
> 1 error generated.
>
> Yes, we may get an error when the function pointer is referenced in
> start_bg_command() or if we're using it to initialize a vtable or
> something, but those errors are further away from the actual error
> (and sometimes they can be a little cryptic).
>
> Also, it helps document that this function's signature is predefined
> for a reason.
>
> It's a quirky trick I know, but it has served me well over the years.

I haven't seen this idiom before. I think it's best to avoid patterns
designed to massage messages out of any specific compilers/versions.

It seems inevitable that it'll either be counter-productive or
redundant. Here with clang v11 doing this makes the warning
worse. I.e. without the forward declaration:

    t/helper/test-simple-ipc.c:315:31: error: incompatible function
    pointer types passing 'int (void *, const struct child_process *,
    int)' to parameter of type ' start_bg_wait_cb *' (aka 'int (*)(void
    *, const struct child_process *)')
    [-Werror,-Wincompatible-function-pointer-types]
            sbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec);
                                         ^~~~~~~~~~
    ./run-command.h:564:29: note: passing argument to parameter 'wait_cb' here
                                      start_bg_wait_cb *wait_cb,
                                                        ^
    1 error generated.

I.e. we get the specific warning category for this type of error
(-Werror,-Wincompatible-function-pointer-types), and we're pointed at
the caller in question (which to be fair, it seems you don't prefer),
but also a reference to the run-command.h definition.

Most importantly, we get quoted what the type is/should be, which is
missing with the forward declaration. It's the equivalent of saying "you
did bad!" instead of "you did bad X, do Y instead!".

>> E.g., applying this (intentionally broken) diff on top:
>> --- 8< ---
>> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
>> index 59a950f3b0..3aed787206 100644
>> --- a/t/helper/test-simple-ipc.c
>> +++ b/t/helper/test-simple-ipc.c
>> @@ -275,9 +275,7 @@ static int daemon__run_server(void)
>>   	return ret;
>>   }
>> -static start_bg_wait_cb bg_wait_cb;
>> -
>> -static int bg_wait_cb(void *cb_data, const struct child_process *cp)
>> +static int bg_wait_cb(const void *cb_data, const struct child_process *cp)
>>   {
>>   	int s = ipc_get_active_state(cl_args.path);
>> --- >8 ---
>> and then compiling still warns of a mismatched type when calling
>> start_bg_command().
>> 
>>> -	*pid = fork();
>>> -
>>> -	switch (*pid) {
>>> -	case 0:
>>> -		if (setsid() == -1)
>>> -			error_errno(_("setsid failed"));
>>> -		close(0);
>>> -		close(1);
>>> -		close(2);
>>> -		sanitize_stdfds();
>>> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)
>>> +{
>>> +	int s = ipc_get_active_state(cl_args.path);
>>>
>>> -		return ipc_server_run(cl_args.path, &opts, test_app_cb,
>>> -				      (void*)&my_app_data);
>>> +	switch (s) {
>>> +	case IPC_STATE__LISTENING:
>>> +		/* child is "ready" */
>>> +		return 0;
>>>
>>> -	case -1:
>>> -		return error_errno(_("could not spawn daemon in the background"));
>>> +	case IPC_STATE__NOT_LISTENING:
>>> +	case IPC_STATE__PATH_NOT_FOUND:
>>> +		/* give child more time */
>>> +		return 1;
>>>
>>>   	default:
>> I'm always a little hesitant to have default cases when switch over
>> enum
>> types, since it suppresses the warning when there's a new value of that
>> type. But we already have a similar default in client__probe_server().
>
> Do all compilers now handle switching over an enum and detect unhandled
> cases?  Once upon a time that wasn't the case IIRC.

I don't think so, but the ones we widely use do, i.e. clang and gcc at
least.

For this sort of thing it really doesn't matter if *all* compilers
support it, since we'll only need to catch such "missing enum arm"
issues with one of them.

E.g. in my 338abb0f045 (builtins + test helpers: use return instead of
exit() in cmd_*, 2021-06-08) I fixed something that I've only gotten
Oracle SunCC to emit (gcc and clang don't detect it), but as long as
that one compiler does & someone checks it regularly...

By having a "default" case you're hiding that detection from the
compilers capable of detecting a logic error in this code, whereas if
the compiler can't do that it'll just ignore it.

Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ævar Arnfjörð Bjarmason]

On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:

[...]

> +	default:
> +	case SBGR_ERROR:
> +	case SBGR_CB_ERROR:
> +		return error("daemon failed to start");
>  
> -			/*
> -			 * We don't care about the WEXITSTATUS() nor
> -			 * any of the WIF*(status) values because
> -			 * `cmd__simple_ipc()` does the `!!result`
> -			 * trick on all function return values.
> -			 *
> -			 * So it is sufficient to just report the
> -			 * early shutdown as an error.
> -			 */
> -			return error(_("daemon failed to start"));
> -		}
> +	case SBGR_TIMEOUT:
> +		return error("daemon not online yet");
>  
> -		else
> -			return error(_("waitpid is confused"));
> +	case SBGR_DIED:
> +		return error("daemon terminated");
>  	}
>  }

It's not mentioned in the commit message, but the while-we're-at-it
dropping of _() makes sense here, it shouldn't have been used in a test
helper to begin with. I.e. translators don't need to be translating
stuff purely internal to the test suite.

[PATCH v2 0/7] Builtin FSMonitor Part 1

[Jeff Hostetler via GitGitGadget]

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");
     + 	}
     + }
     + 

-- 
gitgitgadget

[PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create "child_ready" event to capture the state of a child process
created in the background.

When a child command is started a "child_start" event is generated in
the Trace2 log.  For normal synchronous children, a "child_exit" event
is later generated when the child exits or is terminated.  The two events
include information, such as the "child_id" and "pid", to allow post
analysis to match-up the command line and exit status.

When a child is started in the background (and may outlive the parent
process), it is not possible for the parent to emit a "child_exit"
event.  Create a new "child_ready" event to indicate whether the
child was successfully started.  Also include the "child_id" and "pid"
to allow similar post processing.

This will be used in a later commit with the new "start_bg_command()".

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/technical/api-trace2.txt | 40 ++++++++++++++++++++++++++
 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 ++++++++++
 7 files changed, 152 insertions(+)

diff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt
index b9f3198fbe7..ef7fe02a8f7 100644
--- a/Documentation/technical/api-trace2.txt
+++ b/Documentation/technical/api-trace2.txt
@@ -613,6 +613,46 @@ stopping after the waitpid() and includes OS process creation overhead).
 So this time will be slightly larger than the atexit time reported by
 the child process itself.
 
+`"child_ready"`::
+	This event is generated after the current process has started
+	a background process and released all handles to it.
++
+------------
+{
+	"event":"child_ready",
+	...
+	"child_id":2,
+	"pid":14708,	 # child PID
+	"ready":"ready", # child ready state
+	"t_rel":0.110605 # observed run-time of child process
+}
+------------
++
+Note that the session-id of the child process is not available to
+the current/spawning process, so the child's PID is reported here as
+a hint for post-processing.  (But it is only a hint because the child
+process may be a shell script which doesn't have a session-id.)
++
+This event is generated after the child is started in the background
+and given a little time to boot up and start working.  If the child
+startups normally and while the parent is still waiting, the "ready"
+field will have the value "ready".
+If the child is too slow to start and the parent times out, the field
+will have the value "timeout".
+If the child starts but the parent is unable to probe it, the field
+will have the value "error".
++
+After the parent process emits this event, it will release all of its
+handles to the child process and treat the child as a background
+daemon.  So even if the child does eventually finish booting up,
+the parent will not emit an updated event.
++
+Note that the `t_rel` field contains the observed run time in seconds
+when the parent released the child process into the background.
+The child is assumed to be a long-running daemon process and may
+outlive the parent process.  So the parent's child event times should
+not be compared to the child's atexit times.
+
 `"exec"`::
 	This event is generated before git attempts to `exec()`
 	another command rather than starting a child process.
diff --git a/trace2.c b/trace2.c
index b9b154ac440..b2d471526fd 100644
--- a/trace2.c
+++ b/trace2.c
@@ -394,6 +394,37 @@ void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,
 						 us_elapsed_child);
 }
 
+void trace2_child_ready_fl(const char *file, int line,
+			   struct child_process *cmd,
+			   const char *ready)
+{
+	struct tr2_tgt *tgt_j;
+	int j;
+	uint64_t us_now;
+	uint64_t us_elapsed_absolute;
+	uint64_t us_elapsed_child;
+
+	if (!trace2_enabled)
+		return;
+
+	us_now = getnanotime() / 1000;
+	us_elapsed_absolute = tr2tls_absolute_elapsed(us_now);
+
+	if (cmd->trace2_child_us_start)
+		us_elapsed_child = us_now - cmd->trace2_child_us_start;
+	else
+		us_elapsed_child = 0;
+
+	for_each_wanted_builtin (j, tgt_j)
+		if (tgt_j->pfn_child_ready_fl)
+			tgt_j->pfn_child_ready_fl(file, line,
+						  us_elapsed_absolute,
+						  cmd->trace2_child_id,
+						  cmd->pid,
+						  ready,
+						  us_elapsed_child);
+}
+
 int trace2_exec_fl(const char *file, int line, const char *exe,
 		   const char **argv)
 {
diff --git a/trace2.h b/trace2.h
index 9b7286c572f..6fe9802598b 100644
--- a/trace2.h
+++ b/trace2.h
@@ -253,6 +253,31 @@ void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,
 #define trace2_child_exit(cmd, code) \
 	trace2_child_exit_fl(__FILE__, __LINE__, (cmd), (code))
 
+/**
+ * Emits a "child_ready" message containing the "child-id" and a flag
+ * indicating whether the child was considered "ready" when we
+ * released it.
+ *
+ * This function should be called after starting a daemon process in
+ * the background (and after giving it sufficient time to boot
+ * up) to indicate that we no longer control or own it.
+ *
+ * The "ready" argument should contain one of { "ready", "timeout",
+ * "error" } to indicate the state of the running daemon when we
+ * released it.
+ *
+ * If the daemon process fails to start or it exits or is terminated
+ * while we are still waiting for it, the caller should emit a
+ * regular "child_exit" to report the normal process exit information.
+ *
+ */
+void trace2_child_ready_fl(const char *file, int line,
+			   struct child_process *cmd,
+			   const char *ready);
+
+#define trace2_child_ready(cmd, ready) \
+	trace2_child_ready_fl(__FILE__, __LINE__, (cmd), (ready))
+
 /**
  * Emit an 'exec' event prior to calling one of exec(), execv(),
  * execvp(), and etc.  On Unix-derived systems, this will be the
diff --git a/trace2/tr2_tgt.h b/trace2/tr2_tgt.h
index 1f66fd65730..65f94e15748 100644
--- a/trace2/tr2_tgt.h
+++ b/trace2/tr2_tgt.h
@@ -45,6 +45,10 @@ typedef void(tr2_tgt_evt_child_exit_fl_t)(const char *file, int line,
 					  uint64_t us_elapsed_absolute, int cid,
 					  int pid, int code,
 					  uint64_t us_elapsed_child);
+typedef void(tr2_tgt_evt_child_ready_fl_t)(const char *file, int line,
+					   uint64_t us_elapsed_absolute,
+					   int cid, int pid, const char *ready,
+					   uint64_t us_elapsed_child);
 
 typedef void(tr2_tgt_evt_thread_start_fl_t)(const char *file, int line,
 					    uint64_t us_elapsed_absolute);
@@ -116,6 +120,7 @@ struct tr2_tgt {
 	tr2_tgt_evt_alias_fl_t                  *pfn_alias_fl;
 	tr2_tgt_evt_child_start_fl_t            *pfn_child_start_fl;
 	tr2_tgt_evt_child_exit_fl_t             *pfn_child_exit_fl;
+	tr2_tgt_evt_child_ready_fl_t            *pfn_child_ready_fl;
 	tr2_tgt_evt_thread_start_fl_t           *pfn_thread_start_fl;
 	tr2_tgt_evt_thread_exit_fl_t            *pfn_thread_exit_fl;
 	tr2_tgt_evt_exec_fl_t                   *pfn_exec_fl;
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 578a9a5287a..70cfc2f77cc 100644
--- a/trace2/tr2_tgt_event.c
+++ b/trace2/tr2_tgt_event.c
@@ -383,6 +383,27 @@ static void fn_child_exit_fl(const char *file, int line,
 	jw_release(&jw);
 }
 
+static void fn_child_ready_fl(const char *file, int line,
+			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      const char *ready, uint64_t us_elapsed_child)
+{
+	const char *event_name = "child_ready";
+	struct json_writer jw = JSON_WRITER_INIT;
+	double t_rel = (double)us_elapsed_child / 1000000.0;
+
+	jw_object_begin(&jw, 0);
+	event_fmt_prepare(event_name, file, line, NULL, &jw);
+	jw_object_intmax(&jw, "child_id", cid);
+	jw_object_intmax(&jw, "pid", pid);
+	jw_object_string(&jw, "ready", ready);
+	jw_object_double(&jw, "t_rel", 6, t_rel);
+	jw_end(&jw);
+
+	tr2_dst_write_line(&tr2dst_event, &jw.json);
+
+	jw_release(&jw);
+}
+
 static void fn_thread_start_fl(const char *file, int line,
 			       uint64_t us_elapsed_absolute)
 {
@@ -610,6 +631,7 @@ struct tr2_tgt tr2_tgt_event = {
 	fn_alias_fl,
 	fn_child_start_fl,
 	fn_child_exit_fl,
+	fn_child_ready_fl,
 	fn_thread_start_fl,
 	fn_thread_exit_fl,
 	fn_exec_fl,
diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c
index a5751c88644..58d9e430f05 100644
--- a/trace2/tr2_tgt_normal.c
+++ b/trace2/tr2_tgt_normal.c
@@ -251,6 +251,19 @@ static void fn_child_exit_fl(const char *file, int line,
 	strbuf_release(&buf_payload);
 }
 
+static void fn_child_ready_fl(const char *file, int line,
+			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      const char *ready, uint64_t us_elapsed_child)
+{
+	struct strbuf buf_payload = STRBUF_INIT;
+	double elapsed = (double)us_elapsed_child / 1000000.0;
+
+	strbuf_addf(&buf_payload, "child_ready[%d] pid:%d ready:%s elapsed:%.6f",
+		    cid, pid, ready, elapsed);
+	normal_io_write_fl(file, line, &buf_payload);
+	strbuf_release(&buf_payload);
+}
+
 static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,
 		       int exec_id, const char *exe, const char **argv)
 {
@@ -330,6 +343,7 @@ struct tr2_tgt tr2_tgt_normal = {
 	fn_alias_fl,
 	fn_child_start_fl,
 	fn_child_exit_fl,
+	fn_child_ready_fl,
 	NULL, /* thread_start */
 	NULL, /* thread_exit */
 	fn_exec_fl,
diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c
index af4d65a0a5f..e4acca13d64 100644
--- a/trace2/tr2_tgt_perf.c
+++ b/trace2/tr2_tgt_perf.c
@@ -360,6 +360,20 @@ static void fn_child_exit_fl(const char *file, int line,
 	strbuf_release(&buf_payload);
 }
 
+static void fn_child_ready_fl(const char *file, int line,
+			      uint64_t us_elapsed_absolute, int cid, int pid,
+			      const char *ready, uint64_t us_elapsed_child)
+{
+	const char *event_name = "child_ready";
+	struct strbuf buf_payload = STRBUF_INIT;
+
+	strbuf_addf(&buf_payload, "[ch%d] pid:%d ready:%s", cid, pid, ready);
+
+	perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,
+			 &us_elapsed_child, NULL, &buf_payload);
+	strbuf_release(&buf_payload);
+}
+
 static void fn_thread_start_fl(const char *file, int line,
 			       uint64_t us_elapsed_absolute)
 {
@@ -553,6 +567,7 @@ struct tr2_tgt tr2_tgt_perf = {
 	fn_alias_fl,
 	fn_child_start_fl,
 	fn_child_exit_fl,
+	fn_child_ready_fl,
 	fn_thread_start_fl,
 	fn_thread_exit_fl,
 	fn_exec_fl,
-- 
gitgitgadget

[PATCH v2 2/7] simple-ipc: preparations for supporting binary messages.

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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`
argument was just a `const char *`.

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.

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>
---
 compat/simple-ipc/ipc-unix-socket.c | 14 +++++++-----
 compat/simple-ipc/ipc-win32.c       | 14 +++++++-----
 simple-ipc.h                        |  7 ++++--
 t/helper/test-simple-ipc.c          | 34 +++++++++++++++++++----------
 4 files changed, 46 insertions(+), 23 deletions(-)

diff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c
index 1927e6ef4bc..4e28857a0a1 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -168,7 +168,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)
 
 int ipc_client_send_command_to_connection(
 	struct ipc_client_connection *connection,
-	const char *message, struct strbuf *answer)
+	const char *message, size_t message_len,
+	struct strbuf *answer)
 {
 	int ret = 0;
 
@@ -176,7 +177,7 @@ int ipc_client_send_command_to_connection(
 
 	trace2_region_enter("ipc-client", "send-command", NULL);
 
-	if (write_packetized_from_buf_no_flush(message, strlen(message),
+	if (write_packetized_from_buf_no_flush(message, message_len,
 					       connection->fd) < 0 ||
 	    packet_flush_gently(connection->fd) < 0) {
 		ret = error(_("could not send IPC command"));
@@ -197,7 +198,8 @@ done:
 
 int ipc_client_send_command(const char *path,
 			    const struct ipc_client_connect_options *options,
-			    const char *message, struct strbuf *answer)
+			    const char *message, size_t message_len,
+			    struct strbuf *answer)
 {
 	int ret = -1;
 	enum ipc_active_state state;
@@ -208,7 +210,9 @@ int ipc_client_send_command(const char *path,
 	if (state != IPC_STATE__LISTENING)
 		return ret;
 
-	ret = ipc_client_send_command_to_connection(connection, message, answer);
+	ret = ipc_client_send_command_to_connection(connection,
+						    message, message_len,
+						    answer);
 
 	ipc_client_close_connection(connection);
 
@@ -503,7 +507,7 @@ static int worker_thread__do_io(
 	if (ret >= 0) {
 		ret = worker_thread_data->server_data->application_cb(
 			worker_thread_data->server_data->application_data,
-			buf.buf, do_io_reply_callback, &reply_data);
+			buf.buf, buf.len, do_io_reply_callback, &reply_data);
 
 		packet_flush_gently(reply_data.fd);
 	}
diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 8dc7bda087d..8e889d6a506 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -208,7 +208,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)
 
 int ipc_client_send_command_to_connection(
 	struct ipc_client_connection *connection,
-	const char *message, struct strbuf *answer)
+	const char *message, size_t message_len,
+	struct strbuf *answer)
 {
 	int ret = 0;
 
@@ -216,7 +217,7 @@ int ipc_client_send_command_to_connection(
 
 	trace2_region_enter("ipc-client", "send-command", NULL);
 
-	if (write_packetized_from_buf_no_flush(message, strlen(message),
+	if (write_packetized_from_buf_no_flush(message, message_len,
 					       connection->fd) < 0 ||
 	    packet_flush_gently(connection->fd) < 0) {
 		ret = error(_("could not send IPC command"));
@@ -239,7 +240,8 @@ done:
 
 int ipc_client_send_command(const char *path,
 			    const struct ipc_client_connect_options *options,
-			    const char *message, struct strbuf *response)
+			    const char *message, size_t message_len,
+			    struct strbuf *response)
 {
 	int ret = -1;
 	enum ipc_active_state state;
@@ -250,7 +252,9 @@ int ipc_client_send_command(const char *path,
 	if (state != IPC_STATE__LISTENING)
 		return ret;
 
-	ret = ipc_client_send_command_to_connection(connection, message, response);
+	ret = ipc_client_send_command_to_connection(connection,
+						    message, message_len,
+						    response);
 
 	ipc_client_close_connection(connection);
 
@@ -458,7 +462,7 @@ static int do_io(struct ipc_server_thread_data *server_thread_data)
 	if (ret >= 0) {
 		ret = server_thread_data->server_data->application_cb(
 			server_thread_data->server_data->application_data,
-			buf.buf, do_io_reply_callback, &reply_data);
+			buf.buf, buf.len, do_io_reply_callback, &reply_data);
 
 		packet_flush_gently(reply_data.fd);
 
diff --git a/simple-ipc.h b/simple-ipc.h
index 2c48a5ee004..9c7330fcda0 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -107,7 +107,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection);
  */
 int ipc_client_send_command_to_connection(
 	struct ipc_client_connection *connection,
-	const char *message, struct strbuf *answer);
+	const char *message, size_t message_len,
+	struct strbuf *answer);
 
 /*
  * Used by the client to synchronously connect and send and receive a
@@ -119,7 +120,8 @@ int ipc_client_send_command_to_connection(
  */
 int ipc_client_send_command(const char *path,
 			    const struct ipc_client_connect_options *options,
-			    const char *message, struct strbuf *answer);
+			    const char *message, size_t message_len,
+			    struct strbuf *answer);
 
 /*
  * Simple IPC Server Side API.
@@ -144,6 +146,7 @@ typedef int (ipc_server_reply_cb)(struct ipc_server_reply_data *,
  */
 typedef int (ipc_server_application_cb)(void *application_data,
 					const char *request,
+					size_t request_len,
 					ipc_server_reply_cb *reply_cb,
 					struct ipc_server_reply_data *reply_data);
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 42040ef81b1..91345180750 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -112,7 +112,7 @@ static int app__slow_command(ipc_server_reply_cb *reply_cb,
 /*
  * The client sent a command followed by a (possibly very) large buffer.
  */
-static int app__sendbytes_command(const char *received,
+static int app__sendbytes_command(const char *received, size_t received_len,
 				  ipc_server_reply_cb *reply_cb,
 				  struct ipc_server_reply_data *reply_data)
 {
@@ -123,6 +123,13 @@ static int app__sendbytes_command(const char *received,
 	int errs = 0;
 	int ret;
 
+	/*
+	 * The test is setup to send:
+	 *     "sendbytes" SP <n * char>
+	 */
+	if (received_len < strlen("sendbytes "))
+		BUG("received_len is short in app__sendbytes_command");
+
 	if (skip_prefix(received, "sendbytes ", &p))
 		len_ballast = strlen(p);
 
@@ -160,7 +167,7 @@ static ipc_server_application_cb test_app_cb;
  * by this application.
  */
 static int test_app_cb(void *application_data,
-		       const char *command,
+		       const char *command, size_t command_len,
 		       ipc_server_reply_cb *reply_cb,
 		       struct ipc_server_reply_data *reply_data)
 {
@@ -173,7 +180,7 @@ static int test_app_cb(void *application_data,
 	if (application_data != (void*)&my_app_data)
 		BUG("application_cb: application_data pointer wrong");
 
-	if (!strcmp(command, "quit")) {
+	if (command_len == 4 && !strncmp(command, "quit", 4)) {
 		/*
 		 * The client sent a "quit" command.  This is an async
 		 * request for the server to shutdown.
@@ -193,22 +200,23 @@ static int test_app_cb(void *application_data,
 		return SIMPLE_IPC_QUIT;
 	}
 
-	if (!strcmp(command, "ping")) {
+	if (command_len == 4 && !strncmp(command, "ping", 4)) {
 		const char *answer = "pong";
 		return reply_cb(reply_data, answer, strlen(answer));
 	}
 
-	if (!strcmp(command, "big"))
+	if (command_len == 3 && !strncmp(command, "big", 3))
 		return app__big_command(reply_cb, reply_data);
 
-	if (!strcmp(command, "chunk"))
+	if (command_len == 5 && !strncmp(command, "chunk", 5))
 		return app__chunk_command(reply_cb, reply_data);
 
-	if (!strcmp(command, "slow"))
+	if (command_len == 4 && !strncmp(command, "slow", 4))
 		return app__slow_command(reply_cb, reply_data);
 
-	if (starts_with(command, "sendbytes "))
-		return app__sendbytes_command(command, reply_cb, reply_data);
+	if (command_len >= 10 && starts_with(command, "sendbytes "))
+		return app__sendbytes_command(command, command_len,
+					      reply_cb, reply_data);
 
 	return app__unhandled_command(command, reply_cb, reply_data);
 }
@@ -488,7 +496,9 @@ static int client__send_ipc(void)
 	options.wait_if_busy = 1;
 	options.wait_if_not_found = 0;
 
-	if (!ipc_client_send_command(cl_args.path, &options, command, &buf)) {
+	if (!ipc_client_send_command(cl_args.path, &options,
+				     command, strlen(command),
+				     &buf)) {
 		if (buf.len) {
 			printf("%s\n", buf.buf);
 			fflush(stdout);
@@ -556,7 +566,9 @@ static int do_sendbytes(int bytecount, char byte, const char *path,
 	strbuf_addstr(&buf_send, "sendbytes ");
 	strbuf_addchars(&buf_send, byte, bytecount);
 
-	if (!ipc_client_send_command(path, options, buf_send.buf, &buf_resp)) {
+	if (!ipc_client_send_command(path, options,
+				     buf_send.buf, buf_send.len,
+				     &buf_resp)) {
 		strbuf_rtrim(&buf_resp);
 		printf("sent:%c%08d %s\n", byte, bytecount, buf_resp.buf);
 		fflush(stdout);
-- 
gitgitgadget

[PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>

Move the declartion of the `enum ipc_active_state` type outside of
the SUPPORTS_SIMPLE_IPC ifdef.

A later commit will introduce the `fsmonitor_ipc__*()` API and stub in
a "mock" implementation that requires this enum in some function
signatures.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 simple-ipc.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/simple-ipc.h b/simple-ipc.h
index 9c7330fcda0..b396293bdfc 100644
--- a/simple-ipc.h
+++ b/simple-ipc.h
@@ -5,13 +5,6 @@
  * See Documentation/technical/api-simple-ipc.txt
  */
 
-#ifdef SUPPORTS_SIMPLE_IPC
-#include "pkt-line.h"
-
-/*
- * Simple IPC Client Side API.
- */
-
 enum ipc_active_state {
 	/*
 	 * The pipe/socket exists and the daemon is waiting for connections.
@@ -43,6 +36,13 @@ enum ipc_active_state {
 	IPC_STATE__OTHER_ERROR,
 };
 
+#ifdef SUPPORTS_SIMPLE_IPC
+#include "pkt-line.h"
+
+/*
+ * Simple IPC Client Side API.
+ */
+
 struct ipc_client_connect_options {
 	/*
 	 * Spin under timeout if the server is running but can't
-- 
gitgitgadget

[PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create "ipc-debug" category events to log unexpected errors
when creating Simple-IPC connections.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/simple-ipc/ipc-win32.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 8e889d6a506..a8dd46bd922 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -49,6 +49,9 @@ static enum ipc_active_state get_active_state(wchar_t *pipe_path)
 	if (GetLastError() == ERROR_FILE_NOT_FOUND)
 		return IPC_STATE__PATH_NOT_FOUND;
 
+	trace2_data_intmax("ipc-debug", NULL, "getstate/waitpipe/gle",
+			   (intmax_t)GetLastError());
+
 	return IPC_STATE__OTHER_ERROR;
 }
 
@@ -109,9 +112,15 @@ static enum ipc_active_state connect_to_server(
 			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;
 
+				trace2_data_intmax("ipc-debug", NULL,
+						   "connect/waitpipe/gle",
+						   (intmax_t)gleWait);
+
 				return IPC_STATE__OTHER_ERROR;
 			}
 
@@ -133,17 +142,31 @@ static enum ipc_active_state connect_to_server(
 			break; /* try again */
 
 		default:
+			trace2_data_intmax("ipc-debug", NULL,
+					   "connect/createfile/gle",
+					   (intmax_t)gle);
+
 			return IPC_STATE__OTHER_ERROR;
 		}
 	}
 
 	if (!SetNamedPipeHandleState(hPipe, &mode, NULL, NULL)) {
+		gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL,
+				   "connect/setpipestate/gle",
+				   (intmax_t)gle);
+
 		CloseHandle(hPipe);
 		return IPC_STATE__OTHER_ERROR;
 	}
 
 	*pfd = _open_osfhandle((intptr_t)hPipe, O_RDWR|O_BINARY);
 	if (*pfd < 0) {
+		gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL,
+				   "connect/openosfhandle/gle",
+				   (intmax_t)gle);
+
 		CloseHandle(hPipe);
 		return IPC_STATE__OTHER_ERROR;
 	}
-- 
gitgitgadget

[PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Set an ACL on the named pipe to allow the well-known group EVERYONE
to read and write to the IPC server's named pipe.  In the event that
the daemon was started with elevation, allow non-elevated clients
to communicate with the daemon.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 compat/simple-ipc/ipc-win32.c | 140 +++++++++++++++++++++++++++++++---
 1 file changed, 129 insertions(+), 11 deletions(-)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index a8dd46bd922..20ea7b65e0b 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -3,6 +3,8 @@
 #include "strbuf.h"
 #include "pkt-line.h"
 #include "thread-utils.h"
+#include "accctrl.h"
+#include "aclapi.h"
 
 #ifndef SUPPORTS_SIMPLE_IPC
 /*
@@ -592,11 +594,132 @@ finished:
 	return NULL;
 }
 
+/*
+ * We need to build a Windows "SECURITY_ATTRIBUTES" object and use it
+ * to apply an ACL when we create the initial instance of the Named
+ * Pipe.  The construction is somewhat involved and consists of
+ * several sequential steps and intermediate objects.
+ *
+ * We use this structure to hold these intermediate pointers so that
+ * we can free them as a group.  (It is unclear from the docs whether
+ * some of these intermediate pointers can be freed before we are
+ * finished using the "lpSA" member.)
+ */
+struct my_sa_data
+{
+	PSID pEveryoneSID;
+	PACL pACL;
+	PSECURITY_DESCRIPTOR pSD;
+	LPSECURITY_ATTRIBUTES lpSA;
+};
+
+static void init_sa(struct my_sa_data *d)
+{
+	memset(d, 0, sizeof(*d));
+}
+
+static void release_sa(struct my_sa_data *d)
+{
+	if (d->pEveryoneSID)
+		FreeSid(d->pEveryoneSID);
+	if (d->pACL)
+		LocalFree(d->pACL);
+	if (d->pSD)
+		LocalFree(d->pSD);
+	if (d->lpSA)
+		LocalFree(d->lpSA);
+
+	memset(d, 0, sizeof(*d));
+}
+
+/*
+ * Create SECURITY_ATTRIBUTES to apply to the initial named pipe.  The
+ * creator of the first server instance gets to set the ACLs on it.
+ *
+ * We allow the well-known group `EVERYONE` to have read+write access
+ * to the named pipe so that clients can send queries to the daemon
+ * and receive the response.
+ *
+ * Normally, this is not necessary since the daemon is usually
+ * automatically started by a foreground command like `git status`,
+ * but in those cases where an elevated Git command started the daemon
+ * (such that the daemon itself runs with elevation), we need to add
+ * the ACL so that non-elevated commands can write to it.
+ *
+ * The following document was helpful:
+ * https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c--
+ *
+ * Returns d->lpSA set to a SA or NULL.
+ */
+static LPSECURITY_ATTRIBUTES get_sa(struct my_sa_data *d)
+{
+	SID_IDENTIFIER_AUTHORITY sid_auth_world = SECURITY_WORLD_SID_AUTHORITY;
+#define NR_EA (1)
+	EXPLICIT_ACCESS ea[NR_EA];
+	DWORD dwResult;
+
+	if (!AllocateAndInitializeSid(&sid_auth_world, 1,
+				      SECURITY_WORLD_RID, 0,0,0,0,0,0,0,
+				      &d->pEveryoneSID)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "alloc-world-sid/gle",
+				   (intmax_t)gle);
+		goto fail;
+	}
+
+	memset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS));
+
+	ea[0].grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;
+	ea[0].grfAccessMode = SET_ACCESS;
+	ea[0].grfInheritance = NO_INHERITANCE;
+	ea[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;
+	ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
+	ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
+	ea[0].Trustee.ptstrName = (LPTSTR)d->pEveryoneSID;
+
+	dwResult = SetEntriesInAcl(NR_EA, ea, NULL, &d->pACL);
+	if (dwResult != ERROR_SUCCESS) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "set-acl-entry/gle",
+				   (intmax_t)gle);
+		trace2_data_intmax("ipc-debug", NULL, "set-acl-entry/dw",
+				   (intmax_t)dwResult);
+		goto fail;
+	}
+
+	d->pSD = (PSECURITY_DESCRIPTOR)LocalAlloc(
+		LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);
+	if (!InitializeSecurityDescriptor(d->pSD, SECURITY_DESCRIPTOR_REVISION)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "init-sd/gle", (intmax_t)gle);
+		goto fail;
+	}
+
+	if (!SetSecurityDescriptorDacl(d->pSD, TRUE, d->pACL, FALSE)) {
+		DWORD gle = GetLastError();
+		trace2_data_intmax("ipc-debug", NULL, "set-sd-dacl/gle", (intmax_t)gle);
+		goto fail;
+	}
+
+	d->lpSA = (LPSECURITY_ATTRIBUTES)LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));
+	d->lpSA->nLength = sizeof(SECURITY_ATTRIBUTES);
+	d->lpSA->lpSecurityDescriptor = d->pSD;
+	d->lpSA->bInheritHandle = FALSE;
+
+	return d->lpSA;
+
+fail:
+	release_sa(d);
+	return NULL;
+}
+
 static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
 {
 	HANDLE hPipe;
 	DWORD dwOpenMode, dwPipeMode;
-	LPSECURITY_ATTRIBUTES lpsa = NULL;
+	struct my_sa_data my_sa_data;
+
+	init_sa(&my_sa_data);
 
 	dwOpenMode = PIPE_ACCESS_INBOUND | PIPE_ACCESS_OUTBOUND |
 		FILE_FLAG_OVERLAPPED;
@@ -612,20 +735,15 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first)
 		 * set the ACL / Security Attributes on the named
 		 * pipe; subsequent instances inherit and cannot
 		 * change them.
-		 *
-		 * TODO Should we allow the application layer to
-		 * specify security attributes, such as `LocalService`
-		 * or `LocalSystem`, when we create the named pipe?
-		 * This question is probably not important when the
-		 * daemon is started by a foreground user process and
-		 * only needs to talk to the current user, but may be
-		 * if the daemon is run via the Control Panel as a
-		 * System Service.
 		 */
+		get_sa(&my_sa_data);
 	}
 
 	hPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,
-				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);
+				 PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,
+				 my_sa_data.lpSA);
+
+	release_sa(&my_sa_data);
 
 	return hPipe;
 }
-- 
gitgitgadget

[PATCH v2 6/7] run-command: create start_bg_command

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

Create a variation of `run_command()` and `start_command()` to launch a command
into the background and optionally wait for it to become "ready" before returning.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 run-command.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++
 run-command.h |  57 ++++++++++++++++++++++
 2 files changed, 186 insertions(+)

diff --git a/run-command.c b/run-command.c
index 3e4e082e94d..76bbef9d96d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1901,3 +1901,132 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
 	}
 	strvec_pushf(env_array, "%s=%s", GIT_DIR_ENVIRONMENT, new_git_dir);
 }
+
+enum start_bg_result start_bg_command(struct child_process *cmd,
+				      start_bg_wait_cb *wait_cb,
+				      void *cb_data,
+				      unsigned int timeout_sec)
+{
+	enum start_bg_result sbgr = SBGR_ERROR;
+	int ret;
+	int wait_status;
+	pid_t pid_seen;
+	time_t time_limit;
+
+	/*
+	 * 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.
+	 */
+	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) {
+		/*
+		 * We assume that if `start_command()` fails, we
+		 * either get a complete `trace2_child_start() /
+		 * trace2_child_exit()` pair or it fails before the
+		 * `trace2_child_start()` is emitted, so we do not
+		 * need to worry about it here.
+		 *
+		 * We also assume that `start_command()` does not add
+		 * us to the cleanup list.  And that it calls
+		 * calls `child_process_clear()`.
+		 */
+		sbgr = SBGR_ERROR;
+		goto done;
+	}
+
+	time(&time_limit);
+	time_limit += timeout_sec;
+
+wait:
+	pid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);
+
+	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)(cmd, cb_data);
+		if (!ret) {
+			/*
+			 * The child is running and "ready".
+			 */
+			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);
+			if (now < time_limit)
+				goto wait;
+
+			/*
+			 * Our timeout has expired.  We don't try to
+			 * kill the child, but rather let it continue
+			 * (hopefully) trying to startup.
+			 */
+			trace2_child_ready(cmd, "timeout");
+			sbgr = SBGR_TIMEOUT;
+			goto done;
+		} else {
+			/*
+			 * 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;
+		}
+	}
+
+	else if (pid_seen == cmd->pid) {
+		int child_code = -1;
+
+		/*
+		 * The child started, but exited or was terminated
+		 * 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);
+		else if (WIFSIGNALED(wait_status))
+			child_code = WTERMSIG(wait_status) + 128;
+		trace2_child_exit(cmd, child_code);
+
+		sbgr = SBGR_DIED;
+		goto done;
+	}
+
+	else if (pid_seen < 0 && errno == EINTR)
+		goto wait;
+
+	trace2_child_exit(cmd, -1);
+	sbgr = SBGR_ERROR;
+
+done:
+	child_process_clear(cmd);
+	invalidate_lstat_cache();
+	return sbgr;
+}
diff --git a/run-command.h b/run-command.h
index af1296769f9..17b1b80c3d7 100644
--- a/run-command.h
+++ b/run-command.h
@@ -496,4 +496,61 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
  */
 void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);
 
+/**
+ * Possible return values for start_bg_command().
+ */
+enum start_bg_result {
+	/* child process is "ready" */
+	SBGR_READY = 0,
+
+	/* child process could not be started */
+	SBGR_ERROR,
+
+	/* callback error when testing for "ready" */
+	SBGR_CB_ERROR,
+
+	/* timeout expired waiting for child to become "ready" */
+	SBGR_TIMEOUT,
+
+	/* child process exited or was signalled before becomming "ready" */
+	SBGR_DIED,
+};
+
+/**
+ * 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.
+ */
+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" (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.
+ *
+ * The caller must not 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,
+				      void *cb_data,
+				      unsigned int timeout_sec);
+
 #endif
-- 
gitgitgadget

[PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Jeff Hostetler via GitGitGadget]

From: Jeff Hostetler <jeffhost@microsoft.com>

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 | 199 ++++++++-----------------------------
 1 file changed, 43 insertions(+), 156 deletions(-)

diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 91345180750..28365ff85b6 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -9,6 +9,7 @@
 #include "parse-options.h"
 #include "thread-utils.h"
 #include "strvec.h"
+#include "run-command.h"
 
 #ifndef SUPPORTS_SIMPLE_IPC
 int cmd__simple_ipc(int argc, const char **argv)
@@ -267,185 +268,71 @@ 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;
 }
 
-#ifndef GIT_WINDOWS_NATIVE
-/*
- * This is adapted from `daemonize()`.  Use `fork()` to directly create and
- * run the daemon in a child process.
- */
-static int spawn_server(pid_t *pid)
-{
-	struct ipc_server_opts opts = {
-		.nr_threads = cl_args.nr_threads,
-	};
+static start_bg_wait_cb bg_wait_cb;
 
-	*pid = fork();
-
-	switch (*pid) {
-	case 0:
-		if (setsid() == -1)
-			error_errno(_("setsid failed"));
-		close(0);
-		close(1);
-		close(2);
-		sanitize_stdfds();
+static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+{
+	int s = ipc_get_active_state(cl_args.path);
 
-		return ipc_server_run(cl_args.path, &opts, test_app_cb,
-				      (void*)&my_app_data);
+	switch (s) {
+	case IPC_STATE__LISTENING:
+		/* child is "ready" */
+		return 0;
 
-	case -1:
-		return error_errno(_("could not spawn daemon in the background"));
+	case IPC_STATE__NOT_LISTENING:
+	case IPC_STATE__PATH_NOT_FOUND:
+		/* give child more time */
+		return 1;
 
 	default:
-		return 0;
+	case IPC_STATE__INVALID_PATH:
+	case IPC_STATE__OTHER_ERROR:
+		/* all the time in world won't help */
+		return -1;
 	}
 }
-#else
-/*
- * Conceptually like `daemonize()` but different because Windows does not
- * have `fork(2)`.  Spawn a normal Windows child process but without the
- * limitations of `start_command()` and `finish_command()`.
- */
-static int spawn_server(pid_t *pid)
-{
-	char test_tool_exe[MAX_PATH];
-	struct strvec args = STRVEC_INIT;
-	int in, out;
-
-	GetModuleFileNameA(NULL, test_tool_exe, MAX_PATH);
-
-	in = open("/dev/null", O_RDONLY);
-	out = open("/dev/null", O_WRONLY);
-
-	strvec_push(&args, test_tool_exe);
-	strvec_push(&args, "simple-ipc");
-	strvec_push(&args, "run-daemon");
-	strvec_pushf(&args, "--name=%s", cl_args.path);
-	strvec_pushf(&args, "--threads=%d", cl_args.nr_threads);
-
-	*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);
-	close(in);
-	close(out);
-
-	strvec_clear(&args);
 
-	if (*pid < 0)
-		return error(_("could not spawn daemon in the background"));
-
-	return 0;
-}
-#endif
-
-/*
- * This is adapted from `wait_or_whine()`.  Watch the child process and
- * let it get started and begin listening for requests on the socket
- * before reporting our success.
- */
-static int wait_for_server_startup(pid_t pid_child)
+static int daemon__start_server(void)
 {
-	int status;
-	pid_t pid_seen;
-	enum ipc_active_state s;
-	time_t time_limit, now;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	enum start_bg_result sbgr;
 
-	time(&time_limit);
-	time_limit += cl_args.max_wait_sec;
+	strvec_push(&cp.args, "test-tool");
+	strvec_push(&cp.args, "simple-ipc");
+	strvec_push(&cp.args, "run-daemon");
+	strvec_pushf(&cp.args, "--name=%s", cl_args.path);
+	strvec_pushf(&cp.args, "--threads=%d", cl_args.nr_threads);
 
-	for (;;) {
-		pid_seen = waitpid(pid_child, &status, WNOHANG);
+	cp.no_stdin = 1;
+	cp.no_stdout = 1;
+	cp.no_stderr = 1;
 
-		if (pid_seen == -1)
-			return error_errno(_("waitpid failed"));
+	sbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec);
 
-		else if (pid_seen == 0) {
-			/*
-			 * The child is still running (this should be
-			 * the normal case).  Try to connect to it on
-			 * the socket and see if it is ready for
-			 * business.
-			 *
-			 * If there is another daemon already running,
-			 * our child will fail to start (possibly
-			 * after a timeout on the lock), but we don't
-			 * care (who responds) if the socket is live.
-			 */
-			s = ipc_get_active_state(cl_args.path);
-			if (s == IPC_STATE__LISTENING)
-				return 0;
-
-			time(&now);
-			if (now > time_limit)
-				return error(_("daemon not online yet"));
-
-			continue;
-		}
+	switch (sbgr) {
+	case SBGR_READY:
+		return 0;
 
-		else if (pid_seen == pid_child) {
-			/*
-			 * The new child daemon process shutdown while
-			 * it was starting up, so it is not listening
-			 * on the socket.
-			 *
-			 * Try to ping the socket in the odd chance
-			 * that another daemon started (or was already
-			 * running) while our child was starting.
-			 *
-			 * Again, we don't care who services the socket.
-			 */
-			s = ipc_get_active_state(cl_args.path);
-			if (s == IPC_STATE__LISTENING)
-				return 0;
+	default:
+	case SBGR_ERROR:
+	case SBGR_CB_ERROR:
+		return error("daemon failed to start");
 
-			/*
-			 * We don't care about the WEXITSTATUS() nor
-			 * any of the WIF*(status) values because
-			 * `cmd__simple_ipc()` does the `!!result`
-			 * trick on all function return values.
-			 *
-			 * So it is sufficient to just report the
-			 * early shutdown as an error.
-			 */
-			return error(_("daemon failed to start"));
-		}
+	case SBGR_TIMEOUT:
+		return error("daemon not online yet");
 
-		else
-			return error(_("waitpid is confused"));
+	case SBGR_DIED:
+		return error("daemon terminated");
 	}
 }
 
-/*
- * This process will start a simple-ipc server in a background process and
- * wait for it to become ready.  This is like `daemonize()` but gives us
- * more control and better error reporting (and makes it easier to write
- * unit tests).
- */
-static int daemon__start_server(void)
-{
-	pid_t pid_child;
-	int ret;
-
-	/*
-	 * Run the actual daemon in a background process.
-	 */
-	ret = spawn_server(&pid_child);
-	if (pid_child <= 0)
-		return ret;
-
-	/*
-	 * Let the parent wait for the child process to get started
-	 * and begin listening for requests on the socket.
-	 */
-	ret = wait_for_server_startup(pid_child);
-
-	return ret;
-}
-
 /*
  * This process will run a quick probe to see if a simple-ipc server
  * is active on this path.
@@ -548,7 +435,7 @@ 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");
 	}
 }
 
-- 
gitgitgadget

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ævar Arnfjörð Bjarmason]

On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:

> +	switch (sbgr) {
> +	case SBGR_READY:
> +		return 0;
>  
> -		else if (pid_seen == pid_child) {
> -			/*
> -			 * The new child daemon process shutdown while
> -			 * it was starting up, so it is not listening
> -			 * on the socket.
> -			 *
> -			 * Try to ping the socket in the odd chance
> -			 * that another daemon started (or was already
> -			 * running) while our child was starting.
> -			 *
> -			 * Again, we don't care who services the socket.
> -			 */
> -			s = ipc_get_active_state(cl_args.path);
> -			if (s == IPC_STATE__LISTENING)
> -				return 0;
> +	default:
> +	case SBGR_ERROR:
> +	case SBGR_CB_ERROR:
> +		return error("daemon failed to start");

There was a discussion on v1 about the "default" being redundant here
and hiding future compiler checks, this is another "not sure what you
thought of that" case (per [1]).

Interestingly in this case if I drop the "default" my local gcc
uncharacteristically complains about a missing "return" in this
function, but clang doesn't. I needed to add a BUG() to shut up the
former. Maybe I'm wrong, but perhaps it's a sign of some deeper
trouble. This is with gcc/clang 10.2.1-6/11.0.1-2.

1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/

I played with the diff below on top of this, I can't remember if it was
noted already, but the way you declare function ptrs and use them isn't
the usual style:

-- >8 --
diff --git a/run-command.c b/run-command.c
index 76bbef9d96d..5c831545201 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
 }
 
 enum start_bg_result start_bg_command(struct child_process *cmd,
-				      start_bg_wait_cb *wait_cb,
+				      start_bg_wait_cb wait_cb,
 				      void *cb_data,
 				      unsigned int timeout_sec)
 {
diff --git a/run-command.h b/run-command.h
index 17b1b80c3d7..e8a637d1967 100644
--- a/run-command.h
+++ b/run-command.h
@@ -527,7 +527,7 @@ enum start_bg_result {
  * 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)(const struct child_process *cmd, void *cb_data);
+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
@@ -549,7 +549,7 @@ typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
  * 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,
+				      start_bg_wait_cb wait_cb,
 				      void *cb_data,
 				      unsigned int timeout_sec);
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 28365ff85b6..82dc2906a2a 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -275,9 +275,7 @@ static int daemon__run_server(void)
 	return ret;
 }
 
-static start_bg_wait_cb bg_wait_cb;
-
-static int bg_wait_cb(const struct child_process *cp, void *cb_data)
+static int bg_wait_cb(const struct child_process *cp, void *cb_data, int foo)
 {
 	int s = ipc_get_active_state(cl_args.path);
 
@@ -319,9 +317,8 @@ static int daemon__start_server(void)
 	switch (sbgr) {
 	case SBGR_READY:
 		return 0;
-
-	default:
 	case SBGR_ERROR:
+		return 0;
 	case SBGR_CB_ERROR:
 		return error("daemon failed to start");
 
@@ -331,6 +328,7 @@ static int daemon__start_server(void)
 	case SBGR_DIED:
 		return error("daemon terminated");
 	}
+	BUG("unreachable");
 }
 
 /*

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Jeff Hostetler]

On 9/23/21 11:03 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:
> 
>> +	switch (sbgr) {
>> +	case SBGR_READY:
>> +		return 0;
>>   
>> -		else if (pid_seen == pid_child) {
>> -			/*
>> -			 * The new child daemon process shutdown while
>> -			 * it was starting up, so it is not listening
>> -			 * on the socket.
>> -			 *
>> -			 * Try to ping the socket in the odd chance
>> -			 * that another daemon started (or was already
>> -			 * running) while our child was starting.
>> -			 *
>> -			 * Again, we don't care who services the socket.
>> -			 */
>> -			s = ipc_get_active_state(cl_args.path);
>> -			if (s == IPC_STATE__LISTENING)
>> -				return 0;
>> +	default:
>> +	case SBGR_ERROR:
>> +	case SBGR_CB_ERROR:
>> +		return error("daemon failed to start");
> 
> There was a discussion on v1 about the "default" being redundant here
> and hiding future compiler checks, this is another "not sure what you
> thought of that" case (per [1]).
> 
> Interestingly in this case if I drop the "default" my local gcc
> uncharacteristically complains about a missing "return" in this
> function, but clang doesn't. I needed to add a BUG() to shut up the
> former. Maybe I'm wrong, but perhaps it's a sign of some deeper
> trouble. This is with gcc/clang 10.2.1-6/11.0.1-2.

The issue of whether C needs a "default" case in switch statements
on an enum is one I didn't have bandwidth to think about (and is
completely independent of my series).

I was thinking that as a later task, someone could investigate which
compilers do and do not generate errors for missing enum values in
the switch.  Perhaps that leads to a macro in config.mak.uname on
a system-by-system basis that "does the right thing".

Then one could have something like:

     switch (e) {
     DEFAULT_HANDLER;
     case e1: ...
     case e2: ...
     }


> 
> 1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/
> 
> I played with the diff below on top of this, I can't remember if it was
> noted already, but the way you declare function ptrs and use them isn't
> the usual style:
> 
> -- >8 --
> diff --git a/run-command.c b/run-command.c
> index 76bbef9d96d..5c831545201 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>   }
>   
>   enum start_bg_result start_bg_command(struct child_process *cmd,
> -				      start_bg_wait_cb *wait_cb,
> +				      start_bg_wait_cb wait_cb,
>   				      void *cb_data,
>   				      unsigned int timeout_sec)
>   {
> diff --git a/run-command.h b/run-command.h
> index 17b1b80c3d7..e8a637d1967 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -527,7 +527,7 @@ enum start_bg_result {
>    * 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)(const struct child_process *cmd, void *cb_data);
> +typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);

By defining the typedef WITHOUT the star, we get a function type.

We can then use it for forward function declarations.

But additionally, when declare a function that takes a function
pointer or when we define a vtable of function pointers, they look
like pointers.

start_bg_wait_cb *pfn = my_cb;

int foo(struct child_process *cmd, start_bg_wait_cb *cb);

Or if we look a the target vtable in Trace2.  This looks like
a structure of (function) pointers.

struct tr2_tgt {
	tr2_tgt_init_t                          *pfn_init;
	tr2_tgt_term_t                          *pfn_term;
	...
};

So I prefer to leave the star out of function typedef and then
we can use the typedef in both contexts.

Jeff

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I played with the diff below on top of this, I can't remember if it was
> noted already, but the way you declare function ptrs and use them isn't
> the usual style:
>
> -- >8 --
> diff --git a/run-command.c b/run-command.c
> index 76bbef9d96d..5c831545201 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)
>  }
>  
>  enum start_bg_result start_bg_command(struct child_process *cmd,
> -				      start_bg_wait_cb *wait_cb,
> +				      start_bg_wait_cb wait_cb,
> -typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);
> +typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);

I have no comment on the "default" thing, but I agree that the
preimage does look _unusual_ in our codebase.  You cannot even
declare a "variable" of that type with the typedef, i.e.

	start_bg_wait_cb an_instance_of_that;

If there is a good reason behind choosing the unusual "the type of
the function is...", that is OK, but otherwise...

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Adam Dinwoodie]

On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:
> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> 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.

As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've
found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert
test-simple-ipc to use start_bg_command, 2021-09-20), according to my
bisecting -- is causing t0052.1 to fail on 32-bit Cygwin.  Somehow this
is only affecting the 32-bit Cygwin build; the 64-bit build is working
as expected.

Specifically, the failure I'm seeing is as below:

```
$ GIT_TRACE=1 sh t0052-simple-ipc.sh -vix
trace: built-in: git init '/cygdrive/d/git-32/t/trash directory.t0052-simple-ipc'
Initialized empty Git repository in /cygdrive/d/git-32/t/trash directory.t0052-simple-ipc/.git/
expecting success of 0052.1 'start simple command server': 
	test_atexit stop_simple_IPC_server &&
	test-tool simple-ipc start-daemon --threads=8 &&
	test-tool simple-ipc is-active

++ test_atexit stop_simple_IPC_server
++ test 0 = 0
++ test_atexit_cleanup='{ stop_simple_IPC_server
		} && (exit "$eval_ret"); eval_ret=$?; :'
++ test-tool simple-ipc start-daemon --threads=8
trace: run_command: test-tool simple-ipc run-daemon --name=ipc-test --threads=8
error: daemon failed to start
error: last command exited with $?=1
not ok 1 - start simple command server
#	
#		test_atexit stop_simple_IPC_server &&
#		test-tool simple-ipc start-daemon --threads=8 &&
#		test-tool simple-ipc is-active
#	
++ stop_simple_IPC_server
++ test-tool simple-ipc stop-daemon
++ exit 1
++ eval_ret=1
++ :
```

I've had a look at the code changes, and cannot work out what might be
being handled differently in 32-bit and 64-bit Cygwin environments.
Given the Cygwin project is considering dropping support for 32-bit
Cygwin anyway, it might not be worth doing anything about this.  But I
thought it worth reporting in case there's something obvious to folk
more familiar with this code.

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ramsay Jones]

Hi Adam,

On 04/11/2021 19:46, Adam Dinwoodie wrote:
> On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> 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.
> 
> As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've
> found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert
> test-simple-ipc to use start_bg_command, 2021-09-20), according to my
> bisecting -- is causing t0052.1 to fail on 32-bit Cygwin.  Somehow this
> is only affecting the 32-bit Cygwin build; the 64-bit build is working
> as expected.

Hmmm, I am seeing exactly the same, but on 64-bit cygwin!

I haven't found time to look at this in detail yet (except for
what you have already done). Unfortunately, about an hour ago,
I did a 'make test' for the '-rc1' build, so I won't be able to
take a look for hours yet, ... :(

ATB,
Ramsay Jones

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Jeff Hostetler]

On 11/4/21 3:46 PM, Adam Dinwoodie wrote:
> On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> 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.
> 
> As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've
> found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert
> test-simple-ipc to use start_bg_command, 2021-09-20), according to my
> bisecting -- is causing t0052.1 to fail on 32-bit Cygwin.  Somehow this
> is only affecting the 32-bit Cygwin build; the 64-bit build is working
> as expected.
> 
> Specifically, the failure I'm seeing is as below:
> 
> ```
> $ GIT_TRACE=1 sh t0052-simple-ipc.sh -vix
> trace: built-in: git init '/cygdrive/d/git-32/t/trash directory.t0052-simple-ipc'
> Initialized empty Git repository in /cygdrive/d/git-32/t/trash directory.t0052-simple-ipc/.git/
> expecting success of 0052.1 'start simple command server':
> 	test_atexit stop_simple_IPC_server &&
> 	test-tool simple-ipc start-daemon --threads=8 &&
> 	test-tool simple-ipc is-active
> 
> ++ test_atexit stop_simple_IPC_server
> ++ test 0 = 0
> ++ test_atexit_cleanup='{ stop_simple_IPC_server
> 		} && (exit "$eval_ret"); eval_ret=$?; :'
> ++ test-tool simple-ipc start-daemon --threads=8
> trace: run_command: test-tool simple-ipc run-daemon --name=ipc-test --threads=8
> error: daemon failed to start
> error: last command exited with $?=1
> not ok 1 - start simple command server
> #	
> #		test_atexit stop_simple_IPC_server &&
> #		test-tool simple-ipc start-daemon --threads=8 &&
> #		test-tool simple-ipc is-active
> #	
> ++ stop_simple_IPC_server
> ++ test-tool simple-ipc stop-daemon
> ++ exit 1
> ++ eval_ret=1
> ++ :
> ```
> 
> I've had a look at the code changes, and cannot work out what might be
> being handled differently in 32-bit and 64-bit Cygwin environments.
> Given the Cygwin project is considering dropping support for 32-bit
> Cygwin anyway, it might not be worth doing anything about this.  But I
> thought it worth reporting in case there's something obvious to folk
> more familiar with this code.
> 

How odd!  Thanks for the report.  I'll investigate.
Jeff

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Johannes Schindelin]

Hi Adam,

On Thu, 4 Nov 2021, Adam Dinwoodie wrote:

> On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:
> > From: Jeff Hostetler <jeffhost@microsoft.com>
> >
> > 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.
>
> As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've
> found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert
> test-simple-ipc to use start_bg_command, 2021-09-20), according to my
> bisecting -- is causing t0052.1 to fail on 32-bit Cygwin.  Somehow this
> is only affecting the 32-bit Cygwin build; the 64-bit build is working
> as expected.
>
> Specifically, the failure I'm seeing is as below:
>
> ```
> $ GIT_TRACE=1 sh t0052-simple-ipc.sh -vix
> trace: built-in: git init '/cygdrive/d/git-32/t/trash directory.t0052-simple-ipc'
> Initialized empty Git repository in /cygdrive/d/git-32/t/trash directory.t0052-simple-ipc/.git/
> expecting success of 0052.1 'start simple command server':
> 	test_atexit stop_simple_IPC_server &&
> 	test-tool simple-ipc start-daemon --threads=8 &&
> 	test-tool simple-ipc is-active
>
> ++ test_atexit stop_simple_IPC_server
> ++ test 0 = 0
> ++ test_atexit_cleanup='{ stop_simple_IPC_server
> 		} && (exit "$eval_ret"); eval_ret=$?; :'
> ++ test-tool simple-ipc start-daemon --threads=8
> trace: run_command: test-tool simple-ipc run-daemon --name=ipc-test --threads=8
> error: daemon failed to start
> error: last command exited with $?=1
> not ok 1 - start simple command server
> #
> #		test_atexit stop_simple_IPC_server &&
> #		test-tool simple-ipc start-daemon --threads=8 &&
> #		test-tool simple-ipc is-active
> #
> ++ stop_simple_IPC_server
> ++ test-tool simple-ipc stop-daemon
> ++ exit 1
> ++ eval_ret=1
> ++ :
> ```
>
> I've had a look at the code changes, and cannot work out what might be
> being handled differently in 32-bit and 64-bit Cygwin environments.
> Given the Cygwin project is considering dropping support for 32-bit
> Cygwin anyway, it might not be worth doing anything about this.  But I
> thought it worth reporting in case there's something obvious to folk
> more familiar with this code.

I had a look at this and could reproduce... partially. I only managed to
make it fail every once in a while.

Digging deeper, it turns out that the `lstat()` call in
`ipc_get_active_state()` does not receive an `st_mode` indicating a
socket, but rather a file (in my tests, it was usually 0100644, but
sometimes even 0100755).

The reason is, of course, that Cygwin _emulates_ Unix sockets. What is in
the file system is just a special file, it is marked with the `system` bit
(which only exists on Windows), and its contents start with the tell-tale
`!<socket>`.

And as you might have guessed, there is a race going on between Cygwin
writing that file _and_ flipping that `system` bit, and Git trying to
access the Unix socket and encountering an unexpected file.

Now, why this only happens in your 32-bit setup, I have no idea.

In my tests, the following patch works around the issue. Could I ask you
to test it in your environment?

-- snip --
diff --git a/compat/simple-ipc/ipc-unix-socket.c
b/compat/simple-ipc/ipc-unix-socket.c
index 4e28857a0a..1c591b2adf 100644
--- a/compat/simple-ipc/ipc-unix-socket.c
+++ b/compat/simple-ipc/ipc-unix-socket.c
@@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char
*path)
 	}

 	/* also complain if a plain file is in the way */
+#ifdef __CYGWIN__
+	{
+		static const int delay[] = { 1, 10, 20, 40, -1 };
+		int i;
+
+		for (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {
+			/*
+			 * Cygwin might still be in the process of marking the
+			 * underlying file as a system file.
+			 */
+			sleep_millisec(delay[i]);
+			if (lstat(path, &st) == -1)
+				return IPC_STATE__INVALID_PATH;
+		}
+	}
+#endif
+
 	if ((st.st_mode & S_IFMT) != S_IFSOCK)
 		return IPC_STATE__INVALID_PATH;

-- snap --

FWIW it looks as if the loop might be a bit of an overkill, as I could not
get the code to need more than a single one-millisecond sleep, but it's
probably safer to just keep the delay loop in place as-is.

Ciao,
Dscho

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ramsay Jones]

On 08/11/2021 23:59, Johannes Schindelin wrote:
[snip]
> I had a look at this and could reproduce... partially. I only managed to
> make it fail every once in a while.
> 
> Digging deeper, it turns out that the `lstat()` call in
> `ipc_get_active_state()` does not receive an `st_mode` indicating a
> socket, but rather a file (in my tests, it was usually 0100644, but
> sometimes even 0100755).
> 
> The reason is, of course, that Cygwin _emulates_ Unix sockets. What is in
> the file system is just a special file, it is marked with the `system` bit
> (which only exists on Windows), and its contents start with the tell-tale
> `!<socket>`.
> 
> And as you might have guessed, there is a race going on between Cygwin
> writing that file _and_ flipping that `system` bit, and Git trying to
> access the Unix socket and encountering an unexpected file.
> 
> Now, why this only happens in your 32-bit setup, I have no idea.
> 
> In my tests, the following patch works around the issue. Could I ask you
> to test it in your environment?

Just FYI, I just tried the patch below (on 64-bit cygwin) and this test
now works fine for me. (well, run 5 times by hand - not with --stress).

This is on windows 10 21H1 and cygwin:

  $ uname -a
  CYGWIN_NT-10.0 satellite 3.3.2(0.341/5/3) 2021-11-08 16:55 x86_64 Cygwin
  $ 

[Yes, I updated last night!]

ATB,
Ramsay Jones

> -- snip --
> diff --git a/compat/simple-ipc/ipc-unix-socket.c
> b/compat/simple-ipc/ipc-unix-socket.c
> index 4e28857a0a..1c591b2adf 100644
> --- a/compat/simple-ipc/ipc-unix-socket.c
> +++ b/compat/simple-ipc/ipc-unix-socket.c
> @@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char
> *path)
>  	}
> 
>  	/* also complain if a plain file is in the way */
> +#ifdef __CYGWIN__
> +	{
> +		static const int delay[] = { 1, 10, 20, 40, -1 };
> +		int i;
> +
> +		for (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {
> +			/*
> +			 * Cygwin might still be in the process of marking the
> +			 * underlying file as a system file.
> +			 */
> +			sleep_millisec(delay[i]);
> +			if (lstat(path, &st) == -1)
> +				return IPC_STATE__INVALID_PATH;
> +		}
> +	}
> +#endif
> +
>  	if ((st.st_mode & S_IFMT) != S_IFSOCK)
>  		return IPC_STATE__INVALID_PATH;
> 
> -- snap --
> 
> FWIW it looks as if the loop might be a bit of an overkill, as I could not
> get the code to need more than a single one-millisecond sleep, but it's
> probably safer to just keep the delay loop in place as-is.
> 
> Ciao,
> Dscho
> 

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Johannes Schindelin]

Hi Ramsay,

On Tue, 9 Nov 2021, Ramsay Jones wrote:

> On 08/11/2021 23:59, Johannes Schindelin wrote:
> [snip]
> > I had a look at this and could reproduce... partially. I only managed to
> > make it fail every once in a while.
> >
> > Digging deeper, it turns out that the `lstat()` call in
> > `ipc_get_active_state()` does not receive an `st_mode` indicating a
> > socket, but rather a file (in my tests, it was usually 0100644, but
> > sometimes even 0100755).
> >
> > The reason is, of course, that Cygwin _emulates_ Unix sockets. What is in
> > the file system is just a special file, it is marked with the `system` bit
> > (which only exists on Windows), and its contents start with the tell-tale
> > `!<socket>`.
> >
> > And as you might have guessed, there is a race going on between Cygwin
> > writing that file _and_ flipping that `system` bit, and Git trying to
> > access the Unix socket and encountering an unexpected file.
> >
> > Now, why this only happens in your 32-bit setup, I have no idea.
> >
> > In my tests, the following patch works around the issue. Could I ask you
> > to test it in your environment?
>
> Just FYI, I just tried the patch below (on 64-bit cygwin) and this test
> now works fine for me. (well, run 5 times by hand - not with --stress).

Very good!

I fear that it is a bit late in the -rc cycle to try to get this into the
official v2.34.0. Adam, since you are the maintainer of the Cygwin git
package, would you mind incorporating this patch into Cygwin's version of
Git?

> This is on windows 10 21H1 and cygwin:
>
>   $ uname -a
>   CYGWIN_NT-10.0 satellite 3.3.2(0.341/5/3) 2021-11-08 16:55 x86_64 Cygwin
>   $
>
> [Yes, I updated last night!]

Good thing, too: v3.3.2 fixes a critical bug in the pipe code. One symptom
was that you could not use Git Credential Manager Core as credential
helper (because Git thought that the helper had hung up, well before the
helper sent any information).

Ciao,
Dscho

>
> ATB,
> Ramsay Jones
>
> > -- snip --
> > diff --git a/compat/simple-ipc/ipc-unix-socket.c
> > b/compat/simple-ipc/ipc-unix-socket.c
> > index 4e28857a0a..1c591b2adf 100644
> > --- a/compat/simple-ipc/ipc-unix-socket.c
> > +++ b/compat/simple-ipc/ipc-unix-socket.c
> > @@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char
> > *path)
> >  	}
> >
> >  	/* also complain if a plain file is in the way */
> > +#ifdef __CYGWIN__
> > +	{
> > +		static const int delay[] = { 1, 10, 20, 40, -1 };
> > +		int i;
> > +
> > +		for (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {
> > +			/*
> > +			 * Cygwin might still be in the process of marking the
> > +			 * underlying file as a system file.
> > +			 */
> > +			sleep_millisec(delay[i]);
> > +			if (lstat(path, &st) == -1)
> > +				return IPC_STATE__INVALID_PATH;
> > +		}
> > +	}
> > +#endif
> > +
> >  	if ((st.st_mode & S_IFMT) != S_IFSOCK)
> >  		return IPC_STATE__INVALID_PATH;
> >
> > -- snap --
> >
> > FWIW it looks as if the loop might be a bit of an overkill, as I could not
> > get the code to need more than a single one-millisecond sleep, but it's
> > probably safer to just keep the delay loop in place as-is.
> >
> > Ciao,
> > Dscho
> >
>
>

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I fear that it is a bit late in the -rc cycle to try to get this into the
> official v2.34.0. Adam, since you are the maintainer of the Cygwin git
> package, would you mind incorporating this patch into Cygwin's version of
> Git?

I do not mind taking a Cygwin-only #ifdef block in compat/ like we
see below from folks who have stake in Cygwin, and who are clearly
leading Cygwin users on the list, like Ramsay and Adam are, even
after I tag -rc2.

I cannot give the change any better test than they can, and it is
their platform to improve, or break by accident while trying to do
so.

Thanks.

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Johannes Schindelin]

Hi Junio,

On Tue, 9 Nov 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I fear that it is a bit late in the -rc cycle to try to get this into the
> > official v2.34.0. Adam, since you are the maintainer of the Cygwin git
> > package, would you mind incorporating this patch into Cygwin's version of
> > Git?
>
> I do not mind taking a Cygwin-only #ifdef block in compat/ like we
> see below from folks who have stake in Cygwin, and who are clearly
> leading Cygwin users on the list, like Ramsay and Adam are, even
> after I tag -rc2.

Thank you for your encouragement, I contributed it here:
https://lore.kernel.org/git/pull.1074.git.1636542550889.gitgitgadget@gmail.com

> I cannot give the change any better test than they can, and it is
> their platform to improve, or break by accident while trying to do
> so.

Right. I tested this as well as I could, via the `--stress` option, and am
fairly confident that it is correct. Since the patch touches only
`simply-ipc` code, the only test that could possibly affected is t0052,
and it passes with `--stress` over here (when it failed without the
patch).

Ciao,
Dscho

P.S.: in case you wondered, no, I did not run the entire test suite. With
the performance characteristics of the POSIX emulation provided by the
Cygwin runtime, this would simply take too long. It's not the first time I
wish our test suite was more efficient, across _all_ supported platforms.

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Adam Dinwoodie]

On Wed, 10 Nov 2021 at 12:28, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Junio,
>
> On Tue, 9 Nov 2021, Junio C Hamano wrote:
>
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >
> > > I fear that it is a bit late in the -rc cycle to try to get this into the
> > > official v2.34.0. Adam, since you are the maintainer of the Cygwin git
> > > package, would you mind incorporating this patch into Cygwin's version of
> > > Git?
> >
> > I do not mind taking a Cygwin-only #ifdef block in compat/ like we
> > see below from folks who have stake in Cygwin, and who are clearly
> > leading Cygwin users on the list, like Ramsay and Adam are, even
> > after I tag -rc2.
>
> Thank you for your encouragement, I contributed it here:
> https://lore.kernel.org/git/pull.1074.git.1636542550889.gitgitgadget@gmail.com
>
> > I cannot give the change any better test than they can, and it is
> > their platform to improve, or break by accident while trying to do
> > so.
>
> Right. I tested this as well as I could, via the `--stress` option, and am
> fairly confident that it is correct. Since the patch touches only
> `simply-ipc` code, the only test that could possibly affected is t0052,
> and it passes with `--stress` over here (when it failed without the
> patch).
>
> Ciao,
> Dscho
>
> P.S.: in case you wondered, no, I did not run the entire test suite. With
> the performance characteristics of the POSIX emulation provided by the
> Cygwin runtime, this would simply take too long. It's not the first time I
> wish our test suite was more efficient, across _all_ supported platforms.

I have just run the complete test suite on rc2, both with and without
this patch, and I can confirm it resolves this problem and doesn't
cause any other new test failures.

But yes, the (lack of) speed of running the Git test suite on Cygwin
is one of the reasons I run the tests on high-spec Azure VMs rather
than my own systems. Unfortunately the Cygwin compatibility layer plus
the overheads of NTFS mean things are unlikely to get significantly
quicker any time soon, and between WSL and Git for Windows, I expect
interest in improving Cygwin's performance is going to continue to
wane.

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

> I have just run the complete test suite on rc2, both with and without
> this patch, and I can confirm it resolves this problem and doesn't
> cause any other new test failures.

Thanks for a good news.

> But yes, the (lack of) speed of running the Git test suite on Cygwin
> is one of the reasons I run the tests on high-spec Azure VMs rather
> than my own systems. Unfortunately the Cygwin compatibility layer plus
> the overheads of NTFS mean things are unlikely to get significantly
> quicker any time soon, and between WSL and Git for Windows, I expect
> interest in improving Cygwin's performance is going to continue to
> wane.

Out of curiosity, are the use cases and user base of Cygwin waning,
or are there still viable cases where Cygwin is a more preferred
solution over WSL (the question is not limited to use of Git)?

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Adam Dinwoodie]

On Fri, 12 Nov 2021 at 16:01, Junio C Hamano <gitster@pobox.com> wrote:
>
> Adam Dinwoodie <adam@dinwoodie.org> writes:
> > But yes, the (lack of) speed of running the Git test suite on Cygwin
> > is one of the reasons I run the tests on high-spec Azure VMs rather
> > than my own systems. Unfortunately the Cygwin compatibility layer plus
> > the overheads of NTFS mean things are unlikely to get significantly
> > quicker any time soon, and between WSL and Git for Windows, I expect
> > interest in improving Cygwin's performance is going to continue to
> > wane.
>
> Out of curiosity, are the use cases and user base of Cygwin waning,
> or are there still viable cases where Cygwin is a more preferred
> solution over WSL (the question is not limited to use of Git)?

No formal research here, just impressions as someone who has used
Cygwin for a long time and who hangs out on the Cygwin mailing list:
for a lot of use cases, WSL is at least as good, if not better, than
Cygwin. There are a few areas where Cygwin is still a better solution,
though:

- WSL requires essentially installing an entire operating system. Disk
space is relatively cheap, so that's not nearly the obstacle it used
to be, but it is an obstacle. This is more relevant for people who
want to distribute packaged installers to Windows users: most
non-technical users won't want to get WSL working, but if you've
written code for *nix and don't want to port it manually to Windows,
it's relatively straightforward to compile it using Cygwin and bundle
the cygwin1.dll file with the installer. That'll mostly get your code
working with a user experience that doesn't differ too much from a
fully native Windows application. (This is essentially what Git for
Windows is doing, albeit with an increasingly distant Cygwin fork.)

- There are some functions that Cygwin offers that WSL doesn't. The
key one for me is the ability to access Windows network file shares,
which WSL doesn't support (or at least didn't last time I checked). I
expect some of these gaps will disappear as WSL gets more features,
but I expect some of them are fairly fundamental restrictions: Cygwin
applications can have code specifically to handle the fact that
there's a Windows OS there, so they can -- with care -- interact with
the Windows OS directly to (say) use Windows file access APIs or the
Windows clipboard. WSL applications generally don't have that ability;
if I install something from apt on my Debian WSL installation, it'll
pull exactly the same binary as if I'd installed it on a normal Debian
system. I guess in theory people could write code to detect that
they're running in WSL and handle that specially, in the same way that
it's normally possible to detect and handle when you're running in a
VM versus running on bare metal. I expect that'll be much less common,
though, just as Git has code for handling Cygwin specially but doesn't
have code for handling Linux-within-WSL specially, even though both
could be used to access a Git repository stored in the same Windows
NTFS directory.

I expect some folk who historically have used Cygwin will shift over
to WSL, some will stick with Cygwin, and a small number (as I do) will
use both in parallel for slightly different jobs.

tl;dr IMO both is true: WSL is better than Cygwin for some use cases
so the user base is waning, but Cygwin is still a very viable
preference over WSL for other use cases.

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Johannes Schindelin]

Hi,

On Fri, 12 Nov 2021, Adam Dinwoodie wrote:

> On Fri, 12 Nov 2021 at 16:01, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Out of curiosity, are the use cases and user base of Cygwin waning,
> > or are there still viable cases where Cygwin is a more preferred
> > solution over WSL (the question is not limited to use of Git)?
>
> No formal research here, just impressions as someone who has used
> Cygwin for a long time and who hangs out on the Cygwin mailing list:
> for a lot of use cases, WSL is at least as good, if not better, than
> Cygwin. There are a few areas where Cygwin is still a better solution,
> though:
>
> - WSL requires essentially installing an entire operating system. Disk
> space is relatively cheap, so that's not nearly the obstacle it used
> to be, but it is an obstacle. This is more relevant for people who
> want to distribute packaged installers to Windows users: most
> non-technical users won't want to get WSL working, but if you've
> written code for *nix and don't want to port it manually to Windows,
> it's relatively straightforward to compile it using Cygwin and bundle
> the cygwin1.dll file with the installer. That'll mostly get your code
> working with a user experience that doesn't differ too much from a
> fully native Windows application. (This is essentially what Git for
> Windows is doing, albeit with an increasingly distant Cygwin fork.)
>
> - There are some functions that Cygwin offers that WSL doesn't. The
> key one for me is the ability to access Windows network file shares,
> which WSL doesn't support (or at least didn't last time I checked). I
> expect some of these gaps will disappear as WSL gets more features,
> but I expect some of them are fairly fundamental restrictions: Cygwin
> applications can have code specifically to handle the fact that
> there's a Windows OS there, so they can -- with care -- interact with
> the Windows OS directly to (say) use Windows file access APIs or the
> Windows clipboard. WSL applications generally don't have that ability;
> if I install something from apt on my Debian WSL installation, it'll
> pull exactly the same binary as if I'd installed it on a normal Debian
> system. I guess in theory people could write code to detect that
> they're running in WSL and handle that specially, in the same way that
> it's normally possible to detect and handle when you're running in a
> VM versus running on bare metal. I expect that'll be much less common,
> though, just as Git has code for handling Cygwin specially but doesn't
> have code for handling Linux-within-WSL specially, even though both
> could be used to access a Git repository stored in the same Windows
> NTFS directory.

I would like to add two additional scenarios where Cygwin needs to be used
instead of WSL:

- In order to install WSL, you need to switch your machine to Developer
  Mode, which requires administrative privileges (which not evey developer
  enjoys, and given recent and not so recent security news, maybe that's a
  good thing, too). Cygwin does not require administrative privileges.

- There is (finally!) a way to run graphical Linux applications in WSL,
  but it requires Windows 11. That excludes many existing Windows users.

So yeah, I think Cygwin is here to stay. Besides, as long as I don't find
any better way to have a POSIX-compliant shell (Git will continue to
depend on one for a long, long time, I expect), Git for Windows will
indirectly _have_ to depend on Cygwin (Git for Windows bundles a Bash
using the MSYS2 runtime, which is a very close fork of the Cygwin
runtime).

Ciao,
Dscho

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Johannes Schindelin]

Hi Adam,

On Fri, 12 Nov 2021, Adam Dinwoodie wrote:

> [...] the (lack of) speed of running the Git test suite on Cygwin
> is one of the reasons I run the tests on high-spec Azure VMs rather
> than my own systems. Unfortunately the Cygwin compatibility layer plus
> the overheads of NTFS mean things are unlikely to get significantly
> quicker any time soon, and between WSL and Git for Windows, I expect
> interest in improving Cygwin's performance is going to continue to
> wane.

Well, at least from the Git point of view, there is still _some_ hope. At
the Git Contributor Summit, we talked (very, very briefly) about moving
parts of Git's test infrastructure from shell code to C.

This would definitely help not only save electricity (and we all _do_ have
to get used to the idea that we cannot continue spending as much energy as
we do right now) when running the CI/PR builds, but also accelerate
running the test suite on Cygwin (or for that matter, I suspect HP NonStop
to be helped tremendously, too).

Ciao,
Dscho

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ramsay Jones]

On 10/11/2021 12:27, Johannes Schindelin wrote:
[snip]
>> I cannot give the change any better test than they can, and it is
>> their platform to improve, or break by accident while trying to do
>> so.
> 
> Right. I tested this as well as I could, via the `--stress` option, and am
> fairly confident that it is correct. Since the patch touches only
> `simply-ipc` code, the only test that could possibly affected is t0052,
> and it passes with `--stress` over here (when it failed without the
> patch).
> 
> Ciao,
> Dscho
> 
> P.S.: in case you wondered, no, I did not run the entire test suite. With
> the performance characteristics of the POSIX emulation provided by the
> Cygwin runtime, this would simply take too long. It's not the first time I
> wish our test suite was more efficient, across _all_ supported platforms.


[I seem to have lost Adam's reply about this now being Hunky-Dory, but ...]

I ran the test-suite on -rc2 on thursday night (note _not_ rc2 + this patch)
and it deadlocked on me; I didn't notice for 4 hours, so (in the early hours)
I simply Ctl+C-ed it and went to bed. I haven't had the test-suite deadlock
for many many years - I've been spoilt! ;-)

I tried -rc2 again last night; this time it finished, but I gained another
test failure: t0301-credential-cache.sh. I have _never_ had this test fail
before, so that was unexpected. :(

[Yes, t0052-simple-ipc.sh failed as expected, since this patch was not
applied].

Also, I was half expecting a small speed-up due to the new pipe code in
v3.3.2 of the cygwin dll, but it actually took an hour longer than normal. :(

The only change to my setup, between -rc1 and -rc2, was the cygwin update
to v3.3.2, so this may point to some more fallout from the new pipe code
(maybe?).

Anyway, I haven't even looked at the new failure (see below), which we will
probably not have time to fix before release, so I am just now building
current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't
have anything to report until tomorrow).

Just FYI:

  $ ./t0301-credential-cache.sh
  ...
  not ok 13 - socket defaults to ~/.cache/git/credential/socket
  #
  #               test_when_finished "
  #                       git credential-cache exit &&
  #                       rmdir -p .cache/git/credential/
  #               " &&
  #               test_path_is_missing "$HOME/.git-credential-cache" &&
  #               test_path_is_socket "$HOME/.cache/git/credential/socket"
  #
  ...
  not ok 26 - use custom XDG_CACHE_HOME if set and default sockets are not created
  #
  #               test_when_finished "git credential-cache exit" &&
  #               test_path_is_socket "$XDG_CACHE_HOME/git/credential/socket" &&
  #               test_path_is_missing "$HOME/.git-credential-cache/socket" &&
  #               test_path_is_missing "$HOME/.cache/git/credential/socket"
  #
  not ok 27 - credential-cache --socket option overrides default location
  #
  #               test_when_finished "
  #                       git credential-cache exit --socket \"\$HOME/dir/socket\" &&
  #                       rmdir \"\$HOME/dir\"
  #               " &&
  #               check approve "cache --socket \"\$HOME/dir/socket\"" <<-\EOF &&
  #               protocol=https
  #               host=example.com
  #               username=store-user
  #               password=store-pass
  #               EOF
  #               test_path_is_socket "$HOME/dir/socket"
  #
  not ok 28 - use custom XDG_CACHE_HOME even if xdg socket exists
  #
  #               test_when_finished "
  #                       git credential-cache exit &&
  #                       sane_unset XDG_CACHE_HOME
  #               " &&
  #               check approve cache <<-\EOF &&
  #               protocol=https
  #               host=example.com
  #               username=store-user
  #               password=store-pass
  #               EOF
  #               test_path_is_socket "$HOME/.cache/git/credential/socket" &&
  #               XDG_CACHE_HOME="$HOME/xdg" &&
  #               export XDG_CACHE_HOME &&
  #               check approve cache <<-\EOF &&
  #               protocol=https
  #               host=example.com
  #               username=store-user
  #               password=store-pass
  #               EOF
  #               test_path_is_socket "$XDG_CACHE_HOME/git/credential/socket"
  #
  not ok 29 - use user socket if user directory exists
  #
  #               test_when_finished "
  #                       git credential-cache exit &&
  #                       rmdir \"\$HOME/.git-credential-cache/\"
  #               " &&
  #               mkdir -p "$HOME/.git-credential-cache/" &&
  #               chmod 700 "$HOME/.git-credential-cache/" &&
  #               check approve cache <<-\EOF &&
  #               protocol=https
  #               host=example.com
  #               username=store-user
  #               password=store-pass
  #               EOF
  #               test_path_is_socket "$HOME/.git-credential-cache/socket"
  #
  not ok 30 - use user socket if user directory is a symlink to a directory
  #
  #               test_when_finished "
  #                       git credential-cache exit &&
  #                       rmdir \"\$HOME/dir/\" &&
  #                       rm \"\$HOME/.git-credential-cache\"
  #               " &&
  #               mkdir -p -m 700 "$HOME/dir/" &&
  #               ln -s "$HOME/dir" "$HOME/.git-credential-cache" &&
  #               check approve cache <<-\EOF &&
  #               protocol=https
  #               host=example.com
  #               username=store-user
  #               password=store-pass
  #               EOF
  #               test_path_is_socket "$HOME/.git-credential-cache/socket"
  #
  ok 31 - helper (cache --timeout=1) times out
  # failed 6 among 31 test(s)
  1..31
  $
  
ATB,
Ramsay Jones

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Ramsay Jones]

On 13/11/2021 19:11, Ramsay Jones wrote:
[snip]
> The only change to my setup, between -rc1 and -rc2, was the cygwin update
> to v3.3.2, so this may point to some more fallout from the new pipe code
> (maybe?).
> 
> Anyway, I haven't even looked at the new failure (see below), which we will
> probably not have time to fix before release, so I am just now building
> current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't
> have anything to report until tomorrow).

So, current 'master' fixes t0052-simple-ipc.sh, which is good, but the
t0301-credential-cache.sh test is still failing. Also, I can confirm
that cygwin v3.3.2 adds an additional hour to a test-suite run. :(

[ie it now takes 6 hours rather than 5 hours to run - I remember a time
when it used to only take 2 hours; those were the days!]

ATB,
Ramsay Jones

Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command

[Adam Dinwoodie]

On Sat, 13 Nov 2021 at 19:11, Ramsay Jones <ramsay@ramsayjones.plus.com> wrote:
> On 10/11/2021 12:27, Johannes Schindelin wrote:
> [snip]
> >> I cannot give the change any better test than they can, and it is
> >> their platform to improve, or break by accident while trying to do
> >> so.
> >
> > Right. I tested this as well as I could, via the `--stress` option, and am
> > fairly confident that it is correct. Since the patch touches only
> > `simply-ipc` code, the only test that could possibly affected is t0052,
> > and it passes with `--stress` over here (when it failed without the
> > patch).
> >
> > Ciao,
> > Dscho
> >
> > P.S.: in case you wondered, no, I did not run the entire test suite. With
> > the performance characteristics of the POSIX emulation provided by the
> > Cygwin runtime, this would simply take too long. It's not the first time I
> > wish our test suite was more efficient, across _all_ supported platforms.
>
>
> [I seem to have lost Adam's reply about this now being Hunky-Dory, but ...]
>
> I ran the test-suite on -rc2 on thursday night (note _not_ rc2 + this patch)
> and it deadlocked on me; I didn't notice for 4 hours, so (in the early hours)
> I simply Ctl+C-ed it and went to bed. I haven't had the test-suite deadlock
> for many many years - I've been spoilt! ;-)
>
> I tried -rc2 again last night; this time it finished, but I gained another
> test failure: t0301-credential-cache.sh. I have _never_ had this test fail
> before, so that was unexpected. :(
>
> [Yes, t0052-simple-ipc.sh failed as expected, since this patch was not
> applied].
>
> Also, I was half expecting a small speed-up due to the new pipe code in
> v3.3.2 of the cygwin dll, but it actually took an hour longer than normal. :(
>
> The only change to my setup, between -rc1 and -rc2, was the cygwin update
> to v3.3.2, so this may point to some more fallout from the new pipe code
> (maybe?).
>
> Anyway, I haven't even looked at the new failure (see below), which we will
> probably not have time to fix before release, so I am just now building
> current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't
> have anything to report until tomorrow).

I'm seeing the same failure. It isn't caused by a change in Git --
I've rebuilt and re-run the test on old versions where that test was
passing, and it's now failing -- so this is clearly something in the
Cygwin environment. I've not investigated further, but it's clearly
caused by a Cygwin change rather than a Git change, so I don't think
there's any reason to hold up the Git release.

(I should probably report it on the Cygwin mailing list, but I haven't
got around to that yet...)

Re: [PATCH v2 0/7] Builtin FSMonitor Part 1

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH v2 0/7] Builtin FSMonitor Part 1

[Jeff Hostetler]

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

Re: [PATCH v2 0/7] Builtin FSMonitor Part 1

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH v2 0/7] Builtin FSMonitor Part 1

[Jeff Hostetler]

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

Re: [PATCH v2 0/7] Builtin FSMonitor Part 1

[Taylor Blau]

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