From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 7SW1LzhRSWihdwkAWB0awg (envelope-from ) for ; Wed, 11 Jun 2025 05:49:44 -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=f0gkq43p; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id AFD491E11C; Wed, 11 Jun 2025 05:49:44 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-10.1 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, RCVD_IN_VALIDITY_RPBL,RCVD_IN_VALIDITY_SAFE autolearn=ham autolearn_force=no version=4.0.1 Received: from server2.sourceware.org (server2.sourceware.org [8.43.85.97]) (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 EF1961E089 for ; Wed, 11 Jun 2025 05:49:43 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 800BC385F00F for ; Wed, 11 Jun 2025 09:49:43 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 800BC385F00F 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=f0gkq43p 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 5CB17385841C for ; Wed, 11 Jun 2025 09:43:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5CB17385841C 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 5CB17385841C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1749635007; cv=none; b=UvVWvrEFpU8URbJg51HJtAPW/H604zVlr9g4f5Vsx/kv6u/xrT+MVXT+rfpXuiZbHsfvm+jkfbz1jeNqZzoQsRIiolnXhaps+UAxKC3V0nh9RHJJtVpZGyTH4pJIVbFXWjjTu/C/zyFsVpy357RzKZu637Jz6kbly1KgAEVxMkc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1749635007; c=relaxed/simple; bh=58qJ5ew5/6Rnb0vQkFKH+HZ3INxj/nuAE4h+EzzgK7g=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=pDbJnVIdATqLf97HKcOygBQEQSl4Le/JRqh8l5gXjrf8vk9FWWiktQoi2RcNuNquw4J/PzftVFNqSxnq9gw8JYfic2SphPV5CRy24j0UgH/xrQel29q9rktrWoHKZHGsm4D76nrue5oWFlWddm+Oc+Dn1FiekaktWAroWJrCcgA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5CB17385841C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1749635007; 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=Ql0MQRZbbTPYAP7NyZIlr+0ZrxyUTJ2FZbylVmGT52A=; b=f0gkq43pya0kx4yCJDoIB2zulUUIE21v25z86ZrvRuDtH6+k7p3g9oEEltBur/BoVIasVP 6i8b3gvp9dLG4I3UxNryI96pp3aPX7++UuzqNpxT/Frt5jKsicaYsn76bsPm1boFhdFd4Y AE8qbKNIIbnR5ZT4l7hdu3iCfut/OLo= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-82-XgsIWPDCOgaSFI1GluN5Bg-1; Wed, 11 Jun 2025 05:43:25 -0400 X-MC-Unique: XgsIWPDCOgaSFI1GluN5Bg-1 X-Mimecast-MFC-AGG-ID: XgsIWPDCOgaSFI1GluN5Bg_1749635004 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-450d50eacafso36247865e9.3 for ; Wed, 11 Jun 2025 02:43:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1749635004; x=1750239804; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Ql0MQRZbbTPYAP7NyZIlr+0ZrxyUTJ2FZbylVmGT52A=; b=PVZZjvgmMUti4rn5WBEoQGCkHDl2qM2vr8ZEYFYB41lO/oEU+pdA48ClmXfL4ZE0ie m5k8szX47d9eGAoZz6tnB4rRpFSFwHwFRDfiWKTMqihXAQwb0dq6t0rVIBmA2poiiHUt QNadAOZUhaS7niBV+vG4XOA2uM7bpUmyWYpB4sNvjBHKutIQ6+HrHze5qRXfh3HzdY4l qisbAF4SAY1Xr6uLLFh08i/POPUTqOOQ+tJfi0DfaxGpCz+KJRwvDFFGT1DjuwJx+DZu lHOkHmt4XtqPQIMA0WIclLq0HoaulSLzHPnQl3nGA5QYS7lVVdZ1nfkADravkn77tiKJ Wcyg== X-Forwarded-Encrypted: i=1; AJvYcCUEZX78NwRWzXSJ1R55OffZN9/u1WWd74ak8V/vTweTUDRnUivJTUiNxC2zHiPeJMFPvaiNgwoLNcx4jw==@sourceware.org X-Gm-Message-State: AOJu0YwxGtuSGxMvorG/b16Z0cpVI9rLkdte2ffQWIYobGLfjQBa5CrU GJWfHDEsVub3z2/6SJ72qaA8fOjXVNKfBW91tpV/GofQyaTgRsC/H7CxaEPFGgcaY53+0uJoBL/ sV/oURBpY5h2FT1+3ubu7A/d+5E1zqv/2Q/oGMwW8FcFcqLk9xP58xp1nseel/YGmhKHMDIg= X-Gm-Gg: ASbGncvF2eENvrz1CnQwBxpJbEWRzu7VsTSm/9IfJKmRBnEyA6DWFKbuFEH/dhVL+Bh 0dGwKWdsCNRlkAlYpWTyK7ec6HF+kOJGjm7fnSZTgQhbVVws9tfwA+SJ4i2qGDDWI+bVKLKk7va OJ2VJ2sXH+ulLpcqwNx2+NyCFjQ7mDWTZiCFdS0t5Q707KJeMbNc62D6iNq0gKxaBic1CJovhqf NGf6fcMueTi2stj7qpq/s5/Nx60QXGhYYJlXML3vFBGUU4BGGSPx27cgPtBX7I8mGcOy4veC5b/ OrSYbg/S+QczUpq9ZfkVKNszb2NuI/k20ElddjkY2tExFYU= X-Received: by 2002:a05:6000:288e:b0:3a4:ebc2:d6ec with SMTP id ffacd0b85a97d-3a5586f26bfmr1900824f8f.14.1749635004139; Wed, 11 Jun 2025 02:43:24 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGcMQBj8PfkvtUC1zIAnx5I6kxK6u9Q9RL4zU/l5uwwZaazC2ep9PH5Dy0Lwo0856xD3lnNhA== X-Received: by 2002:a05:6000:288e:b0:3a4:ebc2:d6ec with SMTP id ffacd0b85a97d-3a5586f26bfmr1900806f8f.14.1749635003667; Wed, 11 Jun 2025 02:43:23 -0700 (PDT) Received: from localhost (75.226.159.143.dyn.plus.net. [143.159.226.75]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-3a5324364f1sm15128297f8f.58.2025.06.11.02.43.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Jun 2025 02:43:23 -0700 (PDT) From: Andrew Burgess To: Fabian Kilger , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2][PR GDB/32956] gdb: implement linux namespace support for fileio_stat In-Reply-To: <06881879-1236-4ad7-b983-c195c1c0a84a@sec.in.tum.de> References: <20250511150113.3163767-1-kilger@sec.in.tum.de> <20250511150113.3163767-2-kilger@sec.in.tum.de> <87a573i48g.fsf@redhat.com> <06881879-1236-4ad7-b983-c195c1c0a84a@sec.in.tum.de> Date: Wed, 11 Jun 2025 10:43:22 +0100 Message-ID: <8734c6ipfp.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: hS3ZNmoto0Ey-Xzd-ySk3glkMWM-gTBCeVmGlcowqr4_1749635004 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 Fabian Kilger writes: > Thanks for reviewing, I'll be including these in v2. I have questions > regarding some style recommendations, where I feel like I'd be breaking > the style of the remaining file. > > On 5/23/25 20:14, Andrew Burgess wrote: >> Fabian Kilger writes: >> >> Thanks for working on this. This mostly looks good, I have just a few >> style issues that I think should be fixed, but it's all pretty minor. >> >> >>> The new algorithm to look for a build-id-based debug file >>> (introduced by commit 22836ca88591ac7efacf06d5b6db191763fd8aba) >>> makes use of fileio_stat. As fileio_stat was not supported by >>> linux-namespace.c, all stat calls would be performed on the host >>> and not inside the namespace >>> >>> --- >>> gdb/linux-nat.c | 14 ++++++++ >>> gdb/linux-nat.h | 3 ++ >>> gdb/nat/linux-namespaces.c | 71 ++++++++++++++++++++++++++++++++++++++ >>> gdb/nat/linux-namespaces.h | 6 ++++ >>> 4 files changed, 94 insertions(+) >>> >>> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c >>> index 3f252370c7b..478a7977c4d 100644 >>> --- a/gdb/linux-nat.c >>> +++ b/gdb/linux-nat.c >>> @@ -4585,6 +4585,20 @@ linux_nat_target::fileio_open (struct inferior *inf, const char *filename, >>> return fd; >>> } >>> >>> +/* Implementation of to_fileio_stat. */ >>> +int >> >> Add an empty line after the comment please to match the other functions >> in this file. >> >>> +linux_nat_target::fileio_stat (struct inferior *inf, const char *filename, >>> + struct stat *sb, fileio_error *target_errno) >>> +{ >>> + int r = linux_mntns_stat (linux_nat_fileio_pid_of (inf), >>> + filename, sb); >> >> There's no need to wrap this argument list. >> > > I've tried copying the style of the other functions in the file. All > fileio_X functions wrap the argument list after linux_nat_fileio_pid_of. > Specifically also fileio_unlink, which has even less arguments. As such, > I feel like not wrapping would break with the style of the other > functions. Should I still not wrap it? My understanding is that the "correct" style takes precedent over matching "incorrect" style in a file. The hope is that, over time, as people touch the code, instances of incorrect style will slowly be fixed. But if we keep adding more incorrect style to match existing code, then things will never get better. Worse, due to copy&paste, the more incorrectly styled code we have, the more the incorrect styling spreads from file to file. > >>> + >>> + if (r == -1) >>> + *target_errno = host_to_fileio_error (errno); >>> + >>> + return r; >>> +} >>> + >>> /* Implementation of to_fileio_readlink. */ >>> >>> std::optional >>> diff --git a/gdb/linux-nat.h b/gdb/linux-nat.h >>> index b630b858e34..42d1ec142b3 100644 >>> --- a/gdb/linux-nat.h >>> +++ b/gdb/linux-nat.h >>> @@ -108,6 +108,9 @@ class linux_nat_target : public inf_ptrace_target >>> const char *filename, >>> fileio_error *target_errno) override; >>> >>> + int fileio_stat (struct inferior *inf, const char *filename, >>> + struct stat *sb, fileio_error *target_errno) override; >>> + >>> int fileio_unlink (struct inferior *inf, >>> const char *filename, >>> fileio_error *target_errno) override; >>> diff --git a/gdb/nat/linux-namespaces.c b/gdb/nat/linux-namespaces.c >>> index 19a05eec905..aa74e9df950 100644 >>> --- a/gdb/nat/linux-namespaces.c >>> +++ b/gdb/nat/linux-namespaces.c >>> @@ -233,6 +233,12 @@ enum mnsh_msg_type >>> MNSH_RET_INT. */ >>> MNSH_REQ_SETNS, >>> >>> + /* A request that the helper call stat. The single >>> + argument (the filename) should be passed in BUF, and >>> + should include a terminating NUL character. The helper >>> + should respond with a MNSH_RET_INTSTR. */ >>> + MNSH_REQ_STAT, >>> + >>> /* A request that the helper call open. Arguments should >>> be passed in BUF, INT1 and INT2. The filename (in BUF) >>> should include a terminating NUL character. The helper >>> @@ -283,6 +289,10 @@ mnsh_debug_print_message (enum mnsh_msg_type type, >>> debug_printf ("ERROR"); >>> break; >>> >>> + case MNSH_REQ_STAT: >>> + debug_printf ("STAT"); >>> + break; >>> + >>> case MNSH_REQ_SETNS: >>> debug_printf ("SETNS"); >>> break; >>> @@ -514,6 +524,20 @@ mnsh_handle_setns (int sock, int fd, int nstype) >>> return mnsh_return_int (sock, result, errno); >>> } >>> >>> + >>> +/* Handle a MNSH_REQ_STAT message. Must be async-signal-safe. */ >>> + >>> +static ssize_t >>> +mnsh_handle_stat(int sock, const char *filename) >>> +{ >>> + struct stat sb; >>> + int stat_ok = stat(filename, &sb); >> >> Space needed after 'stat'. >> >>> + >>> + return mnsh_return_intstr(sock, stat_ok, &sb, >>> + stat_ok == -1 ? 0 : sizeof (sb), >>> + errno); >> >> Space needed after 'mnsh_return_intstr'. >> >>> +} >>> + >>> /* Handle a MNSH_REQ_OPEN message. Must be async-signal-safe. */ >>> >>> static ssize_t >>> @@ -574,6 +598,11 @@ mnsh_main (int sock) >>> response = mnsh_handle_setns (sock, fd, int1); >>> break; >>> >>> + case MNSH_REQ_STAT: >>> + if (size > 0 && buf[size - 1] == '\0') >>> + response = mnsh_handle_stat(sock, buf); >> >> Space needed after 'mnsh_handle_stat' >> >>> + break; >>> + >>> case MNSH_REQ_OPEN: >>> if (size > 0 && buf[size - 1] == '\0') >>> response = mnsh_handle_open (sock, buf, int1, int2); >>> @@ -765,6 +794,10 @@ mnsh_maybe_mourn_peer (void) >>> mnsh_send_message (helper->sock, MNSH_REQ_OPEN, -1, flags, mode, \ >>> filename, strlen (filename) + 1) >>> >>> +#define mnsh_send_stat(helper, filename) \ >>> + mnsh_send_message (helper->sock, MNSH_REQ_STAT, -1, 0, 0, \ >>> + filename, strlen (filename) + 1) >>> + >>> #define mnsh_send_unlink(helper, filename) \ >>> mnsh_send_message (helper->sock, MNSH_REQ_UNLINK, -1, 0, 0, \ >>> filename, strlen (filename) + 1) >>> @@ -945,6 +978,44 @@ linux_mntns_access_fs (pid_t pid) >>> return MNSH_FS_HELPER; >>> } >>> >>> + >>> +/* See nat/linux-namespaces.h. */ >>> +int >> >> Move the empty line from before the comment to after the comment please. >> >>> +linux_mntns_stat (pid_t pid, const char *filename, >>> + struct stat *sb) >>> +{ >>> + enum mnsh_fs_code access = linux_mntns_access_fs (pid); >>> + struct linux_mnsh *helper; >>> + int stat_ok, error; >>> + ssize_t size; >> >> The declarations should be moved inline below. > > Here I've also copied the style of the remaining functions: All > functions in that file declare the variables at the beginning in the > exact same fashion. See linux_mntns_open_cloexec right below it, for > example. As above. Much of this code originated from when GDB was a C project, at which time the GDB style was that everything should be declared at the start of the scope, even when C itself no longer required that. Shortly after the switch to C++ the style changed so that things should be declared as close to the first use as possible. You're right to check these things. And it can be weird to go against the file's existing style, but hopefully, over time, GDB will become more consistent. Thanks, Andrew