On Dec 3 21:31, Takashi Yano wrote: > On Tue, 3 Dec 2024 21:17:47 +0900 > Takashi Yano wrote: > > On Mon, 2 Dec 2024 16:14:59 +0100 > > Corinna Vinschen wrote: > > > On Nov 29 20:48, Takashi Yano wrote: > > > > The queue is cleaned up by removing the entries having si_signo == 0 > > > > while processing the queued signals, however, sipacket::process() may > > > > set si_signo in the queue to 0 of the entry already processed but not > > > > succeed by calling sig_clear(). This patch ensures the sig_clear() > > > > to remove the entry from the queue chain. > > > > > > Thanks for this patch. I just have two questions. > > > > > > > Addresses: https://cygwin.com/pipermail/cygwin/2024-November/256744.html > > > > Fixes: 9d2155089e87 ("(wait_sig): Define variable q to be the start of > > > > the signal queue. Just iterate through sigq queue, deleting processed > > > > or zeroed signals") > > > > Reported-by: Christian Franke <christian.fra...@t-online.de> > > > > Reviewed-by: Corinna Vinschen <cori...@vinschen.de> > > > > Signed-off-by: Takashi Yano <takashi.y...@nifty.ne.jp> > > > > --- > > > > winsup/cygwin/local_includes/sigproc.h | 3 +- > > > > winsup/cygwin/sigproc.cc | 48 +++++++++++++++++--------- > > > > 2 files changed, 34 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/winsup/cygwin/local_includes/sigproc.h > > > > b/winsup/cygwin/local_includes/sigproc.h > > > > index 46e26db19..8b7062aae 100644 > > > > --- a/winsup/cygwin/local_includes/sigproc.h > > > > +++ b/winsup/cygwin/local_includes/sigproc.h > > > > @@ -50,8 +50,9 @@ struct sigpacket > > > > { > > > > HANDLE wakeup; > > > > HANDLE thread_handle; > > > > - struct sigpacket *next; > > > > }; > > > > + struct sigpacket *next; > > > > + struct sigpacket *prev; > > > > > > The former method using q and qnext ptr didn't need prev. The question > > > is, why did you add prev? If you think this has an advantage, even if > > > just better readability, it would be nice to document this in the commit > > > message. > > > > Consider the queued signal chain is like: > > A->B->C->D > > Assume that now 'q' and 'qnext' ptr points to C and process() is > > processing C. If B is cleared in process(), A->next should be set > > to C. > in clear(). > > > Then, if process() for C succeeds, C should be removed from > > the queue, so A->next should be set to D. However, now we cannot > > access to A because we do not have the pointer to A. > in while loop in wait_sig().
Thanks for the reasoning behind this. On second thought, there's no reason to add this to the commit message. Or just a single brief sentence. Corinna
