Re: [PATCH] Re: Cygwin select() issues and improvements

[Corinna Vinschen <corinna-cygwin at cygwin dot com>]

John, ping?

On Mar 20 16:00, Corinna Vinschen wrote:
> On Mar 19 18:43, john hood wrote:
> > From c805552cdc9e673ef2330388ddb8b7a0da741766 Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Thu, 28 Jan 2016 17:08:39 -0500
> > Subject: [PATCH 1/5] Use high-resolution timebases for select().
> > 
> > 	* cygwait.h: Add cygwait_us() methods.
> > 	* select.h: Change prototype for select_stuff::wait() for larger
> > 	microsecond timeouts.
> > 	* select.cc (pselect): Convert from old cygwin_select().
> > 	Implement microsecond timeouts.
> > 	(cygwin_select): Rewrite as a wrapper on pselect().
> > 	(select): Implement microsecond timeouts.
> > 	(select_stuff::wait): Implement microsecond timeouts with a timer
> > 	object.
> 
> I have a bit of a problem with patch 1 and 4.  In the same patchset
> you add cygwait_us and remove it again.  That doesn't really look
> useful to me.  Can you create the patches so that this part is skipped,
> please?  The rest of the patch should work as is with the existing version
> of cygwait.
> 
> Two general style issues:
> 
> - Please don't use Microsofts variable naming convention.  It's used in
>   kernel.cc to use the same names as in the documentation and there are
>   a few rare cases where class members are using this convention, but
>   other than that we usually use lowercase and underscores only.  Please
>   use that as well.
> 
> - Always prepend a space to an opening bracket in function or macro calls,
>   gnu-style.  There are a couple of cases where you missed that.  If you find
>   such cases prior to your patch, pleae change them while you're at it.
> 
> Btw., it would be helpful to get a patch series the way git format-patch/
> send-email generates them.  It allows easier review to have every patch
> in a single mail.  I changed the text on https://cygwin.com/contrib.html
> to be a bit more clear about this.  Well, hopefully a bit more clear.
> 
> > @@ -362,13 +362,50 @@ err:
> >  /* The heart of select.  Waits for an fd to do something interesting. */
> >  select_stuff::wait_states
> >  select_stuff::wait (fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> > -		    DWORD ms)
> > +		    LONGLONG us)
> >  {
> >    HANDLE w4[MAXIMUM_WAIT_OBJECTS];
> >    select_record *s = &start;
> >    DWORD m = 0;
> >  
> > +  /* Always wait for signals. */
> >    wait_signal_arrived here (w4[m++]);
> > +
> > +  /* Set a timeout, or not, for WMFO. */
> > +  DWORD dTimeoutMs;
> > +  if (us == 0)
> > +    {
> > +      dTimeoutMs = 0;
> > +    }
> > +  else
> > +    {
> > +      dTimeoutMs = INFINITE;
> > +    }
> 
> Please, no braces for oneliners.  Also, this entire statement can be
> folded into a oneliner:
> 
>      ms = us ? INFINITE : 0;
> 
> > +  status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
> 
> Does it really make sense to build up and break down a timer per each
> call to select_stuff::wait?  This function is called in a loop.  What
> about creating the timer in the caller and only arm and disarm it in the
> wait call?
> 
> > +  if (dTimeoutMs == INFINITE)
> > +    {
> > +      CancelWaitableTimer (hTimeout);
> > +      CloseHandle (hTimeout);
> > +    }
> 
> For clarity, since the timer has been created and armed using native
> functions, please use NtCancelTimer and NtClose here.
> 
> > From 225f852594d9ff6a1231063ece3d529b9cc1bf7f Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Sat, 30 Jan 2016 17:33:36 -0500
> > Subject: [PATCH 2/5] Move get_nonascii_key into fhandler_console.
> > 
> > 	* fhandler.h (fhandler_console): Move get_nonascii_key() from
> > 	select.c into this class.
> > 	* select.cc (peek_console): Move get_nonascii_key() into
> > 	fhandler_console class.
> 
> Patch applied.
> 
> > From b2e2b5ac1d6b62610c51a66113e5ab97b1d43750 Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Sat, 30 Jan 2016 17:37:33 -0500
> > Subject: [PATCH 3/5] Debug printfs.
> > 
> > 	* fhandler.cc (fhandler_base::get_readahead): Add debug code.
> > 	* fhandler_console.cc (fhandler_console::read): Add debug code.
> > 	* select.cc (pselect): Add debug code.
> > 	(peek_console): Add debug code.
> 
> Why?  It's a lot of additional debug output.  Was that only required for
> developing or does it serve a real purpose for debugging user bug reports
> in future?  If so, I wouldn't mind to have a bit of additional description
> in the git log to explain the debug statements...
> 
> > From cf2db014fefd4a8488316cf9313325b79e25518d Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Thu, 4 Feb 2016 00:44:56 -0500
> > Subject: [PATCH 4/5] Improve and simplify select().
> > 
> > 	* cygwait.h (cygwait_us) Remove; this reverts previous changes.
> > 	* select.h: Eliminate redundant select_stuff::select_loop state.
> > 	* select.cc (select): Eliminate redundant
> 
> See above.
> 
> > @@ -182,30 +181,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
> >  	  }
> >        select_printf ("sel.always_ready %d", sel.always_ready);
> >  
> > -      /* Degenerate case.  No fds to wait for.  Just wait for time to run out
> > -	 or signal to arrive. */
> > -      if (sel.start.next == NULL)
> > -	switch (cygwait_us (us))
> > -	  {
> > -	  case WAIT_SIGNALED:
> > -	    select_printf ("signal received");
> > -	    /* select() is always interrupted by a signal so set EINTR,
> > -	       unconditionally, ignoring any SA_RESTART detection by
> > -	       call_signal_handler().  */
> > -	    _my_tls.call_signal_handler ();
> > -	    set_sig_errno (EINTR);
> > -	    wait_state = select_stuff::select_signalled;
> > -	    break;
> > -	  case WAIT_CANCELED:
> > -	    sel.destroy ();
> > -	    pthread::static_cancel_self ();
> > -	    /*NOTREACHED*/
> > -	  default:
> > -	    /* Set wait_state to zero below. */
> > -	    wait_state = select_stuff::select_set_zero;
> > -	    break;
> > -	  }
> > -      else if (sel.always_ready || us == 0)
> 
> This obviously allows to fold everything into select_stuff::wait, but
> the more it seems like a good idea to move the timer creation into the
> caller for this case, doesn't it?
> 
> Alternatively, we could add a per-thread timer handle to the cygtls
> area.  It could be created on first use and deleted when the thread
> exits.  But that's just an idea for a future improvement, never
> mind for now.
> 
> > From 3f3f7112f948d70c15046641cf3cc898a9ca4c71 Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Fri, 18 Mar 2016 04:31:16 -0400
> > Subject: [PATCH 5/5] 	* winsup/testsuite/configure: chmod a+x
> 
> Applied.
> 
> 
> Thanks,
> Corinna
> 
> -- 
> Corinna Vinschen                  Please, send mails regarding Cygwin to
> Cygwin Maintainer                 cygwin AT cygwin DOT com
> Red Hat



-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

Attachment: signature.asc
Description: PGP signature