diff mbox series

[bpf] tools, bpftool: check return value of function codegen

Message ID 20200610130807.21497-1-tklauser@distanz.ch
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series [bpf] tools, bpftool: check return value of function codegen | expand

Commit Message

Tobias Klauser June 10, 2020, 1:08 p.m. UTC 
The codegen function might fail an return an error. Check its return
value in all call sites and handle it properly.

Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 tools/bpf/bpftool/gen.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

Comments

Andrii Nakryiko June 10, 2020, 6:50 p.m. UTC | #1 
On Wed, Jun 10, 2020 at 6:09 AM Tobias Klauser <tklauser@distanz.ch> wrote:
>
> The codegen function might fail an return an error. Check its return
> value in all call sites and handle it properly.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> ---

codegen() can fail only if the system ran out of memory or the static
template is malformed. Both are highly unlikely. I wonder if the
better approach would be to just exit(1) on such an unlikely error
inside codegen() and make the function itself void-returning.

We'll probably expand codegen to other languages soon, so not having
to do those annoying error checks everywhere is a good thing.

What do you think?

[...]
Tobias Klauser June 11, 2020, 9:05 a.m. UTC | #2 
On 2020-06-10 at 20:50:06 +0200, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> On Wed, Jun 10, 2020 at 6:09 AM Tobias Klauser <tklauser@distanz.ch> wrote:
> >
> > The codegen function might fail an return an error. Check its return
> > value in all call sites and handle it properly.
> >
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
> > ---
> 
> codegen() can fail only if the system ran out of memory or the static
> template is malformed. Both are highly unlikely. I wonder if the
> better approach would be to just exit(1) on such an unlikely error
> inside codegen() and make the function itself void-returning.
> 
> We'll probably expand codegen to other languages soon, so not having
> to do those annoying error checks everywhere is a good thing.
> 
> What do you think?

Sounds good to me, thanks. I'll send a v2 implementing your suggestion.
diff mbox series

Patch

diff --git a/tools/bpf/bpftool/gen.c b/tools/bpf/bpftool/gen.c
index ecbae47e66b8..b5fa3060dce3 100644
--- a/tools/bpf/bpftool/gen.c
+++ b/tools/bpf/bpftool/gen.c
@@ -325,7 +325,7 @@  static int do_skeleton(int argc, char **argv)
 	}
 
 	get_header_guard(header_guard, obj_name);
-	codegen("\
+	err = codegen("\
 		\n\
 		/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */   \n\
 									    \n\
@@ -342,6 +342,8 @@  static int do_skeleton(int argc, char **argv)
 		",
 		obj_name, header_guard
 	);
+	if (err)
+		goto out;
 
 	if (map_cnt) {
 		printf("\tstruct {\n");
@@ -376,7 +378,7 @@  static int do_skeleton(int argc, char **argv)
 			goto out;
 	}
 
-	codegen("\
+	err = codegen("\
 		\n\
 		};							    \n\
 									    \n\
@@ -453,8 +455,10 @@  static int do_skeleton(int argc, char **argv)
 		",
 		obj_name
 	);
+	if (err)
+		goto out;
 
-	codegen("\
+	err = codegen("\
 		\n\
 									    \n\
 		static inline int					    \n\
@@ -474,7 +478,7 @@  static int do_skeleton(int argc, char **argv)
 		obj_name
 	);
 	if (map_cnt) {
-		codegen("\
+		err = codegen("\
 			\n\
 									    \n\
 				/* maps */				    \n\
@@ -486,6 +490,9 @@  static int do_skeleton(int argc, char **argv)
 			",
 			map_cnt
 		);
+		if (err)
+			goto out;
+
 		i = 0;
 		bpf_object__for_each_map(map, obj) {
 			ident = get_map_ident(map);
@@ -493,13 +500,16 @@  static int do_skeleton(int argc, char **argv)
 			if (!ident)
 				continue;
 
-			codegen("\
+			err = codegen("\
 				\n\
 									    \n\
 					s->maps[%zu].name = \"%s\";	    \n\
 					s->maps[%zu].map = &obj->maps.%s;   \n\
 				",
 				i, bpf_map__name(map), i, ident);
+			if (err)
+				goto out;
+
 			/* memory-mapped internal maps */
 			if (bpf_map__is_internal(map) &&
 			    (bpf_map__def(map)->map_flags & BPF_F_MMAPABLE)) {
@@ -510,7 +520,7 @@  static int do_skeleton(int argc, char **argv)
 		}
 	}
 	if (prog_cnt) {
-		codegen("\
+		err = codegen("\
 			\n\
 									    \n\
 				/* programs */				    \n\
@@ -522,6 +532,9 @@  static int do_skeleton(int argc, char **argv)
 			",
 			prog_cnt
 		);
+		if (err)
+			goto out;
+
 		i = 0;
 		bpf_object__for_each_program(prog, obj) {
 			codegen("\
@@ -535,13 +548,15 @@  static int do_skeleton(int argc, char **argv)
 			i++;
 		}
 	}
-	codegen("\
+	err = codegen("\
 		\n\
 									    \n\
 			s->data_sz = %d;				    \n\
 			s->data = (void *)\"\\				    \n\
 		",
 		file_sz);
+	if (err)
+		goto out;
 
 	/* embed contents of BPF object file */
 	for (i = 0, len = 0; i < file_sz; i++) {
@@ -558,7 +573,7 @@  static int do_skeleton(int argc, char **argv)
 			printf("\\x%02x", (unsigned char)obj_data[i]);
 	}
 
-	codegen("\
+	err = codegen("\
 		\n\
 		\";							    \n\
 									    \n\
@@ -571,7 +586,6 @@  static int do_skeleton(int argc, char **argv)
 		#endif /* %s */						    \n\
 		",
 		header_guard);
-	err = 0;
 out:
 	bpf_object__close(obj);
 	if (obj_data)