| Message ID | 20210817092729.433-13-magnus.karlsson@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | selftests: xsk: various simplifications | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 9 maintainers not CCed: davem@davemloft.net songliubraving@fb.com shuah@kernel.org kafai@fb.com hawk@kernel.org kuba@kernel.org john.fastabend@gmail.com linux-kselftest@vger.kernel.org kpsingh@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 53 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Tue, Aug 17, 2021 at 11:27:25AM +0200, Magnus Karlsson wrote: > From: Magnus Karlsson <magnus.karlsson@intel.com> > > Remove the cleanup right before the program/process exits as this will > trigger the cleanup without us having to write or maintain any > code. The application is not a library, so let us benefit from that. Not a fan of that, I'd rather keep things tidy, but you're right that dropping this logic won't hurt us. > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com> > --- > tools/testing/selftests/bpf/xdpxceiver.c | 29 +++++------------------- > 1 file changed, 6 insertions(+), 23 deletions(-) > > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c > index 8ff24472ef1e..c1bb03e0ca07 100644 > --- a/tools/testing/selftests/bpf/xdpxceiver.c > +++ b/tools/testing/selftests/bpf/xdpxceiver.c > @@ -1041,28 +1041,24 @@ static void run_pkt_test(int mode, int type) > int main(int argc, char **argv) > { > struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY }; > - bool failure = false; > int i, j; > > if (setrlimit(RLIMIT_MEMLOCK, &_rlim)) > exit_with_error(errno); > > - for (int i = 0; i < MAX_INTERFACES; i++) { > + for (i = 0; i < MAX_INTERFACES; i++) { > ifdict[i] = malloc(sizeof(struct ifobject)); > if (!ifdict[i]) > exit_with_error(errno); > > ifdict[i]->ifdict_index = i; > ifdict[i]->xsk_arr = calloc(2, sizeof(struct xsk_socket_info *)); > - if (!ifdict[i]->xsk_arr) { > - failure = true; > - goto cleanup; > - } > + if (!ifdict[i]->xsk_arr) > + exit_with_error(errno); > + > ifdict[i]->umem_arr = calloc(2, sizeof(struct xsk_umem_info *)); > - if (!ifdict[i]->umem_arr) { > - failure = true; > - goto cleanup; > - } > + if (!ifdict[i]->umem_arr) > + exit_with_error(errno); > } > > setlocale(LC_ALL, ""); > @@ -1081,19 +1077,6 @@ int main(int argc, char **argv) > } > } > > -cleanup: > - for (int i = 0; i < MAX_INTERFACES; i++) { > - if (ifdict[i]->ns_fd != -1) > - close(ifdict[i]->ns_fd); > - free(ifdict[i]->xsk_arr); > - free(ifdict[i]->umem_arr); > - free(ifdict[i]); > - } > - > - if (failure) > - exit_with_error(errno); > - > ksft_exit_pass(); > - > return 0; > } > -- > 2.29.0 >
On Thu, Aug 19, 2021 at 11:43 AM Maciej Fijalkowski <maciej.fijalkowski@intel.com> wrote: > > On Tue, Aug 17, 2021 at 11:27:25AM +0200, Magnus Karlsson wrote: > > From: Magnus Karlsson <magnus.karlsson@intel.com> > > > > Remove the cleanup right before the program/process exits as this will > > trigger the cleanup without us having to write or maintain any > > code. The application is not a library, so let us benefit from that. > > Not a fan of that, I'd rather keep things tidy, but you're right that > dropping this logic won't hurt us. My strategy here was that all functions should clean up themselves and be tidy, the exception to that being the main function as if it exists, the program is gone and libc will clean up the allocations for us. Maybe a bit pragmatic, but I do prefer less code in this case. On the other hand, I do not have any strong objections to keeping the code. Just think it is unnecessary. But if we hide the allocations in a function, then I would need to deallocate them later for it to look clean (according to the above logic). Maybe that will improve readabilty. Will try it out. /Magnus > > > > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com> > > --- > > tools/testing/selftests/bpf/xdpxceiver.c | 29 +++++------------------- > > 1 file changed, 6 insertions(+), 23 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c > > index 8ff24472ef1e..c1bb03e0ca07 100644 > > --- a/tools/testing/selftests/bpf/xdpxceiver.c > > +++ b/tools/testing/selftests/bpf/xdpxceiver.c > > @@ -1041,28 +1041,24 @@ static void run_pkt_test(int mode, int type) > > int main(int argc, char **argv) > > { > > struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY }; > > - bool failure = false; > > int i, j; > > > > if (setrlimit(RLIMIT_MEMLOCK, &_rlim)) > > exit_with_error(errno); > > > > - for (int i = 0; i < MAX_INTERFACES; i++) { > > + for (i = 0; i < MAX_INTERFACES; i++) { > > ifdict[i] = malloc(sizeof(struct ifobject)); > > if (!ifdict[i]) > > exit_with_error(errno); > > > > ifdict[i]->ifdict_index = i; > > ifdict[i]->xsk_arr = calloc(2, sizeof(struct xsk_socket_info *)); > > - if (!ifdict[i]->xsk_arr) { > > - failure = true; > > - goto cleanup; > > - } > > + if (!ifdict[i]->xsk_arr) > > + exit_with_error(errno); > > + > > ifdict[i]->umem_arr = calloc(2, sizeof(struct xsk_umem_info *)); > > - if (!ifdict[i]->umem_arr) { > > - failure = true; > > - goto cleanup; > > - } > > + if (!ifdict[i]->umem_arr) > > + exit_with_error(errno); > > } > > > > setlocale(LC_ALL, ""); > > @@ -1081,19 +1077,6 @@ int main(int argc, char **argv) > > } > > } > > > > -cleanup: > > - for (int i = 0; i < MAX_INTERFACES; i++) { > > - if (ifdict[i]->ns_fd != -1) > > - close(ifdict[i]->ns_fd); > > - free(ifdict[i]->xsk_arr); > > - free(ifdict[i]->umem_arr); > > - free(ifdict[i]); > > - } > > - > > - if (failure) > > - exit_with_error(errno); > > - > > ksft_exit_pass(); > > - > > return 0; > > } > > -- > > 2.29.0 > >
diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c index 8ff24472ef1e..c1bb03e0ca07 100644 --- a/tools/testing/selftests/bpf/xdpxceiver.c +++ b/tools/testing/selftests/bpf/xdpxceiver.c @@ -1041,28 +1041,24 @@ static void run_pkt_test(int mode, int type) int main(int argc, char **argv) { struct rlimit _rlim = { RLIM_INFINITY, RLIM_INFINITY }; - bool failure = false; int i, j; if (setrlimit(RLIMIT_MEMLOCK, &_rlim)) exit_with_error(errno); - for (int i = 0; i < MAX_INTERFACES; i++) { + for (i = 0; i < MAX_INTERFACES; i++) { ifdict[i] = malloc(sizeof(struct ifobject)); if (!ifdict[i]) exit_with_error(errno); ifdict[i]->ifdict_index = i; ifdict[i]->xsk_arr = calloc(2, sizeof(struct xsk_socket_info *)); - if (!ifdict[i]->xsk_arr) { - failure = true; - goto cleanup; - } + if (!ifdict[i]->xsk_arr) + exit_with_error(errno); + ifdict[i]->umem_arr = calloc(2, sizeof(struct xsk_umem_info *)); - if (!ifdict[i]->umem_arr) { - failure = true; - goto cleanup; - } + if (!ifdict[i]->umem_arr) + exit_with_error(errno); } setlocale(LC_ALL, ""); @@ -1081,19 +1077,6 @@ int main(int argc, char **argv) } } -cleanup: - for (int i = 0; i < MAX_INTERFACES; i++) { - if (ifdict[i]->ns_fd != -1) - close(ifdict[i]->ns_fd); - free(ifdict[i]->xsk_arr); - free(ifdict[i]->umem_arr); - free(ifdict[i]); - } - - if (failure) - exit_with_error(errno); - ksft_exit_pass(); - return 0; }