Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4db19e2a-40eb-dce9-0094-f6b1e479a95b@redhat.com>
Date:   Tue, 20 Sep 2022 14:48:45 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.0
Subject: Re: [PATCH v2 1/2] nfs4_setfacl: add a specific option for indexes
Content-Language: en-US
To:     Pierguido Lambri <plambri@redhat.com>,
        Linux NFS Mailing list <linux-nfs@vger.kernel.org>
References: <20220914173115.296058-1-plambri@redhat.com>
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <20220914173115.296058-1-plambri@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/14/22 1:31 PM, Pierguido Lambri wrote:
> nfs4_setfacl had the possibility to use an optional index
> to add/remove an ACL entry.
> This was causing some confusion as numeric files could be interpreted
> as indexes.
> This change adds an extra command line option '-i' to specifically
> handle the indexes.
> The index can be used only with certain operations (add and remove).
> The new syntax, when using indexes, would be:
> 
> ~]# nfs4_setfacl -i 3 -a A::101:rxtncy file123
> 
> Signed-off-by: Pierguido Lambri <plambri@redhat.com>
Committed (tag: nfs4-acl-tools-0.4.1-rc3)

steved.
> ---
>   nfs4_setfacl/nfs4_setfacl.c | 60 +++++++++++++++++++++++--------------
>   1 file changed, 38 insertions(+), 22 deletions(-)
> 
> diff --git a/nfs4_setfacl/nfs4_setfacl.c b/nfs4_setfacl/nfs4_setfacl.c
> index e581608..d10e073 100644
> --- a/nfs4_setfacl/nfs4_setfacl.c
> +++ b/nfs4_setfacl/nfs4_setfacl.c
> @@ -143,7 +143,7 @@ int main(int argc, char **argv)
>   	int opt, err = 1;
>   	int numpaths = 0, curpath = 0;
>   	char *tmp, **paths = NULL, *path = NULL, *spec_file = NULL;
> -	FILE *s_fp = NULL;
> +	FILE *s_fp, *fd = NULL;
>   
>   	if (!strcmp(basename(argv[0]), "nfs4_editfacl")) {
>   		action = EDIT_ACTION;
> @@ -155,7 +155,7 @@ int main(int argc, char **argv)
>   		return err;
>   	}
>   
> -	while ((opt = getopt_long(argc, argv, "-:a:A:s:S:x:X:m:ethvHRPL", long_options, NULL)) != -1) {
> +	while ((opt = getopt_long(argc, argv, "-:a:A:i:s:S:x::X:m:ethvHRPL", long_options, NULL)) != -1) {
>   		switch (opt) {
>   			case 'a':
>   				mod_string = optarg;
> @@ -165,21 +165,14 @@ int main(int argc, char **argv)
>   			add:
>   				assert_wu_wei(action);
>   				action = INSERT_ACTION;
> -
> -				/* run along if no more args (defaults to ace_index 1 == prepend) */
> -				if (optind == argc)
> -					break;
> -				ace_index = strtoul_reals(argv[optind++], 10);
> -				if (ace_index == ULONG_MAX) {
> -					/* oops it wasn't an ace_index; reset */
> -					optind--;
> -					ace_index = -1;
> -				} else if (ace_index == 0) {
> -					fprintf(stderr, "Sorry, valid indices start at '1'.\n");
> -					goto out;
> +				break;
> +			case 'i':
> +				ace_index = strtoul_reals(optarg, 10);
> +				if (ace_index == 0) {
> +                                    fprintf(stderr, "Sorry, valid indices start at '1'.\n");
> +                                    goto out;
>   				}
>   				break;
> -
>   			case 's':
>   				mod_string = optarg;
>   				goto set;
> @@ -191,9 +184,14 @@ int main(int argc, char **argv)
>   				break;
>   
>   			case 'x':
> -				ace_index = strtoul_reals(optarg, 10);
> -				if(ace_index == ULONG_MAX)
> -					mod_string = optarg;
> +				/* make sure we handle the argument even if
> +				 * it doesn't immediately follow the option
> +				 */
> +				if (optarg == NULL && optind < argc && argv[optind][0] != '-')
> +				{
> +					optarg = argv[optind++];
> +				}
> +				mod_string = optarg;
>   				goto remove;
>   			case 'X':
>   				spec_file = optarg;
> @@ -255,6 +253,9 @@ int main(int argc, char **argv)
>   					case 'A':
>   						fprintf(stderr, "Sorry, -a requires an 'acl_spec', whilst -A requires a 'spec_file'.\n");
>   						goto out;
> +					case 'i':
> +						fprintf(stderr, "Sorry, -i requires an index (numerical)\n");
> +						goto out;
>   					case 's':
>   						fprintf(stderr, "Sorry, -s requires an 'acl_spec'.\n");
>   						goto out;
> @@ -297,7 +298,21 @@ int main(int argc, char **argv)
>   	if (action == NO_ACTION) {
>   		fprintf(stderr, "No action specified.\n");
>   		goto out;
> -	} else if (numpaths < 1) {
> +	} else if (action != INSERT_ACTION && action != REMOVE_ACTION && ace_index >= 0) {
> +		fprintf(stderr, "Index can be used only with add or remove.\n");
> +		goto out;
> +	} else if (numpaths <= 0 && ace_index >= 0 && mod_string)
> +	{
> +		/* Make sure the argument is a file */
> +		if (!(fd = fopen(mod_string, "r"))) {
> +			fprintf(stderr, "No path(s) specified.\n");
> +			goto out;
> +		} else
> +			fclose(fd);
> +		paths = malloc(sizeof(char *) * (argc - optind + 1));
> +		paths[numpaths++] = mod_string;
> +	} else if (numpaths < 1)
> +	{
>   		fprintf(stderr, "No path(s) specified.\n");
>   		goto out;
>   	}
> @@ -609,9 +624,10 @@ static void __usage(const char *name, int is_ef)
>   	"%s %s -- manipulate NFSv4 file/directory access control lists\n"
>   	"Usage: %s [OPTIONS] COMMAND file ...\n"
>   	" .. where COMMAND is one of:\n"
> -	"   -a acl_spec [index]	 add ACL entries in acl_spec at index (DEFAULT: 1)\n"
> -	"   -A file [index]	 read ACL entries to add from file\n"
> -	"   -x acl_spec | index	 remove ACL entries or entry-at-index from ACL\n"
> +	"   -a acl_spec		 add ACL entries in acl_spec at defaul index (DEFAULT: 1)\n"
> +	"   -A file 		 read ACL entries to add from file\n"
> +	"   -i index 		 use the entry-at-index from ACL (only for add and remove)\n"
> +	"   -x acl_speci 	 remove ACL entries\n"
>   	"   -X file  		 read ACL entries to remove from file\n"
>   	"   -s acl_spec		 set ACL to acl_spec (replaces existing ACL)\n"
>   	"   -S file		 read ACL entries to set from file\n"