2023-03-07 18:15 UTC+0000 ~ Rae Marks <Raeanne.Marks@xxxxxxxxxxxxx> > Hi Quentin, > > My apologies, I linked to the incorrect line. In particular, I want to continue iterating if the call to bpf_map_get_fd_by_id fails. I cannot disclose at this time our use case due to confidentiality, but I can imagine a scenario for example if a kernel module wants to override the return value for some reason (not our use case, but a legitimate one). Below is my patch. Hi Rae, If you want to formally submit your patch, please make sure to also take a look at the generic documentation for patch submission [0]. In particular, you'll need to sign off your patch, and to make sure you generate it against the kernel tree (not the GitHub mirror). Please avoid top-posting on the mailing list, it makes it harder to follow conversations. [0] https://docs.kernel.org/process/submitting-patches.html#submittingpatches > > From 7f3eb5c045ec0169435c18af448ebe5eeb642cc6 Mon Sep 17 00:00:00 2001 > From: Rae Marks ramark@xxxxxxxxxxxxx > Date: Tue, 7 Mar 2023 10:06:34 -0800 > Subject: [PATCH] bpftool: Continue iterating if individual map operations fail > > If a call to bpf_map_get_fd_by_id or bpf_map_get_info_by_fd fails, > the current behavior is to bail out of the loop, which means no > other maps can be displayed or modified. With this change, the loop > will continue, so an error with one map will not affect the others. > --- > src/map.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/map.c b/src/map.c > index aaeb893..17074c1 100644 > --- a/src/map.c > +++ b/src/map.c > @@ -705,14 +705,14 @@ static int do_show(int argc, char **argv) > continue; > p_err("can't get map by id (%u): %s", > id, strerror(errno)); > - break; > + continue; You'd also need to remove the "if ... continue" three lines above this one. > } > > err = bpf_map_get_info_by_fd(fd, &info, &len); > if (err) { > p_err("can't get map info: %s", strerror(errno)); > close(fd); > - break; > + continue; > } > > if (json_output) I'm not really convinced at this stage. I can't see a good reason to keep iterating other than a map that has gone during the process, in which case it's -ENOENT and we already continue (commit 8207c6dd4746c). As I see it, any other error means something is going very wrong and it makes sense to abort, as retrieving the info for the next maps is likely to fail for the same reason. I prefer bpftool to print a single error in that case, rather than a list of errors for all existing maps. It's true that a kernel module could change the return value, but then modules can change return values in many places, and my feeling is that we can't afford to hypothetically accommodate for all of those. Quentin
