From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21315 invoked by alias); 7 Dec 2014 19:04:48 -0000 Mailing-List: contact zsh-workers-help@zsh.org; run by ezmlm Precedence: bulk X-No-Archive: yes List-Id: Zsh Workers List List-Post: List-Help: X-Seq: 33911 Received: (qmail 24318 invoked from network); 7 Dec 2014 19:04:47 -0000 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on f.primenet.com.au X-Spam-Level: X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 X-Originating-IP: [86.6.25.230] X-Spam: 0 X-Authority: v=2.1 cv=dY0O3Bne c=1 sm=1 tr=0 a=c0CwWhpM9oUd/BnC3z6Gzg==:117 a=c0CwWhpM9oUd/BnC3z6Gzg==:17 a=NLZqzBF-AAAA:8 a=A92cGCtB03wA:10 a=s-uEEaQwMzQbBxhQdNsA:9 a=NRXItZ7bQybxucbA:21 a=XhmnJ72iKMn2lZKm:21 Message-Id: <201412071859.sB7Ix5Xq005917@pws-pc.ntlworld.com> X-Authentication-Warning: pws-pc.ntlworld.com: pws owned process doing -bs From: Peter Stephenson To: zsh-workers@zsh.org (Zsh hackers list) Subject: Re: Interrupting globs (Re: Something rotten in tar completion) In-Reply-To: Your message of "Sun, 07 Dec 2014 10:34:19 PST." <141207103419.ZM22731@torch.brasslantern.com> Date: Sun, 07 Dec 2014 18:59:04 +0000 Bart Schaefer wrote: > On Dec 7, 5:07pm, Peter Stephenson wrote: > } > } Other issues: > } > } Mikael needs to ^C three times to get back to the command line --- > } apparently related to the code from Sven I took at its word. > > (Sven is Oberon? If that's true, I'd entirely forgotten it.) I'm pretty sure I didn't make that up. > } I agree we might want to look at some always blocks. BTW, I didn't > } actually think through the handling of "always" blocks for this patch, > } so it might want looking at. > > In particular with respect to always-blocks, there should be some way > for TRY_BLOCK_ERROR to clear interrupts as well as (maybe even instead > of? TRY_BLOCK_INTERRUPT?) internal errors. A trap that returns zero > only clears interrupt if there isn't another trap in a deeper scope. Yes, probably. I realised we'd better not do that with bits in the shell code as the documentation suggests setting TRY_BLOCK_ERROR to 0 clears errors. We'd want clearing interrupts to be a separate explicit action. > Maybe TRY_BLOCK_ERROR does still clear both in the patch, I didn't > parse it that carefully. The current code causes ERRFLAG_INT to propagate and only updates ERRFLAG_ERROR owing to TRY_BLOCK_ERROR. I suppose that's correct for the basic operation, i.e. unless we do some something special as above interrupts propagate. However, I think it definitely needs to clear ERRFLAG_INT for the duration of the always clause, or the code won't execute at all, even if it restores it later. The patch below does this and notes the other issue. > } It's also just occurred to me I may have introduced some rare but > } entirely possible read-modify-write races because we set the ERRFLAG_INT > } bit in interrupts and set the other and clear both bits separately in > } the main shell. I guess it would be better to queue interrupts whenever > } we add or remove a single bit of errflag > > I don't think single bits are a problem because it's always done with > bitwise-or. If anything, we'd need to queue any time we clear the > interrupt bit, so that a signal that comes in while we're clearing it > gets treated as if arrived after clearing. In practice I don't think > it makes that much difference. I'm worried about the various occurrences of this: /* Keep any user interrupt error status */ errflag = oef | (errflag & ERRFLAG_INT); which can cause - read errflag to determine ERRFLAG_INT bit; find it's not set - interrupt sets ERRFLAG_INT - errflag gets set to value without ERRFLAG_INT. It's a narrow race, but I think it's a real one. pws diff --git a/Src/loop.c b/Src/loop.c index 69805ea..9b0a8d7 100644 --- a/Src/loop.c +++ b/Src/loop.c @@ -643,7 +643,7 @@ exectry(Estate state, int do_exec) { Wordcode end, always; int endval; - int save_retflag, save_breaks, save_contflag; + int save_retflag, save_breaks, save_contflag, try_interrupt; zlong save_try_errflag, save_try_tryflag; end = state->pc + WC_TRY_SKIP(state->pc[-1]); @@ -671,8 +671,10 @@ exectry(Estate state, int do_exec) /* The always clause. */ save_try_errflag = try_errflag; - try_errflag = (zlong)errflag; - errflag &= ~ERRFLAG_ERROR; + try_errflag = (zlong)(errflag & ERRFLAG_ERROR); + try_interrupt = errflag & ERRFLAG_INT; + /* We need to reset all errors to allow the block to execute */ + errflag = 0; save_retflag = retflag; retflag = 0; save_breaks = breaks; @@ -687,6 +689,17 @@ exectry(Estate state, int do_exec) errflag |= ERRFLAG_ERROR; else errflag &= ~ERRFLAG_ERROR; + /* + * TODO: currently, we always restore the interrupt + * error status. We should have a way of clearing it. + * Doing this with try_errflag (the shell variable TRY_BLOCK_ERROR) + * is probably not a good idea since currently that's documented + * such that setting it to 0 clears errors, and we don't want + * to clear interrupts as a side effect. So it probably needs + * a different variable. + */ + if (try_interrupt) + errflag |= ERRFLAG_INT; try_errflag = save_try_errflag; if (!retflag) retflag = save_retflag;