This is the mail archive of the cygwin-patches mailing list for the Cygwin project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi John, 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
Attachment:
signature.asc
Description: PGP signature
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |