From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id F1A1F1FF09B for ; Mon, 09 Nov 2026 07:58:50 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id F187621423; Mon, 09 Nov 2026 07:58:46 +0100 (CET) From: Kefu Chai To: w.bumiller@proxmox.com Subject: Re: [PATCH common 1/2] cmd: run: do not cancel the caller's alarm Date: Mon, 9 Nov 2026 14:58:35 +0800 Message-ID: <20261109065835.1694200-1-k.chai@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: References: <20261021110126.4024539-1-k.chai@proxmox.com> <20261021110126.4024539-2-k.chai@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1794207520547 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.231 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: MJDO7XVQIPXLK2DPKIYVGOQQAEMBOPSK X-Message-ID-Hash: MJDO7XVQIPXLK2DPKIYVGOQQAEMBOPSK X-MailFrom: k.chai@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Hi Wolfgang, thanks for the review! > There is technically a race here where at *this* point the outer alarm > can trigger. > > I suggest placing > > $oldtimeout = alarm(0) if $timeout; > > *above* the `local $SIG{ALRM}` line above first, so that if a timeout is > created, we first disable the alarm, then establish the signal handler, > *then* the new alarm (without re-assigning to `$oldtimeout` again). Good catch, the v2 does exactly that. [snip] > This now means an external timer can interrupt `$errfunc` which it could > not previously. > > I think it makes more sense to `eval` the `$errfunc` call and do > something like: > > if ($err eq "got timeout\n") { > warn $errfunc_error; > } else { > $err = "$errfunc_error\n$err"; > } Done like that. The new test has an errfunc that dies on the flushed line: run still reports its timeout and only warns the error. [snip] > ...then move this hunk to after the kill/wait code below, replacing > its `die`s with appending the error similar to the above code, so > the external timer also cannot prevent the kill/wait. > > This might then also make the 2nd patch unnecessary? The series already has that order: the second patch kills and reaps the command right after the eval, before the flush and before the caller's alarm is re-armed. The dies on the error paths come after the reap, so they cannot skip it either. But I don't think the second patch becomes redundant, because that unconditional kill/wait is what it adds in the first place. In the first patch alone, the kills sit on the timeout, sysread and callback paths. When the caller's alarm handler is what dies while the command runs, the 'rbd create' case, nothing kills the command, however the first patch is ordered. Happy to fold the two into one patch if that reads better. I kept them apart because they fix two different things. One window stays open on purpose: without a timeout of its own, run leaves the caller's alarm armed during the kill/wait too, so its handler can still die between the eval and the kill. Disarming it there would leave nothing to break out of the waitpid when a child is stuck in uninterruptible sleep, and I prefer keeping the caller's watchdog working over guaranteeing the reap. I'll send the v2 with both changes and their tests right after this.