From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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_MED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 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 8678C1F601 for ; Fri, 2 Dec 2022 00:59:04 +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.b="Bl2fZxeO"; dkim-atps=neutral Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1p0uOA-0000ql-4Q; Thu, 01 Dec 2022 19:58:50 -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 1p0uO6-0000qS-Fc for bug-gnulib@gnu.org; Thu, 01 Dec 2022 19:58:47 -0500 Received: from mo4-p00-ob.smtp.rzone.de ([85.215.255.23]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0uO4-0006HZ-1J for bug-gnulib@gnu.org; Thu, 01 Dec 2022 19:58:46 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1669942711; s=strato-dkim-0002; d=clisp.org; h=References:In-Reply-To:Message-ID:Date:Subject:To:From:Cc:Date:From: Subject:Sender; bh=ssce2VMYB1mgpjiHR690uLFSq5WIsYUkvVQxczcdERg=; b=Bl2fZxeOSNcvFrRwkXdErV921msWo/i1Af4NoVzh68Of5YzqzvW7Rz1eIItR8t+f+/ qql7Oz4X/cut3sHqnwXiSEpSFQohnpZlUJVbRGnKO59K6uiJICMSjfx7zWBa4Jiy1SNr WBPk1m5S8jbaQZ6Qg7YisE7hAJ0ZUO6GInC3CTTUBAzMLTlm4eE3+O9Sm9xjAzGXAEP/ yIQxAI7K8DrfV7aGePSJLN8CQ19BySVwx5/PxPWU8uu0Spt5RvE0cxdfdwS1r9l2a2s5 IAbPq7MvXHcepwwP/fIpV8xTw+GQVS28Ro/kpaiw4W7xwT8Dq1wHPai7taVBj6ot99AK XIbA== Authentication-Results: strato.com; dkim=none X-RZG-AUTH: ":Ln4Re0+Ic/6oZXR1YgKryK8brlshOcZlIWs+iCP5vnk6shH0WWb0LN8XZoH94zq68+3cfpPE2faGIYKovsaNk9hwvYMVtbw64A==" X-RZG-CLASS-ID: mo00 Received: from nimes.localnet by smtp.strato.de (RZmta 48.2.1 AUTH) with ESMTPSA id v9c7e6yB20wUP6R (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Fri, 2 Dec 2022 01:58:30 +0100 (CET) From: Bruno Haible To: bug-gnulib@gnu.org, Ondrej Valousek Subject: Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux Date: Fri, 02 Dec 2022 01:58:30 +0100 Message-ID: <2513994.otXNkdZ6W1@nimes> In-Reply-To: <20221201142448.1419071-1-ondrej.valousek.xm@renesas.com> References: <20221201142448.1419071-1-ondrej.valousek.xm@renesas.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Received-SPF: none client-ip=85.215.255.23; 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, Still some formatting issues. But now, with formatting nearly right, I'm able to spot other things too. > +# define ACE4_WHO_OWNER "OWNER@" > +# define ACE4_WHO_GROUP "GROUP@" > +# define ACE4_WHO_EVERYONE "EVERYONE@" > + > +# define ACE4_ACCESS_ALLOWED_ACE_TYPE 0 > +# define ACE4_ACCESS_DENIED_ACE_TYPE 1 > + > +/* ACE flag values */ > +# define ACE4_IDENTIFIER_GROUP 0x00000040 > +# define ROUNDUP(x, y) (((x) + (y) - 1) & - (y)) These are all inside a '# if' / '# endif' pair, so the preprocessor statement indentation level is 2. Please add one more space between '#' and 'define' here. > +int > +acl_nfs4_nontrivial (char *xattr, int len) > +{ > + int ret = 0, > + wholen, > + bufs = len; > + uint32_t flag, > + type, > + num_aces = ntohl (*((uint32_t*)(xattr))); /* Grab the number of aces in the acl */ > + char *bufp = xattr; > + int num_a_aces = 0, > + num_d_aces = 0; > + > + ret = 0; 'ret' is already initialized to 0 above. One of the two initializations is redundant. Hmm... is the variable 'ret' used at all? You should routinely compile with "gcc -Wall"; this catches unused variables, usually. > + bufp += 4; /* sizeof(uint32_t); */ > + bufs -= 4; > + > + for (uint32_t ace_n = 0; num_aces > ace_n ; ace_n++) > + { > + int d_ptr; There are more variables which can be moved into the 'for' loop. Generally, moving variables into the smallest scope that they need provides for a better readability of the code. If I'm not mistaken, here, it applies to the variables wholen, flag, type. > + > + /* Get the acl type */ > + if (bufs <= 0) > + return -1; > + > + type = ntohl (*((uint32_t*)bufp)); > + > + bufp += 4; > + bufs -= 4; > + if (bufs <= 0) > + return -1; > + > + flag = ntohl (*((uint32_t*)bufp)); > + /* As per RFC 7530, the flag should be 0, but we are just generous to Netapp > + * and also accept the Group flag > + */ > + if (flag & ~ACE4_IDENTIFIER_GROUP) > + return 1; > + > + /* we skip mask - > + * it's too risky to test it and it does not seem to be actually needed */ > + bufp += 2*4; > + bufs -= 2*4; > + > + if (bufs <= 0) > + return -1; > + > + wholen = ntohl (*((uint32_t*)bufp)); > + > + bufp += 4; > + bufs -= 4; > + > + /* Get the who string */ > + if (bufs <= 0) > + return -1; > + > + /* for trivial ACL, we expect max 5 (typically 3) ACES, 3 Allow, 2 deny */ > + if (((strncmp (bufp,ACE4_WHO_OWNER,wholen) == 0) || > + (strncmp (bufp,ACE4_WHO_GROUP,wholen) == 0)) && > + wholen == 6) Please move the || and && operators to the next line: if (((strncmp (bufp,ACE4_WHO_OWNER,wholen) == 0) || (strncmp (bufp,ACE4_WHO_GROUP,wholen) == 0)) && wholen == 6) Add add a space after each comma in an argument list. > + { > + if (type == ACE4_ACCESS_ALLOWED_ACE_TYPE) > + num_a_aces++; > + if (type == ACE4_ACCESS_DENIED_ACE_TYPE) > + num_d_aces++; > + } else Please put the closing brace and the 'else' on separate lines. > + if ((strncmp (bufp,ACE4_WHO_EVERYONE,wholen) == 0) > + && (type == ACE4_ACCESS_ALLOWED_ACE_TYPE) > + && (wholen == 9)) > + num_a_aces++; > + else > + return 1; > + > + d_ptr = ROUNDUP (wholen,4); > + bufp += d_ptr; > + bufs -= d_ptr; > + > + /* Make sure we aren't outside our domain */ > + if (bufs < 0) > + return -1; > + > + } > + return (num_a_aces <= 3) && (num_d_aces <= 2) > + && (num_a_aces + num_d_aces == num_aces) ? 0 : 1; You can simplify this: Knowing that boolean expressions have the value 1 for true and 0 for false in C, you can write it as return !((num_a_aces <= 3) && (num_d_aces <= 2) && (num_a_aces + num_d_aces == num_aces)); which is smaller and simpler. > diff --git a/lib/acl-internal.h b/lib/acl-internal.h > index 94553fab2..b674c7702 100644 > --- a/lib/acl-internal.h > +++ b/lib/acl-internal.h > @@ -146,6 +146,9 @@ rpl_acl_set_fd (int fd, acl_t acl) > # define acl_entries rpl_acl_entries > extern int acl_entries (acl_t); > # endif > +/* Return 1 if given ACL in XDR format is non-trivial > + * Return 0 if it is trivial */ > +extern int acl_nfs4_nontrivial (char *,int); Please add a space after the comma. > diff --git a/lib/file-has-acl.c b/lib/file-has-acl.c > index e02f0626a..82830ba4a 100644 > --- a/lib/file-has-acl.c > +++ b/lib/file-has-acl.c > @@ -32,6 +32,11 @@ > #if GETXATTR_WITH_POSIX_ACLS > # include > # include > +# include > +#ifndef XATTR_NAME_NFSV4_ACL > +# define XATTR_NAME_NFSV4_ACL "system.nfs4_acl" > +#endif Please indent the last three of these lines by one more space, after the '#'. Bruno