From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id yWFmBsibumpBpRIAWB0awg (envelope-from ) for ; Mon, 28 Sep 2026 12:54:32 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SGtGBuUi; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 10F7D1E06B; Mon, 28 Sep 2026 12:54:32 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,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 BDF351E01F for ; Mon, 28 Sep 2026 12:54:30 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 8D1914B9DB76 for ; Mon, 28 Sep 2026 16:54:29 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8D1914B9DB76 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SGtGBuUi Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 506EC4BA23F4 for ; Mon, 28 Sep 2026 16:54:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 506EC4BA23F4 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 506EC4BA23F4 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790614442; cv=none; b=uzRwzQ5th/95PSKYHkcNbgzRbEPzClBsaJrwsV8beOSmp+8ptBUzM2WLwp179x216wk3rv+Y9v0Ss5L5YjF6LX+CFA92VMwDJZvCZQG4qZTQkS6V+wVPwbI1xrZOs6BdncVKE5lmhEfINCpbDD7LCnmtb5o7kst+hDW5cxAz7lU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1790614442; c=relaxed/simple; bh=8Sy4c+JKAOGXn3e9no5Tx6v1cWuPcN/Hsf8DGqxmc5k=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=g6tUs1ZOA6fqlnis5aF4xipkV+wKU9GlAdeGzuoXTQUWP9p6o68OgZtFdF43d65/4Xav/DECAL2tuAf3DZUxph2W+dOcN51ZeiumHds3a2ciUfFVwj8HrdDZ7W8MyPAlgBpONMz0PMbXRUq62bmTWyFdEmw0MJZN7RX2pf6QqGw= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SGtGBuUi DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 506EC4BA23F4 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1790614441; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=mBKhFyWAbQ+ptkPZKV7KcVbWZN7pWRg/8MDnXAWpWHQ=; b=SGtGBuUiZijT+CT97Ns55Vzj6deNxjK7pYSA2s6wN8o/nhm3KmRqhwsegb57dSmewm9ddy o3qDJh/j1qxDclOh2t6meifSXxXFR2GoeV3J+utUupyOWxZ8q2y2fE4mK3HUJyelPafTxl W1C+s7a5avmiq4S2RLjcnamxc7uzxpI= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-272-O9_u--4zMfC5WuNrQL6pxQ-1; Mon, 28 Sep 2026 12:54:00 -0400 X-MC-Unique: O9_u--4zMfC5WuNrQL6pxQ-1 X-Mimecast-MFC-AGG-ID: O9_u--4zMfC5WuNrQL6pxQ_1790614439 Received: by mail-wm1-f72.google.com with SMTP id 5b1f17b1804b1-49fcc575709so32041825e9.3 for ; Mon, 28 Sep 2026 09:54:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20260707; t=1790614439; x=1791219239; h=content-type:mime-version:message-id:date:references:in-reply-to :subject:to:from:x-gm-gg:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to:content-type; bh=mBKhFyWAbQ+ptkPZKV7KcVbWZN7pWRg/8MDnXAWpWHQ=; b=TsIVEXQQjZeARFKuntR0irp+9ObfGzgTfP04+1KklV6vnrcvbzmo6yflvkupTAwVXa SqyNzZmWF+q0HRG8ieSf8f67YubaJDsMloP5yptzwwN/ryC15TvwNtmb15QR5QGFfxyY mNh3FMGtV7R+dRFri+TGx2m7VqAptT0FAoEH16ktoys+UOvcx1D7rjXgKDN6eVrXL5zf 0p1+nO2DlXogDEiC2eNSqehc02ryzRtBeTcqNXpvBnQubm0xMKb/c03/xHNIwajmiSgk 3WVpkS93QmWFdXx3pwUrSabLnttBCzEbWJWDAzxRk+L3WN+pBGdCLrw8yox+gTCPV/EK /uIg== X-Forwarded-Encrypted: i=1; AKwUvByQ6+NASHVOdlHT9miNLoFUJpTG9+IUwpTh0udYwKU9gMhsuJmkm+0gP4uMlu8XPlX6YcE2LNok5kXMMw==@sourceware.org X-Gm-Message-State: AFuF++mnI/dAE/lXA37RpCp/1b9GpchpMhDLpE8gOwthrANVqAOSBkjk hzOo4Ey5c7FVPlE6v6uwHE21TE7fC7XNH6EViQGTjr6FuAVa72Se6vXPrDiHglO3MNmR0flESwR eG7AiPCedAqMVqqLPIM7/bsaOgjYAYj6roSGtRnc/AXPwPog4AkmjaHoElKpX3dQwwyjuL2g= X-Gm-Gg: AYBFou229Sz6QZraLT8cDu80t0vCfeIPZvgSsQq/oXLHukvPJeznel8dLtU6qKGUqQV IFoCNR2q3jptHmy9re12n/1lgrUNIOjN/hQ4bm1Ee/GMgzxZ6cGi7o/PNa9jBadD84ttnCPlCgF +kNmLPfr2oyLhKWeU9MnvR0RxU31kgFIrMhNvn/Np/8y1djjJjNOWFxOlt3Krqouzts6vcFnGKc s1lcqRukDDKQhY15Mvv6VJbuFBmkt24k2yomy+wMZwgTwdo0DvMdj6KPJtHUW+NBULJH7Kf/Vs3 u+Iv+UtB6Hev38ekt6vFp3zqUCWIoc6Fd5gy8ch/yyUOo/stSa3I49SBbwJUpbzXPQxG X-Received: by 2002:a05:600c:3b9c:b0:49c:fed6:cd3f with SMTP id 5b1f17b1804b1-49fe66dfdf8mr303160465e9.23.1790614439026; Mon, 28 Sep 2026 09:53:59 -0700 (PDT) X-Received: by 2002:a05:600c:3b9c:b0:49c:fed6:cd3f with SMTP id 5b1f17b1804b1-49fe66dfdf8mr303160195e9.23.1790614438529; Mon, 28 Sep 2026 09:53:58 -0700 (PDT) Received: from localhost ([213.31.44.29]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4a00cf9dd27sm5582005e9.2.2026.09.28.09.53.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Sep 2026 09:53:58 -0700 (PDT) From: Andrew Burgess To: Tom de Vries , gdb-patches@sourceware.org Subject: Re: [PATCH] [gdb/testsuite] Refactor lock_file_{acquire,release} In-Reply-To: <20260925220601.2021240-1-tdevries@suse.de> References: <20260925220601.2021240-1-tdevries@suse.de> Date: Mon, 28 Sep 2026 17:53:57 +0100 Message-ID: <87eced1fmi.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: GbnNmWYOHddET3BEMcvwIu5aC6HBpdtWrpsK6uZgDIk_1790614439 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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 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? > > 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. > + > +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? > } > - 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. > + } 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. 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