From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS22989 209.51.188.0/24 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.6 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 176211F47C for ; Tue, 3 Jan 2023 16:44:58 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=clisp.org header.i=@clisp.org header.a=rsa-sha256 header.s=strato-dkim-0002 header.b=WpN4BJ5N; dkim-atps=neutral Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1pCkPA-0004Uy-Sg; Tue, 03 Jan 2023 11:44:48 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pCkP9-0004Up-8Y for bug-gnulib@gnu.org; Tue, 03 Jan 2023 11:44:47 -0500 Received: from mo4-p00-ob.smtp.rzone.de ([81.169.146.221]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1pCkP7-000655-2o for bug-gnulib@gnu.org; Tue, 03 Jan 2023 11:44:46 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1672764276; s=strato-dkim-0002; d=clisp.org; h=References:In-Reply-To:Message-ID:Date:Subject:Cc:To:From:Cc:Date: From:Subject:Sender; bh=Z/G+Xjaof2JK0NIUesIQvsHzY3GHpLtQaDETUPv19t4=; b=WpN4BJ5NwrHKDLaADcjqrk+jkD7H8YMAaRWr9/fhfXsEzCETG/ihsOBIXytVe8rO6K yoFa8aC+OJbHSO+eVkdqU9ujRxnO+Mj6KkzKtz3QSGHqKeGtdGSndNMgCVPNMLTWJvew y0qJbM0WFZSqKfF4ZDLC6YA/ENYPb+P11DTTDKMdXpCv+7f7KwrJtKmhNgzqR96UVRDo mRhbaKoQHey/eLvxcr/mooCgef7we4lVhj2/LCwhkqh9/c7e3y6Oc/wse+ErUuQRmE+c dFhPcpp0t3koLapEbBfSMNeBU+Am/bppNcerkyjw6le5OnMHzU7nRdcoyfvEvB3jiE0f s9DQ== Authentication-Results: strato.com; dkim=none X-RZG-AUTH: ":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH0WWb0LN8XZoH94zq68+3cfpOX2vdClfEwFaPfEGVf28qrkcyD5w==" X-RZG-CLASS-ID: mo00 Received: from nimes.localnet by smtp.strato.de (RZmta 48.2.1 AUTH) with ESMTPSA id t05890z03GiZ1K0 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Tue, 3 Jan 2023 17:44:35 +0100 (CET) From: Bruno Haible To: bug-gnulib@gnu.org, Ondrej Valousek Cc: kdudka@redhat.com Subject: Re: [PATCH] Use xattr (Linux) in copy-acl.c Date: Tue, 03 Jan 2023 17:44:35 +0100 Message-ID: <24535951.kmuVQn2iE0@nimes> In-Reply-To: <20230103140816.1121699-1-ondrej.valousek.xm@renesas.com> References: <20230103140816.1121699-1-ondrej.valousek.xm@renesas.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" Received-SPF: none client-ip=81.169.146.221; envelope-from=bruno@clisp.org; helo=mo4-p00-ob.smtp.rzone.de X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_PASS=-0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: bug-gnulib@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gnulib discussion list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Sender: bug-gnulib-bounces+normalperson=yhbt.net@gnu.org Hi Ondrej, > diff --git a/lib/copy-acl.c b/lib/copy-acl.c > index 5fc42b7f6..341d1f605 100644 > --- a/lib/copy-acl.c > +++ b/lib/copy-acl.c > @@ -29,6 +29,20 @@ > #define _(msgid) gettext (msgid) > =20 > =20 > +#if USE_XATTR > + > +# include > + > +static int > +copy_attr_permissions (const char *name, struct error_context *ctx) This function =E2=80=94 like all functions =E2=80=94 deserves a comment bef= ore the function. I would propose: /* Returns 1 if NAME is the name of an extended attribute that is related to permissions, i.e. ACLs. Returns 0 otherwise. */ In view of this description, 'copy_attr_permissions' is a strange name for a function that returns a boolean value. How about renaming it to 'is_permissions_attr' ? > +{ > + int action =3D attr_copy_action (name, ctx); > + return action =3D=3D ATTR_ACTION_PERMISSIONS; Gnulib style: Please indent with spaces, not with tabs. > /* Copy access control lists from one file to another. If SOURCE_DESC is > a valid file descriptor, use file descriptor operations, else use > filename based operations on SRC_NAME. Likewise for DEST_DESC and > @@ -43,7 +57,22 @@ int > copy_acl (const char *src_name, int source_desc, const char *dst_name, > int dest_desc, mode_t mode) > { > - int ret =3D qcopy_acl (src_name, source_desc, dst_name, dest_desc, mod= e); > + int ret; > +#ifdef USE_XATTR > + ret =3D chmod_or_fchmod (dst_name, dest_desc, mode); > + /* Rather than fiddling with acls one by one, we just copy the whole A= CL xattrs=20 > + * (Posix or NFSv4). Of course, that won't address ACLs conversion=20 > + * (i.e. posix <-> nfs4) but we can't do it anyway, so for now, we don= 't care > + */ > + if(ret =3D=3D 0) GNU coding style: Please add a space after 'if'. > + ret =3D source_desc <=3D 0 && dest_desc <=3D 0 > + ? attr_copy_file (src_name, dst_name, copy_attr_permissions, NULL) > + : attr_copy_fd (src_name, source_desc, dst_name, dest_desc, copy_a= ttr_permissions, NULL); > +#else > + /* no XATTR, so we proceed the old dusty way */ > + ret =3D qcopy_acl (src_name, source_desc, dst_name, dest_desc, mode); > +#endif Instead of putting this code into copy_acl, I think qcopy_acl would be the better place. The difference between qcopy_acl and copy_acl is, per definit= ion, that the former is silent (does not write to stdout or stderr), whereas the latter signals errors. > diff --git a/m4/xattr.m4 b/m4/xattr.m4 > new file mode 100644 > index 000000000..5f9248939 > --- /dev/null > +++ b/m4/xattr.m4 > + AC_DEFINE_UNQUOTED([USE_XATTR], [`test $use_xattr !=3D yes; echo $?`], The use of backquotes here is ugly. How about simplifying it by use of a shell variable? E.g. if test $use_xattr =3D yes; then use_xattr_value=3D1 else use_xattr_value=3D0 fi AC_DEFINE_UNQUOTED([USE_XATTR], [$use_xattr_value]) > diff --git a/modules/acl b/modules/acl > index 1a3a14e6c..ca2239823 100644 > --- a/modules/acl > +++ b/modules/acl > @@ -4,6 +4,7 @@ Access control lists of files, with diagnostics. (Unport= able.) > Files: > lib/copy-acl.c > lib/set-acl.c > +m4/xattr.m4 > =20 > Depends-on: > error This change would go into modules/qcopy-acl, not modules/acl. And gl_FUNC_XATTR needs to be invoked from somewhere, otherwise all of m4/xattr.m4 is dead code. Bruno