From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (gnu.wildebeest.org [45.83.234.184]) by sourceware.org (Postfix) with ESMTPS id 9FD6B3858C2C for ; Wed, 13 Apr 2022 14:55:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9FD6B3858C2C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from tarox.wildebeest.org (83-87-18-245.cable.dynamic.v4.ziggo.nl [83.87.18.245]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id E03B030003C6; Wed, 13 Apr 2022 16:55:32 +0200 (CEST) Received: by tarox.wildebeest.org (Postfix, from userid 1000) id 1F269413CD15; Wed, 13 Apr 2022 16:55:32 +0200 (CEST) Message-ID: Subject: Re: [PATCH] PR29022: 000-permissions files cause problems for backups From: Mark Wielaard To: Aaron Merey , elfutils-devel@sourceware.org Date: Wed, 13 Apr 2022 16:55:31 +0200 In-Reply-To: <20220408233711.62613-1-amerey@redhat.com> References: <20220406003407.292374-1-amerey@redhat.com> <20220408233711.62613-1-amerey@redhat.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Mailer: Evolution 3.28.5 (3.28.5-10.el7) Mime-Version: 1.0 X-Spam-Status: No, score=-10.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 13 Apr 2022 14:55:38 -0000 Hi Aaron, On Fri, 2022-04-08 at 19:37 -0400, Aaron Merey via Elfutils-devel wrote: > I've revised this patch so that the negative-hit file's mtime is used > to calculate time since last download attempt instead of the cache_miss_s > file. I've also added a check for old 000-permission files so that they > are unlinked immediately if found. >=20 > Aaron >=20 > --- > PR29022: 000-permissions files cause problems for backups >=20 > 000-permission files currently used for negative caching can cause > permission problems for some backup software and disk usage checkers. >=20 > Fix this by using empty files to for negative caching instead. >=20 > https://sourceware.org/bugzilla/show_bug.cgi?id=3D29022 >=20 > Signed-off-by: Aaron Merey Please also mention the fix for the mtime/stat/cache_miss_s issue in the commit message. > --- > debuginfod/ChangeLog | 5 + > debuginfod/debuginfod-client.c | 94 ++++++++++++----- > -- > tests/ChangeLog | 5 + > tests/Makefile.am | 4 +- > tests/run-debuginfod-federation-link.sh | 4 +- > tests/run-debuginfod-federation-metrics.sh | 4 +- > tests/run-debuginfod-federation-sqlite.sh | 4 +- > ...on.sh =3D> run-debuginfod-negative-cache.sh} | 6 +- > tests/run-debuginfod-tmp-home.sh | 2 +- > 9 files changed, 81 insertions(+), 47 deletions(-) > rename tests/{run-debuginfod-000-permission.sh =3D> run-debuginfod- > negative-cache.sh} (92%) >=20 > diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog > index 578d951d..6c2edbdf 100644 > --- a/debuginfod/ChangeLog > +++ b/debuginfod/ChangeLog > @@ -1,3 +1,8 @@ > +2022-04-05 Aaron Merey > + > + * debuginfod-client.c: Represent negative caching with empty > files > + instead of 000-permission files. A ChangeLog entry really should mention the how things were changed/fixed, not just restate the what, which is what the commit message is for. I admit that I am probably the only person still seeing value in ChangeLog entries, but if we are doing them I would suggest something like: * debuginfod-client.c (debuginfod_query_server): Drop st_mode check. Add st_size > 0 check. Save target_mtime before calling debuginfod_config_cache. unlink target_cache_path on EACCESS. Create target_cache_path with DEFFILEMODE. > @@ -767,42 +767,66 @@ debuginfod_query_server (debuginfod_client *c, > if (rc !=3D 0) > goto out; > =20 > - struct stat st; > - /* Check if the file exists and it's of permission 000; must check > - explicitly rather than trying to open it first (PR28240). */ > - if (stat(target_cache_path, &st) =3D=3D 0 > - && (st.st_mode & 0777) =3D=3D 0) > + /* Check if the target is already in the cache. */ > + int fd =3D open(target_cache_path, O_RDONLY); > + if (fd >=3D 0) > { > - time_t cache_miss; > - > - rc =3D debuginfod_config_cache(cache_miss_path, > cache_miss_default_s, &st); > - if (rc < 0) > - goto out; > + struct stat st; > + if (fstat(fd, &st) !=3D 0) > + { > + close (fd); > + rc =3D -errno; > + goto out; > + } Call close after setting rc =3D -errno. close might also set errno, but we want to make sure to report the original errno that fstat returned (close probably will report the same errno, but might not, it might also set errno to 0 which is super confusing). =20 > - cache_miss =3D (time_t)rc; > - if (time(NULL) - st.st_mtime <=3D cache_miss) > + /* If the file is non-empty, then we are done. */ > + if (st.st_size > 0) > { > - rc =3D -ENOENT; > - goto out; > - } > + if (path !=3D NULL) > + { > + *path =3D strdup(target_cache_path); > + if (*path =3D=3D NULL) > + { > + close (fd); > + rc =3D -errno; > + goto out; > + } Likewise. > diff --git a/tests/ChangeLog b/tests/ChangeLog > index c195f9f7..91ce1ffb 100644 > --- a/tests/ChangeLog > +++ b/tests/ChangeLog > @@ -1,3 +1,8 @@ > +2022-04-05 Aaron Merey > + > + * run-debuginfod-000-permissions.sh: Delete. > + * run-debuginfod-negative-cache.sh: New test. Missing: * Makefile.am (TESTS): Remove run-debuginfod-000-permission.sh and add run-debuginfod-negative-cache.sh. (EXTRA_DIST): Likewise. * run-debuginfod-federation-link.sh: Update comments about negative-hit file. * run-debuginfod-federation-metrics.sh: Likewise. * run-debuginfod-federation-sqlite.sh: Likewise. * run-debuginfod-tmp-home.sh: Likewise. =20 > -if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` !=3D "----= ------" ]; then > - echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is readable" > +if [ `stat -c "%s" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` !=3D 0 ]; = then > + echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is not empty= " > err > fi Changes look good, please commit with the changes suggested above and add Tested-by: Milian Wolff Thanks, Mark