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 <cg...@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 <cg...@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 <cg...@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 <cg...@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 <cg...@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
signature.asc
Description: PGP signature
