git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH] mingw: handle writes to non-blocking pipe
Date: Wed, 10 Aug 2022 16:02:41 -0400	[thread overview]
Message-ID: <YvQO4ZYI8/fAk0Gj@coredump.intra.peff.net> (raw)
In-Reply-To: <72d007c5-9429-1612-24d7-af5db906dd63@web.de>

On Wed, Aug 10, 2022 at 07:39:40AM +0200, René Scharfe wrote:

> write() on Windows reports ENOSPC when writing to a non-blocking pipe
> whose buffer is full and rejects writes bigger than the buffer outright.
> Change the error code to EAGAIN and try a buffer-sized partial write to
> comply with POSIX and the expections of our Git-internal callers.

Bearing in mind that I have no qualifications for reviewing
Windows-specific patches, this seems like the right thing to be doing
from the behavior you saw.

One question:

> +	if (result < 0 && errno == ENOSPC) {
> +		/* check if fd is a non-blocking pipe */
> +		HANDLE h = (HANDLE) _get_osfhandle(fd);
> +		DWORD s;
> +		if (GetFileType(h) == FILE_TYPE_PIPE &&
> +		    GetNamedPipeHandleState(h, &s, NULL, NULL, NULL, NULL, 0) &&
> +		    (s & PIPE_NOWAIT)) {
> +			DWORD obuflen;
> +			if (!GetNamedPipeInfo(h, NULL, &obuflen, NULL, NULL))
> +				obuflen = PIPE_BUFFER_SIZE;
> +			if (len > obuflen)
> +				return mingw_write(fd, buf, obuflen);
> +			errno = EAGAIN;
> +		}

OK, so we call GetNamedPipeInfo() to find the size of the pipe buffer.
It's unclear to me from Microsoft's docs if that is the _total_ size, or
if it's the remaining available size. Hopefully the latter, since none
of this works otherwise. ;)

But two corner cases:

  - If we fail to get the size, we guess that it's the maximum. Is this
    dangerous? I'm not sure why the call would fail, but if for some
    reason it did fail and we can't make forward progress, would we
    enter an infinite recursion of mingw_write()? Would it be safer to
    bail with EAGAIN in such a case (through granted, that probably just
    puts us into an infinite loop in xwrite())?

  - According to the docs:

      If the buffer size is zero, the buffer is allocated as needed.

    If we see this case, we'd then call mingw_write() with 0 bytes,
    which I imagine also makes no forward progress (though maybe we
    eventually return a successful 0-byte write?).

-Peff

  parent reply	other threads:[~2022-08-10 20:02 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-02  4:13 [RFC/PATCH] pipe_command(): mark stdin descriptor as non-blocking Jeff King
2022-08-02 15:04 ` Junio C Hamano
2022-08-02 15:39 ` Jeff King
2022-08-02 16:16   ` Junio C Hamano
2022-08-03  3:53     ` [PATCH v2] " Jeff King
2022-08-03 16:45       ` René Scharfe
2022-08-03 17:20         ` Jeff King
2022-08-03 21:56           ` René Scharfe
2022-08-05 15:36             ` Jeff King
2022-08-05 21:13               ` René Scharfe
2022-08-07 10:15                 ` René Scharfe
2022-08-07 17:41                   ` Jeff King
2022-08-10  5:39                     ` René Scharfe
2022-08-10 19:53                       ` Jeff King
2022-08-10 22:35                         ` René Scharfe
2022-08-11  8:52                           ` Jeff King
2022-08-10  5:39                     ` [PATCH] mingw: handle writes to non-blocking pipe René Scharfe
2022-08-10  9:07                       ` Johannes Schindelin
2022-08-10 20:02                       ` Jeff King [this message]
2022-08-10 22:34                         ` René Scharfe
2022-08-11  8:47                           ` Jeff King
2022-08-11 17:35                             ` René Scharfe
2022-08-11 18:20                               ` Jeff King
2022-08-14 15:37                                 ` René Scharfe
2022-08-17  5:39                                   ` Jeff King
2022-08-17  6:04                                     ` [PATCH v2 0/6] fix pipe_command() deadlock Jeff King
2022-08-17  6:04                                       ` [PATCH v2 1/6] compat: add function to enable nonblocking pipes Jeff King
2022-08-17 20:23                                         ` Junio C Hamano
2022-08-18  5:41                                           ` Jeff King
2022-08-17  6:05                                       ` [PATCH v2 2/6] nonblock: support Windows Jeff King
2022-08-17  6:06                                       ` [PATCH v2 3/6] git-compat-util: make MAX_IO_SIZE define globally available Jeff King
2022-08-17  6:08                                       ` [PATCH v2 4/6] pipe_command(): avoid xwrite() for writing to pipe Jeff King
2022-08-17  6:09                                       ` [PATCH v2 5/6] pipe_command(): handle ENOSPC when writing to a pipe Jeff King
2022-08-17 18:57                                         ` Junio C Hamano
2022-08-18  5:38                                           ` Jeff King
2022-08-17  6:10                                       ` [PATCH v2 6/6] pipe_command(): mark stdin descriptor as non-blocking Jeff King
2022-08-17  6:20                                       ` [PATCH v2 0/6] fix pipe_command() deadlock Jeff King
2022-08-19 21:19                                       ` René Scharfe
2022-08-20  7:04                                         ` Jeff King
2022-08-07 10:14               ` [PATCH v2] pipe_command(): mark stdin descriptor as non-blocking René Scharfe
2022-08-08 12:55                 ` Johannes Schindelin
2022-08-08 12:59       ` Johannes Schindelin
2022-08-09 13:04         ` Jeff King
2022-08-09 22:10           ` Johannes Schindelin

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=YvQO4ZYI8/fAk0Gj@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    /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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).