Hi Takashi, On Wed, 25 Jun 2025, Takashi Yano wrote:
> Reviewed-by: > Signed-off-by: Takashi Yano <[email protected]> This is way too terse. There is a difference between being succinct and leaving things unsaid. Also, please make sure that v2 is a reply to v1 of the patch. I almost commented on v1 by mistake. > --- > winsup/cygwin/fhandler/pipe.cc | 10 ++-------- > 1 file changed, 2 insertions(+), 8 deletions(-) > > diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc > index e35d523bb..c35411abf 100644 > --- a/winsup/cygwin/fhandler/pipe.cc > +++ b/winsup/cygwin/fhandler/pipe.cc > @@ -443,7 +443,6 @@ ssize_t > fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) > { > size_t nbytes = 0; > - ULONG chunk; Okay, removing this local variable is a good indicator that this diff shows all the related logic, without having to resort to looking at the entire `pipe.cc` file that is not reproduced in this email. > NTSTATUS status = STATUS_SUCCESS; > IO_STATUS_BLOCK io; > HANDLE evt; > @@ -540,11 +539,6 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > } > } > > - if (len <= (size_t) avail) > - chunk = len; > - else > - chunk = avail; > - > if (!(evt = CreateEvent (NULL, false, false, NULL))) > { > __seterrno (); > @@ -561,8 +555,8 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t > len) > ULONG len1; > DWORD waitret = WAIT_OBJECT_0; > > - if (left > chunk && !is_nonblocking ()) > - len1 = chunk; > + if (left > (size_t) avail && !is_nonblocking ()) > + len1 = (ULONG) avail; > else > len1 = (ULONG) left; So there is a subtle change here, which _should_ result in the same behavior, but it is far from obvious. If both `left` and `len` are larger than `avail`, the behavior is obviously the same as before because `len1` is clamped to `avail` in that instance. If `left` is smaller than `len`, and `len` is smaller than `avail`, `len1` is clamped to `left`, same as before. But even if `left < avail < len`, `chunk` would have previously been set to `avail` and the behavior is the same. Since the variable `left` is defined as `left = len - nbytes`, it is never larger than `len`. Without giving a simple overview of this in the commit message, every reader will have to (re-)reason out this finding, which is suboptimal. Please improve the commit message, for that reason. Also note that this patch conflicts very much with https://inbox.sourceware.org/cygwin-patches/62e79c50daf4e3ae28db3ae1a3cf52460f0d8968.1750775114.git.johannes.schinde...@gmx.de/ and it would therefore make most sense to focus first on landing that patch, then redo v2 of this here patch on top. Ciao, Johannes
