netdev - Re: [PATCH iproute2-next 2/3] devlink: print missing params even if an unknown one is present

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZoakvNeIz2WK33fj@localhost.localdomain>
Date: Thu, 4 Jul 2024 15:33:48 +0200
From: Michal Kubiak <michal.kubiak@...el.com>
To: Przemek Kitszel <przemyslaw.kitszel@...el.com>
CC: Stephen Hemminger <stephen@...workplumber.org>, David Ahern
	<dsahern@...nel.org>, <netdev@...r.kernel.org>, Tony Nguyen
	<anthony.l.nguyen@...el.com>, Lukasz Czapnik <lukasz.czapnik@...el.com>,
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Subject: Re: [PATCH iproute2-next 2/3] devlink: print missing params even if
 an unknown one is present

On Wed, Jul 03, 2024 at 03:15:20PM +0200, Przemek Kitszel wrote:
> Print all of the missing parameters, also in the presence of unknown ones.
> 
> Take for example a correct command:
>     $ devlink resource set pci/0000:01:00.0 path /kvd/linear size 98304
> And remove the "size" keyword:
>     $ devlink resource set pci/0000:01:00.0 path /kvd/linear 98304
> That yields output:
>     Resource size expected.
>     Unknown option "98304"
> 
> Prior to the patch only the last line of output was present. And if user
> would forgot also the "path" keyword, there will be additional line:
>     Resource path expected.
> in the stderr.
> 
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
> ---
>  devlink/devlink.c | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/devlink/devlink.c b/devlink/devlink.c
> index 57bcc9658bdb..9907712e3ad0 100644
> --- a/devlink/devlink.c
> +++ b/devlink/devlink.c
> @@ -1680,26 +1680,28 @@ static const struct dl_args_metadata dl_args_required[] = {
>  static int dl_args_finding_required_validate(uint64_t o_required,
>  					     uint64_t o_found)
>  {
> -	uint64_t o_flag;
> -	int i;
> +	uint64_t o_flag, o_missing = 0;
> +	int i, err = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(dl_args_required); i++) {
>  		o_flag = dl_args_required[i].o_flag;
>  		if ((o_required & o_flag) && !(o_found & o_flag)) {
> +			o_missing |= o_flag;
>  			pr_err("%s\n", dl_args_required[i].err_msg);
> -			return -ENOENT;
> +			err = -ENOENT;
>  		}
>  	}
> -	if (o_required & ~o_found) {
> +	if (o_required & ~(o_found | o_missing)) {
>  		pr_err("BUG: unknown argument required but not found\n");
>  		return -EINVAL;
>  	}
> -	return 0;
> +	return err;
>  }
>  
>  static int dl_argv_parse(struct dl *dl, uint64_t o_required,
>  			 uint64_t o_optional)
>  {
> +	const char *unknown_option = NULL;
>  	struct dl_opts *opts = &dl->opts;
>  	uint64_t o_all = o_required | o_optional;
>  	char *str = dl_argv_next(dl);
> @@ -2313,8 +2315,9 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
>  			o_found |= DL_OPT_PORT_FN_MAX_IO_EQS;
>  
>  		} else {
> -			pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> -			return -EINVAL;
> +			if (!unknown_option)
> +				unknown_option = dl_argv(dl);
> +			dl_arg_inc(dl);
>  		}
>  	}
>  
> @@ -2325,7 +2328,15 @@ static int dl_argv_parse(struct dl *dl, uint64_t o_required,
>  
>  	opts->present = o_found;
>  
> -	return dl_args_finding_required_validate(o_required, o_found);
> +	err = dl_args_finding_required_validate(o_required, o_found);
> +
> +	if (unknown_option) {
> +		pr_err("Unknown option \"%s\"\n", unknown_option);
> +		if (!err)
> +			return -EINVAL;
> +	}
> +
> +	return err;
>  }
>  
>  static int dl_argv_dry_parse(struct dl *dl, uint64_t o_required,
> -- 
> 2.39.3
> 
> 

Thanks,
Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>