From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id iwh9Kjyju2r7LhcAWB0awg (envelope-from ) for ; Tue, 29 Sep 2026 07:38:36 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=fe9n3Bz9; dkim=pass header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=J1Vo9EiH; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=UkWND9A3; dkim=neutral header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=iN6kNpMD; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 966961E033; Tue, 29 Sep 2026 07:38:36 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 778541E033 for ; Tue, 29 Sep 2026 07:38:35 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id C59CD4BB3B92 for ; Tue, 29 Sep 2026 11:38:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C59CD4BB3B92 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=fe9n3Bz9; dkim=pass header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=J1Vo9EiH; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=UkWND9A3; dkim=neutral header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=iN6kNpMD Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.223.130]) by sourceware.org (Postfix) with ESMTPS id E6AF54BB24D6 for ; Tue, 29 Sep 2026 11:38:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E6AF54BB24D6 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E6AF54BB24D6 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=195.135.223.130 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790681885; cv=none; b=TzYotruPqScyBikJmkxfJqZNbKv2AmG+K2cWVnWSmS8eyLM/YZ6MC/GiGM59r1nWpWuP7pNi/nCtkOBqHJzhN9r7HItnFrwmxl85poXCc+XaknTggRRxZSwSuNbBGprw6UvVB6kLVpx0cUm4Q5p5bTdPZmrsnC24tNwBbBtSk0I= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790681885; c=relaxed/simple; bh=LKD6e8vXlvGxmAthz/G8P2+ADyMNJEgub5xhXQmwgdY=; h=DKIM-Signature:DKIM-Signature:DKIM-Signature:DKIM-Signature: Message-ID:Date:MIME-Version:Subject:To:From; b=DJQ7aoID016+FDe5XCgpLqdAnetQ4oXMmbpaXIS7gAJ0WgxfkbmnXyFmc/B/m6TOgvW0tYTzEsQFp/8monUuW8TfwuRXs2gBBkImZ9KYrgDEZNy3Zt2K39YRH2OG5jn1uEdVzgzhnbIAQlNk4ldnGsxk+PPA/YGov9NWhe5TuXo= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=fe9n3Bz9; dkim=pass header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=J1Vo9EiH; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.a=rsa-sha256 header.s=susede2_rsa header.b=UkWND9A3; dkim=neutral header.d=suse.de header.i=@suse.de header.a=ed25519-sha256 header.s=susede2_ed25519 header.b=iN6kNpMD DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org E6AF54BB24D6 Received: from imap1.dmz-prg2.suse.org (unknown [10.150.64.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id B3E6321BA2; Tue, 29 Sep 2026 11:37:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1790681879; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TKQ2tIt2TvJXCB852xmnc9okjH+U4XPfMZkDI+cDFdI=; b=fe9n3Bz9ft3t51reg/46yvE1wKrHaqG4+sJEieTf0dm69dQmrUOBPapnRotzaKOJMl37l2 yJodadeGMsiTvHskD2rX3wC6zOCwgiKJtBQTH1nmV8A/M+A0YNvu2yej11H2ixr9OvaTeh QvwttiWesmux02wbODWQKRqZX3Q1e1o= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1790681879; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TKQ2tIt2TvJXCB852xmnc9okjH+U4XPfMZkDI+cDFdI=; b=J1Vo9EiHDRD6fmagvO/r8odz0tE3zO0xrENCCXQ8ymEIc2N+3p1hiFJKvHcy358tiV90d5 zXfORA57kQHa6OBw== Authentication-Results: smtp-out1.suse.de; none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1790681875; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TKQ2tIt2TvJXCB852xmnc9okjH+U4XPfMZkDI+cDFdI=; b=UkWND9A3aC03e3iJH7q7ebw1MwpUnPwnKJ8q772gUheoz8AlB+dYPrfParvdinz+neLJUX DjTP26+c/pgPvXxEE8Q+l7rpcDjspja8Ihm832kdco0M4I6++fXY/TtPKa7KWf/V3pTTMz i+Ev79f29aMGZEnzkJ1hPMwXqcQm8DM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1790681875; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TKQ2tIt2TvJXCB852xmnc9okjH+U4XPfMZkDI+cDFdI=; b=iN6kNpMDOFQugrfO4/KGHRovCACiHlXl/dCcwHpVOKZoPKFgZVR/32qN9MwURgJyyrKRKG WS2zaQ//SylY8SDg== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 7E565136E4; Tue, 29 Sep 2026 11:37:55 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id LPHMFBOju2qYIQAAD6G6ig (envelope-from ); Tue, 29 Sep 2026 11:37:55 +0000 Message-ID: <75ddc9f8-33d8-49e4-ad39-4e24e0a8eb8d@suse.de> Date: Tue, 29 Sep 2026 13:37:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] [gdb/testsuite] Refactor lock_file_{acquire,release} To: Andrew Burgess , gdb-patches@sourceware.org References: <20260925220601.2021240-1-tdevries@suse.de> <87eced1fmi.fsf@redhat.com> Content-Language: en-US From: Tom de Vries In-Reply-To: <87eced1fmi.fsf@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spamd-Result: default: False [-4.30 / 50.00]; BAYES_HAM(-3.00)[100.00%]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.20)[-1.000]; MIME_GOOD(-0.10)[text/plain]; RCVD_VIA_SMTP_AUTH(0.00)[]; ARC_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; MID_RHS_MATCH_FROM(0.00)[]; RCPT_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[]; DKIM_SIGNED(0.00)[suse.de:s=susede2_rsa,suse.de:s=susede2_ed25519]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; FROM_EQ_ENVFROM(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; DBL_BLOCKED_OPENRESOLVER(0.00)[imap1.dmz-prg2.suse.org:helo, sourceware.org:url, gnu.org:url, suse.de:email, suse.de:mid] X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org On 9/28/26 6:53 PM, Andrew Burgess wrote: > Tom de Vries writes: > >> Refactor lock_file_acquire to limit the loop body to the minimum. > > It would be great if commits like this explained a little more about > what was being changed and why. Why does it need reducing? > Hi Andrew, thanks for the review. I think the comment is somewhat terse as a consequence of the fact that I didn't take the time to factor this patch out into a series. I've done so now in a v2 with 6 patches (forgot to include cover letter in submission, so): - cover letter: https://sourceware.org/pipermail/gdb-patches/2026-September/230720.html - first patch: https://sourceware.org/pipermail/gdb-patches/2026-September/230716.html I've tried to explain in a bit more detail in the patch dedicated to this specific change. >> Also make error handling more precise by only ignoring EEXIST. >> >> Refactor both lock_file_acquire and lock_file_release to use better variable >> names. >> >> Add a test-case checking how the two procs interact. >> >> In order to facilitate more complex testing, factor out a lock_file_acquire >> parameter called on_retry, without changing default behavior. >> --- >> gdb/testsuite/gdb.testsuite/lock.exp | 86 ++++++++++++++++++++++++++++ >> gdb/testsuite/lib/gdb-utils.exp | 59 ++++++++++++------- >> 2 files changed, 123 insertions(+), 22 deletions(-) >> create mode 100644 gdb/testsuite/gdb.testsuite/lock.exp >> >> diff --git a/gdb/testsuite/gdb.testsuite/lock.exp b/gdb/testsuite/gdb.testsuite/lock.exp >> new file mode 100644 >> index 00000000000..86a972e807c >> --- /dev/null >> +++ b/gdb/testsuite/gdb.testsuite/lock.exp >> @@ -0,0 +1,86 @@ >> +# Copyright 2026 Free Software Foundation, Inc. >> + >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 3 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see . >> + >> +set lockfile [build_standard_output_file "lock.txt"] >> +gdb_assert {![file exists $lockfile]} "Initial" >> + >> +# Scenario 1: >> +# - acquire 1 >> +# - release 1 >> +with_test_prefix "simple" { >> + set res [lock_file_acquire $lockfile] >> + gdb_assert {[file exists $lockfile]} "Acquire lock" >> + >> + lock_file_release $res >> + gdb_assert {![file exists $lockfile]} "Release lock" >> +} >> + >> +# Scenario 2: >> +# - acquire 1 >> +# - acquire 2 fails. On retry, release 1. Acquire 2 succeeds. >> +# - release 2 >> +with_test_prefix "on retry unlock" { >> + set res [lock_file_acquire $lockfile] >> + gdb_assert {[file exists $lockfile]} "Acquire lock" >> + >> + set on_retry [list lock_file_release $res] >> + # Try to acquire the lock. That should fail. >> + # Then on retry, release the lock. >> + # The next try to acquire the lock should succeed. >> + set res2 [lock_file_acquire $lockfile $on_retry] >> + gdb_assert {[file exists $lockfile]} "Re-acquire lock" >> + >> + lock_file_release $res2 >> + gdb_assert {![file exists $lockfile]} "Release lock" >> +} >> + >> +# Scenario 3: >> +# - acquire 1 >> +# - acquire 2 fails, waits, and tries again in a loop. >> +# - release 1, acquire 2 succeeds. >> +# - release 2 >> +with_test_prefix "on retry wait" { >> + set res [lock_file_acquire $lockfile] >> + gdb_assert {[file exists $lockfile]} "Acquire lock" >> + >> + after 1000 {lock_file_release $res} >> + >> + set on_retry { >> + update >> + after 100 >> + } >> + set res2 [lock_file_acquire $lockfile $on_retry] >> + gdb_assert {[file exists $lockfile]} "Re-acquire lock" >> + >> + lock_file_release $res2 >> + gdb_assert {![file exists $lockfile]} "Release lock" >> +} >> + >> +# Scenario 4: >> +# - acquire 1 >> +# - acquire 2 fails >> +with_test_prefix "on retry return" { >> + set res [lock_file_acquire $lockfile] >> + gdb_assert {[file exists $lockfile]} "Acquire lock" >> + >> + set on_retry { >> + return {} >> + } >> + set res2 [lock_file_acquire $lockfile $on_retry] >> + gdb_assert {$res2 == {}} "Acquire lock again" >> + >> + lock_file_release $res >> + gdb_assert {![file exists $lockfile]} "Release lock" >> +} >> diff --git a/gdb/testsuite/lib/gdb-utils.exp b/gdb/testsuite/lib/gdb-utils.exp >> index 35329f3c46e..aa286b74abc 100644 >> --- a/gdb/testsuite/lib/gdb-utils.exp >> +++ b/gdb/testsuite/lib/gdb-utils.exp >> @@ -178,27 +178,39 @@ proc version_compare { l1 op l2 } { >> return 1 >> } >> >> -# Acquire lock file LOCKFILE. Tries forever until the lock file is >> -# successfully created. >> - >> -proc lock_file_acquire {lockfile} { >> +# Acquire lock file LOCKFILE. >> +# If the lock file doesn't exist, create it and return a file handle/file name >> +# pair. >> +# If lock file does exist, with default ON_RETRY try again. A custom ON_RETRY >> +# may do something different like, for instance, using return. >> +# Returns {} on failure. > > It would be nice if there was greater clarity here on the ON_RETRY API. > For example, if ON_RETRY uses return then this will return directly to > the caller proc, leaving lock_file_acquire, this should be called out I > think. > I've now made on_retry a proc, which is easier to describe. >> + >> +proc lock_file_acquire {lockfile {on_retry {after 10}}} { >> verbose -log "acquiring lock file: $::subdir/${::gdb_test_file_name}.exp" >> - while {true} { >> + >> + while {![info exists fh]} { >> try { >> - open $lockfile {WRONLY CREAT EXCL} >> - } on ok {rc} { >> - set msg "locked by $::subdir/${::gdb_test_file_name}.exp" >> - verbose -log "lock file: $msg" >> - # For debugging, put info in the lockfile about who owns >> - # it. >> - puts $rc $msg >> - flush $rc >> - return [list $rc $lockfile] >> - } on error {} { >> - # Ignore and try again. >> + set fh [open $lockfile {WRONLY CREAT EXCL}] >> + } trap "POSIX EEXIST" {} { >> + # Lock in use. Ignore and try again. Propagate all other errors. >> + uplevel 1 $on_retry > > This use of uplevel seems to make the API much more complex, with us > having to handle returns directly from ON_RETRY to the caller proc. Is > this complexity really needed? > I chose uplevel because it allowed me to specify the default behavior as a default value. But agreed, it is complex. I've now made on_retry a proc with default value "", so we have the much simpler: ... + if {$on_retry == ""} { + after 10 + } elseif {![$on_retry]} { + return {} + } ... Thanks for this question, I hope this version is easier to reason about. FWIW, we could even simplify this to something like: ... + if {$on_retry == ""} { + after 10 + } else { + $on_retry + } ... but I think that having on_retry return a value deciding whether to continue retrying or not makes sense from an API perspective. >> } >> - after 10 >> } >> + >> + # Handle the case that ON_RETRY breaks out of the loop. >> + if {![info exists fh]} { >> + return {} > > This code path requires ON_RETRY to use break, but none of the new tests > do this, so this path is untested, would be nice if this case was > covered too. > Ouch, nice catch. Anyway, with the new on_retry as proc approach, this bit is dropped. >> + } > > My AI review tool highlighted this comment as potentially confusing, and > I think in this case I agree. Though the comment does mention 'break' > it is not clear that this path is only encountered when ON_RETRY uses > 'break'. > >> + >> + set msg "locked by $::subdir/${::gdb_test_file_name}.exp" >> + verbose -log "lock file: $msg" >> + >> + # For debugging, put info in the lockfile about who owns >> + # it. >> + puts $fh $msg > > There's an extra space after puts, though I see this was a preexisting > mistake. > I've fixed this in the first patch that touches that line. Thanks, - Tom > > Thanks, > Andrew > >> + flush $fh >> + >> + return [list $fh $lockfile] >> } >> >> # Release a lock file. >> @@ -206,17 +218,20 @@ proc lock_file_acquire {lockfile} { >> proc lock_file_release {info} { >> verbose -log "releasing lock file: $::subdir/${::gdb_test_file_name}.exp" >> >> + set fh [lindex $info 0] >> + set lockfile [lindex $info 1] >> + >> try { >> - fconfigure [lindex $info 0] >> + fconfigure $fh >> } on error {} { >> error "invalid lock" >> } >> >> try { >> - close [lindex $info 0] >> - file delete -force [lindex $info 1] >> - } on error {rc} { >> - error "Error releasing lockfile: '$rc'" >> + close $fh >> + file delete -force $lockfile >> + } on error {err_msg} { >> + error "Error releasing lockfile: '$err_msg'" >> } >> >> return "" >> >> base-commit: aea8b82af17261c88ab4e4b8e623903c49fc81c0 >> -- >> 2.51.0 >