All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Jeff Hostetler via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff Hostetler <jeffhost@microsoft.com>
Subject: Re: [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging
Date: Thu, 16 Sep 2021 07:40:58 +0200	[thread overview]
Message-ID: <87bl4tm5s1.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <30b7bb247c3c8531119f99e293d84b1836297251.1631738177.git.gitgitgadget@gmail.com>


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

  reply	other threads:[~2021-09-16  5:46 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-15 20:36 [PATCH 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-15 20:36 ` [PATCH 1/7] trace2: fix memory leak of thread name Jeff Hostetler via GitGitGadget
2021-09-15 21:01   ` Junio C Hamano
2021-09-16  4:19     ` Taylor Blau
2021-09-16  5:35   ` Ævar Arnfjörð Bjarmason
2021-09-16  5:43     ` Taylor Blau
2021-09-16  8:01       ` Ævar Arnfjörð Bjarmason
2021-09-16 15:35         ` Jeff Hostetler
2021-09-16 15:47           ` Ævar Arnfjörð Bjarmason
2021-09-16 19:13         ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-15 20:43   ` Eric Sunshine
2021-09-17 16:52     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-15 21:06   ` Junio C Hamano
2021-09-17 16:58     ` Jeff Hostetler
2021-09-18  7:03       ` Carlo Arenas
2021-09-20 15:51       ` Junio C Hamano
2021-09-15 20:36 ` [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-16  5:40   ` Ævar Arnfjörð Bjarmason [this message]
2021-09-17 17:27     ` Jeff Hostetler
2021-09-15 20:36 ` [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-16  5:47   ` Ævar Arnfjörð Bjarmason
2021-09-17 18:10     ` Jeff Hostetler
2021-09-17 19:14       ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  4:53   ` Taylor Blau
2021-09-16  4:58     ` Taylor Blau
2021-09-16  5:56   ` Ævar Arnfjörð Bjarmason
2021-09-15 20:36 ` [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-16  5:06   ` Taylor Blau
2021-09-17 19:41     ` Jeff Hostetler
2021-09-18  8:59       ` Ævar Arnfjörð Bjarmason
2021-09-16  5:55   ` Ævar Arnfjörð Bjarmason
2021-09-20 15:36 ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 2/7] simple-ipc: preparations for supporting binary messages Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 6/7] run-command: create start_bg_command Jeff Hostetler via GitGitGadget
2021-09-20 15:36   ` [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command Jeff Hostetler via GitGitGadget
2021-09-23 15:03     ` Ævar Arnfjörð Bjarmason
2021-09-23 17:58       ` Jeff Hostetler
2021-09-23 18:37       ` Junio C Hamano
2021-11-04 19:46     ` Adam Dinwoodie
2021-11-04 20:14       ` Ramsay Jones
2021-11-08 14:58       ` Jeff Hostetler
2021-11-08 23:59       ` Johannes Schindelin
2021-11-09 18:53         ` Ramsay Jones
2021-11-09 23:01           ` Johannes Schindelin
2021-11-09 23:34             ` Junio C Hamano
2021-11-10 12:27               ` Johannes Schindelin
2021-11-12  8:56                 ` Adam Dinwoodie
2021-11-12 16:01                   ` Junio C Hamano
2021-11-12 21:33                     ` Adam Dinwoodie
2021-11-16 10:56                       ` Johannes Schindelin
2021-11-16 11:02                   ` Johannes Schindelin
2021-11-13 19:11                 ` Ramsay Jones
2021-11-14 19:34                   ` Ramsay Jones
2021-11-14 20:10                   ` Adam Dinwoodie
2021-09-23 14:33   ` [PATCH v2 0/7] Builtin FSMonitor Part 1 Ævar Arnfjörð Bjarmason
2021-09-23 17:12     ` Jeff Hostetler
2021-09-23 20:47       ` Ævar Arnfjörð Bjarmason
2021-09-27 13:37         ` Jeff Hostetler
2021-09-23 17:51     ` Taylor Blau

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87bl4tm5s1.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jeffhost@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.