[Patch] implementation of select for write on pipes

The following patch implements select for write on pipes.  Currently,
pipes always select writable, which sometimes causes programs like
rsync to hang when the pipe is full.  This can be observed directly from
strace output:

    http://www.mail-archive.com/rsync <at> lists.samba.org/msg07559.html

We have been using this patch successfully for several months for
automated builds of our Windows products using Cygwin, so it has been
extensively tested.  It is the first of several patches that I plan to
contribute, all with the goal of preventing Cygwin hangs, and it does
seem to really improve the behavior of programs like rsync (and sshd)
that use select extensively with pipes.

The existing code uses PeekNamedPipe to implement select for read on
pipes.  For the corresponding write test, I used NtQueryInformationFile to
fill in a FILE_PIPE_LOCAL_INFORMATION struct, which contains OutboundQuota
and WriteQuotaAvailable fields that allow us to detect when the pipe is
full.  I fixed the signature and return value for NtQueryInformationFile
as well.

Unfortunately, NtQueryInformationFile isn't supported by Win9x, so we
optimistically assume the pipe is always writable on those systems (i.e.,
no change from the status quo).

NtQueryInformationFile also requires FILE_READ_ATTRIBUTES access on the
write side of the pipe, but CreatePipe doesn't set that (at least before
WinXP SP2), so I added a new create_selectable_pipe function to do the
right thing: it uses CreateNamedPipe for the read side, and CreateFile
for the write side (with FILE_READ_ATTRIBUTES).  It's important to only

Re: [Patch] implementation of select for write on pipes

On Thu, Sep 02, 2004 at 06:25:55PM -0400, Bob Byrnes wrote:
>The following patch implements select for write on pipes.

On a quick look, this looks very nice.  I will give it a further review
on Saturday but it looks like you got the formatting and ChangeLog
right, which is almost unheard-of for a first time submission.  So,
this may just slide right in.

Thanks!

cgf

Re: [Patch] implementation of select for write on pipes

On Thu, Sep 02, 2004 at 06:54:49PM -0400, Christopher Faylor wrote:
>On Thu, Sep 02, 2004 at 06:25:55PM -0400, Bob Byrnes wrote:
>>The following patch implements select for write on pipes.
>
>On a quick look, this looks very nice.  I will give it a further review
>on Saturday but it looks like you got the formatting and ChangeLog
>right, which is almost unheard-of for a first time submission.  So,
>this may just slide right in.
>
>Thanks!

Actually, I had time tonight and, while it didn't just slide in, it
was very close.

Here are my changes to your original patch.

There were a few style issues which I've rectified:

In 'debug_printf"s the style is to use "foo %s" rather than "foo = %s"
so I've done that where I noticed it.

Also, the use of "%E" is supposed to be ", %E" rather than ": %E" although
I do see a number of uses of ": %E" in cygwin, now that I notice it.

There were also a few spacing issues and gratuitous use of braces
which I rectified.

Some non-style issues:

While I know that using snprintf is generally a good thing and it

Re: [Patch] mapping root directory to SystemDrive / CurrentDrive

At 10:20 PM 8/3/2004 -0400, Christopher Faylor wrote:
>On Tue, Aug 03, 2004 at 08:23:52PM -0400, Pierre A. Humblet wrote:
>>Here is a patch.
>
>Thanks much but unless you are really really sure that this patch will
>introduce no regressions, I'd like to hold off applying this patch until
>after 1.5.11.

Now is the time.

Pierre

2004-09-05  Pierre Humblet <pierre.humblet <at> ieee.org>

	* cygheap.h (cwdstuff::drive_length): New member.
	(cwdstuff::get_drive): New method.
	* path.cc (normalize_win32_path): Simplify by using cwdstuff::get_drive.
	(mount_info::conv_to_win32_path): Use cwdstuff::get_drive as default for /.
	(cwdstuff::set): Initialize drive_length.

Index: cygheap.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/cygheap.h,v
retrieving revision 1.85
diff -u -p -r1.85 cygheap.h
--- cygheap.h	28 May 2004 19:50:05 -0000	1.85
+++ cygheap.h	5 Sep 2004 11:56:28 -0000
 <at>  <at>  -216,9 +216,16  <at>  <at>  struct cwdstuff

Re: [Patch] mapping root directory to SystemDrive / CurrentDrive

On Sun, Sep 05, 2004 at 07:59:22AM -0400, Pierre A. Humblet wrote:
>At 10:20 PM 8/3/2004 -0400, Christopher Faylor wrote:
>>On Tue, Aug 03, 2004 at 08:23:52PM -0400, Pierre A. Humblet wrote:
>>>Here is a patch.
>>
>>Thanks much but unless you are really really sure that this patch will
>>introduce no regressions, I'd like to hold off applying this patch until
>>after 1.5.11.
>
>Now is the time.

Yep.  Please check in.

cgf

[Patch] implementation of nonblocking writes on pipes

The following patch implements nonblocking writes on pipes.  Currently,
pipes ignore the O_NONBLOCK flag for writing, and programs like sshd or
rsync that use nonblocking I/O heavily can hang when writes unexpectedly
block.

This patch is similar to (and relies on) my previous patch to implement
select for write on pipes.  We have been using this for several months
for automated builds of our Windows products, and it does seem to help
prevent Cygwin hangs.

The existing code uses PeekNamedPipe to check for available data
before attempting nonblocking reads, with a guard mutex to ensure
that the data is not stolen by some other thread between the initial
PeekNamedPipe and the subsequent read.  For nonblocking writes, I used
NtQueryInformationFile to check for available space before writing,
with a similar guard mutex.

