Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2 bpf] tools: bpftool: Add missing close before bpftool net attach exit | From | "wanghai (M)" <> | Date | Tue, 10 Nov 2020 16:40:53 +0800 |
| |
在 2020/11/10 12:33, John Fastabend 写道: > Wang Hai wrote: >> progfd is created by prog_parse_fd(), before 'bpftool net attach' exit, >> it should be closed. >> >> Fixes: 04949ccc273e ("tools: bpftool: add net attach command to attach XDP on interface") >> Signed-off-by: Wang Hai <wanghai38@huawei.com> >> --- >> v1->v2: use cleanup tag instead of repeated closes >> tools/bpf/bpftool/net.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/tools/bpf/bpftool/net.c b/tools/bpf/bpftool/net.c >> index 910e7bac6e9e..1ac7228167e6 100644 >> --- a/tools/bpf/bpftool/net.c >> +++ b/tools/bpf/bpftool/net.c >> @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv) >> >> ifindex = net_parse_dev(&argc, &argv); >> if (ifindex < 1) { >> - close(progfd); >> - return -EINVAL; >> + err = -EINVAL; >> + goto cleanup; >> } >> >> if (argc) { >> @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) >> overwrite = true; >> } else { >> p_err("expected 'overwrite', got: '%s'?", *argv); >> - close(progfd); >> - return -EINVAL; >> + err = -EINVAL; >> + goto cleanup; >> } >> } >> >> @@ -600,13 +600,15 @@ static int do_attach(int argc, char **argv) > I think now that return value depends on this err it should be 'if (err)' > otherwise we risk retunring non-zero error code from do_attach which > will cause programs to fail. I agree with you. Thanks. >> if (err < 0) { > ^^^^^^^^^^^^ > if (err) { > >> p_err("interface %s attach failed: %s", >> attach_type_strings[attach_type], strerror(-err)); >> - return err; >> + goto cleanup; >> } >> >> if (json_output) >> jsonw_null(json_wtr); >> >> - return 0; > > Alternatively we could add an 'err = 0' here, but above should never > return a value >0 as far as I can see. It's true that 'err > 0' doesn't exist currently , but adding 'err = 0' would make the code clearer. Thanks for your advice. >> +cleanup: >> + close(progfd); >> + return err; >> } >> >> static int do_detach(int argc, char **argv) >> -- >> 2.17.1 >> Can it be fixed like this?
--- a/tools/bpf/bpftool/net.c +++ b/tools/bpf/bpftool/net.c @@ -578,8 +578,8 @@ static int do_attach(int argc, char **argv)
ifindex = net_parse_dev(&argc, &argv); if (ifindex < 1) { - close(progfd); - return -EINVAL; + err = -EINVAL; + goto cleanup; }
if (argc) { @@ -587,8 +587,8 @@ static int do_attach(int argc, char **argv) overwrite = true; } else { p_err("expected 'overwrite', got: '%s'?", *argv); - close(progfd); - return -EINVAL; + err = -EINVAL; + goto cleanup; } }
@@ -597,16 +597,19 @@ static int do_attach(int argc, char **argv) err = do_attach_detach_xdp(progfd, attach_type, ifindex, overwrite);
- if (err < 0) { + if (err) { p_err("interface %s attach failed: %s", attach_type_strings[attach_type], strerror(-err)); - return err; + goto cleanup; }
if (json_output) jsonw_null(json_wtr);
- return 0; + ret = 0; +cleanup: + close(progfd); + return err; }
> > . >
| |