From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: Alban Crequy <alban.crequy@gmail.com>
Cc: ast@kernel.org, daniel@iogearbox.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, alban@kinvolk.io, iago@kinvolk.io
Subject: Re: [PATCH bpf-next v1 2/7] tools: bpftool: fix error message on invalid argument count
Date: Wed, 20 Mar 2019 14:02:05 -0700 [thread overview]
Message-ID: <20190320140205.54f87bb9@cakuba.netronome.com> (raw)
In-Reply-To: <20190320173332.18105-2-alban@kinvolk.io>
On Wed, 20 Mar 2019 18:33:27 +0100, Alban Crequy wrote:
> From: Alban Crequy <alban@kinvolk.io>
>
> Symptoms:
> $ sudo bpftool map create /sys/fs/bpf/dir/foobar type hash key 8 value 8 entries
> Error: '8' needs at least 2 arguments, 1 found
>
> After this patch:
> $ sudo bpftool map create /sys/fs/bpf/dir/foobar type hash key 8 value 8 entries
> Error: can't parse max entries: missing argument
>
> Signed-off-by: Alban Crequy <alban@kinvolk.io>
> ---
> tools/bpf/bpftool/common.c | 5 +++++
> tools/bpf/bpftool/main.h | 11 +++++++----
> tools/bpf/bpftool/map.c | 15 ++++++++++++---
> tools/bpf/bpftool/prog.c | 2 --
> 4 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> index f7261fad45c1..e560cd8f66bc 100644
> --- a/tools/bpf/bpftool/common.c
> +++ b/tools/bpf/bpftool/common.c
> @@ -627,6 +627,11 @@ int parse_u32_arg(int *argc, char ***argv, __u32 *val, const char *what)
> return -1;
> }
>
> + if (*argc == 0) {
> + p_err("can't parse %s: missing argument", what);
> + return -1;
> + }
>
> *val = strtoul(**argv, &endptr, 0);
> if (*endptr) {
> p_err("can't parse %s as %s", **argv, what);
> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> index d7dd84d3c660..7fc2973446d0 100644
> --- a/tools/bpf/bpftool/main.h
> +++ b/tools/bpf/bpftool/main.h
> @@ -22,14 +22,17 @@
> #define NEXT_ARGP() ({ (*argc)--; (*argv)++; if (*argc < 0) usage(); })
> #define BAD_ARG() ({ p_err("what is '%s'?", *argv); -1; })
> #define GET_ARG() ({ argc--; *argv++; })
> -#define REQ_ARGS(cnt) \
> +
> +#define REQ_ARGS(cnt) REQ_ARGS_GENERIC(cnt, argc, argv)
> +#define REQ_ARGSP(cnt) REQ_ARGS_GENERIC(cnt, *argc, *argv)
> +#define REQ_ARGS_GENERIC(cnt, ARGC, ARGV) \
> ({ \
> int _cnt = (cnt); \
> bool _res; \
> \
> - if (argc < _cnt) { \
> - p_err("'%s' needs at least %d arguments, %d found", \
> - argv[-1], _cnt, argc); \
> + if ((ARGC) < _cnt) { \
> + p_err("'%s' needs at least %d arguments, %d found", \
> + (ARGV)[-1], _cnt, (ARGC)); \
> _res = false; \
> } else { \
> _res = true; \
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index 994a7e0d16fb..18f9bc3aed4f 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -98,6 +98,9 @@ int map_parse_fd(int *argc, char ***argv)
> char *endptr;
>
> NEXT_ARGP();
> + if (!REQ_ARGSP(1)) {
> + return -1;
> + }
>
> id = strtoul(**argv, &endptr, 0);
> if (*endptr) {
> @@ -114,6 +117,9 @@ int map_parse_fd(int *argc, char ***argv)
> char *path;
>
> NEXT_ARGP();
> + if (!REQ_ARGSP(1)) {
> + return -1;
> + }
>
> path = **argv;
> NEXT_ARGP();
> @@ -1100,11 +1106,10 @@ static int do_create(int argc, char **argv)
> pinfile = GET_ARG();
>
> while (argc) {
> - if (!REQ_ARGS(2))
> - return -1;
Seems like it'd be better to make a version of REQ_ARGS() which
uses the next arg in the error message (if it exists) rather than
pushing all the checks out?
I think the format of the parsing loop is quite canonical here, with
all arguments being in the <keyword> <value> form, therefore I think
it's worthwhile improving the REQ_ARGS(2) check.
> if (is_prefix(*argv, "type")) {
> NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
>
> if (attr.map_type) {
> p_err("map type already specified");
> @@ -1119,6 +1124,8 @@ static int do_create(int argc, char **argv)
> NEXT_ARG();
> } else if (is_prefix(*argv, "name")) {
> NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
> attr.name = GET_ARG();
> } else if (is_prefix(*argv, "key")) {
> if (parse_u32_arg(&argc, &argv, &attr.key_size,
> @@ -1138,6 +1145,8 @@ static int do_create(int argc, char **argv)
> return -1;
> } else if (is_prefix(*argv, "dev")) {
> NEXT_ARG();
> + if (!REQ_ARGS(1))
> + return -1;
>
> if (attr.map_ifindex) {
> p_err("offload device already specified");
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 8ef80d65a474..e0875ef5e444 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -836,8 +836,6 @@ static int parse_attach_detach_args(int argc, char **argv, int *progfd,
> }
>
> NEXT_ARG();
> - if (!REQ_ARGS(2))
> - return -EINVAL;
>
> *mapfd = map_parse_fd(&argc, &argv);
> if (*mapfd < 0)
next prev parent reply other threads:[~2019-03-20 21:02 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-20 17:33 [PATCH bpf-next v1 1/7] tools: bpftool: fix infinite loop Alban Crequy
2019-03-20 17:33 ` [PATCH bpf-next v1 2/7] tools: bpftool: fix error message on invalid argument count Alban Crequy
2019-03-20 21:02 ` Jakub Kicinski [this message]
2019-03-20 17:33 ` [PATCH bpf-next v1 3/7] tools: bpftool: create map of maps Alban Crequy
2019-03-20 19:00 ` Quentin Monnet
2019-03-20 17:33 ` [PATCH bpf-next v1 4/7] tools: bpftool: implement map exec command Alban Crequy
2019-03-20 19:01 ` Quentin Monnet
2019-03-20 21:23 ` Jakub Kicinski
2019-03-21 9:57 ` Alban Crequy
2019-03-21 20:16 ` Jakub Kicinski
2019-03-20 17:33 ` [PATCH bpf-next v1 5/7] tools: bpftool: support loading map by fd from parent process Alban Crequy
2019-03-20 19:05 ` Quentin Monnet
2019-03-20 17:33 ` [PATCH bpf-next v1 6/7] tools: bpftool: fix bpffs mount when pinning subdirectories Alban Crequy
2019-03-20 19:01 ` Quentin Monnet
2019-03-20 17:33 ` [PATCH bpf-next v1 7/7] tools: bpftool: add error message on map pinning failure Alban Crequy
2019-03-20 19:01 ` Quentin Monnet
2019-03-20 20:54 ` [PATCH bpf-next v1 1/7] tools: bpftool: fix infinite loop Jakub Kicinski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190320140205.54f87bb9@cakuba.netronome.com \
--to=jakub.kicinski@netronome.com \
--cc=alban.crequy@gmail.com \
--cc=alban@kinvolk.io \
--cc=ast@kernel.org \
--cc=daniel@iogearbox.net \
--cc=iago@kinvolk.io \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).