[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux
From: |
Bruno Haible |
Subject: |
Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux |
Date: |
Fri, 02 Dec 2022 01:58:30 +0100 |
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 <sys/xattr.h>
> # include <linux/xattr.h>
> +# include <arpa/inet.h>
> +#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
- [PATCH] Basic support for checking NFSv4 ACLs in Linux, Ondrej Valousek, 2022/12/01
- [PATCH] Basic support for checking NFSv4 ACLs in Linux, Ondrej Valousek, 2022/12/01
- Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux,
Bruno Haible <=
- [PATCH] Basic support for checking NFSv4 ACLs in Linux, Ondrej Valousek, 2022/12/02
- Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux, Bruno Haible, 2022/12/02
- Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux, Bruno Haible, 2022/12/22
- Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux, Paul Eggert, 2022/12/27
- RE: [PATCH] Basic support for checking NFSv4 ACLs in Linux, Ondrej Valousek, 2022/12/28
- Re: [PATCH] Basic support for checking NFSv4 ACLs in Linux, Paul Eggert, 2022/12/28