As for select, since NtQueryInformationFile isn't supported by Win9x, we
optimistically assume that we can transfer some data without blocking on
those systems (i.e., no change from the status quo).  We punt in the same
way if NtQueryInformationFile fails because we have somehow inherited a
(non-Cygwin) pipe without FILE_READ_ATTRIBUTES.

POSIX rules for nonblocking writes on pipes are a bit complicated;
I haved tried to annotate the code with comments from the spec.  If we
somehow inherit a tiny (non-Cygwin) pipe with size < PIPE_BUF, then we
try to do partial writes, as POSIX mandates for writes of size > PIPE_BUF.

--
Bob Byrnes

Re: RTLD_DEFAULT & RTLD_NEXT

the (C) assignment is in the mail.

2004-08-31  Sam Steingold  <sds <at> gnu.org>

	* dlfcn.cc (dlsym): Handle RTLD_DEFAULT using EnumProcessModules().
	* include/dlfcn.h (RTLD_DEFAULT): Define to NULL.

-- 
Sam Steingold (http://www.podval.org/~sds) running w2k
<http://www.camera.org> <http://www.iris.org.il> <http://www.memri.org/>
<http://www.mideasttruth.com/> <http://www.honestreporting.com>
Don't use force -- get a bigger hammer.

Index: src/winsup/cygwin/include/dlfcn.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/include/dlfcn.h,v
retrieving revision 1.2
diff -u -w -r1.2 dlfcn.h
--- src/winsup/cygwin/include/dlfcn.h	11 Sep 2001 20:01:01 -0000	1.2
+++ src/winsup/cygwin/include/dlfcn.h	7 Sep 2004 20:47:33 -0000
 <at>  <at>  -28,6 +28,7  <at>  <at> 
 extern void dlfork (int);

 /* following doesn't exist in Win32 API .... */
+#define RTLD_DEFAULT    NULL

 /* valid values for mode argument to dlopen */
 #define RTLD_LAZY	1	/* lazy function call binding */
Index: src/winsup/cygwin/dlfcn.cc
===================================================================

[Patch]: Setting the winpid in pinfo

I am looking at some oddities involving ^C and signals.

In the current code, set_myself sets the dwProcessId when
a pinfo is created. The upshot is that signals to a process
that is exec'ing will be prematurely directed to the child
(using a meaningless handle from the parent). 
The bug can be fixed by simply removing the offending line.

Also, in proc_can_be_signalled, I don't understand the line  
  if (ISSTATE (p, PID_INITIALIZING) ||
      (((p)->process_state & (PID_ACTIVE | PID_IN_USE)) ==
       (PID_ACTIVE | PID_IN_USE)))
    return true;
The test for PID_INITIALIZING will allow a doomed attempt
at signaling a process while it is being forked (and its
sendsig is still NULL). Am I missing something?

Also, on WinME, simply holding down ^C in the bash shell will
cause a crash (thanks to Errol Smith)
~>     142 [sig] BASH 1853149 handle_threadlist_exception: 
handle_threadlist_exception called with threadlist_ix -1
    1751 [sig] BASH 1853149 handle_exceptions: Exception: 
STATUS_ACCESS_VIOLATION

Any idea about what's happening? I have been unable to
make any progress.

Pierre

2004-09-08  Pierre Humblet <pierre.humblet <at> ieee.org>

Re: [Patch]: Setting the winpid in pinfo

On Tue, Sep 07, 2004 at 09:26:02PM -0400, Pierre A. Humblet wrote:
>I am looking at some oddities involving ^C and signals.
>
>In the current code, set_myself sets the dwProcessId when a pinfo is
>created.  The upshot is that signals to a process that is exec'ing will
>be prematurely directed to the child (using a meaningless handle from
>the parent).  The bug can be fixed by simply removing the offending
>line.

It's odd but I would have sworn that I'd removed that setting of
dwProcessId some time ago.  It's been there since 2000, though, so
I am obviously mistaken.

The intent was to set this back to the parent's pid so that the stub
would deal with any potential signal but that wouldn't work right when a
process execs another process anyway, since the cygpid is the pid of the
original process.  I'm not sure why I just didn't forego setting
dwProcessId entirely in that case.  Seems like a botched edit.

Anyway, I've just yanked out all of that code and removed cygpid from
the child_info structure.  child_info reflects a time before the advent
of the cygheap.  Adding the pid to the cygheap at the appropriate time
and using that seems to simplify things a lot.  I basically got rid of
all of the cypid usages and modified set_myself:

Index: pinfo.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/pinfo.cc,v
retrieving revision 1.119
diff -u -p -r1.119 pinfo.cc

Re: RTLD_DEFAULT & RTLD_NEXT

On Sep  7 16:52, Sam Steingold wrote:
> the (C) assignment is in the mail.

Cool.  I'm looking forward to getting the ok from our HQ.

> Index: src/winsup/cygwin/autoload.cc
> ===================================================================
> RCS file: /cvs/src/src/winsup/cygwin/autoload.cc,v
> retrieving revision 1.87
> diff -u -w -r1.87 autoload.cc
> --- src/winsup/cygwin/autoload.cc	3 Sep 2004 01:32:02 -0000	1.87
> +++ src/winsup/cygwin/autoload.cc	7 Sep 2004 20:47:33 -0000
>  <at>  <at>  -309,6 +309,7  <at>  <at> 
>  LoadDLLfunc (DeregisterEventSource, 4, advapi32)
>  LoadDLLfunc (DuplicateToken, 12, advapi32)
>  LoadDLLfuncEx (DuplicateTokenEx, 24, advapi32, 1)
> +LoadDLLfuncEx (EnumProcessModules, 16, psapi, 1)

That's not quite the right place to add this line 
Hint:  The autoload list is sorted by libraries...

Corinna

--

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Project Co-Leader          mailto:cygwin <at> cygwin.com
Red Hat, Inc.