bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Magnus Karlsson <magnus.karlsson@gmail.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: "Karlsson, Magnus" <magnus.karlsson@intel.com>,
	"Björn Töpel" <bjorn@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Network Development" <netdev@vger.kernel.org>,
	"Jonathan Lemon" <jonathan.lemon@gmail.com>,
	"Ciara Loftus" <ciara.loftus@intel.com>,
	bpf <bpf@vger.kernel.org>, "Yonghong Song" <yhs@fb.com>,
	"Andrii Nakryiko" <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v2 12/16] selftests: xsk: remove cleanup at end of program
Date: Thu, 19 Aug 2021 12:02:33 +0200	[thread overview]
Message-ID: <CAJ8uoz28+uM9OMq0JKi2bOXR0F9E66LFuYahnTMUaxnRJDtRMQ@mail.gmail.com> (raw)
In-Reply-To: <20210819092849.GB32204@ranger.igk.intel.com>

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
> >

  reply	other threads:[~2021-08-19 10:02 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17  9:27 [PATCH bpf-next v2 00/16] selftests: xsk: various simplifications Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 01/16] selftests: xsk: remove color mode Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 02/16] selftests: xsk: remove the num_tx_packets option Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 03/16] selftests: xsk: remove unused variables Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 04/16] selftests: xsk: return correct error codes Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 05/16] selftests: xsk: simplify the retry code Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 06/16] selftests: xsk: remove end-of-test packet Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 07/16] selftests: xsk: disassociate umem size with packets sent Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 08/16] selftests: xsk: rename worker_* functions that are not thread entry points Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 09/16] selftests: xsk: simplify packet validation in xsk tests Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 10/16] selftests: xsk: validate tx stats on tx thread Magnus Karlsson
2021-08-19  9:26   ` Maciej Fijalkowski
2021-08-19  9:52     ` Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 11/16] selftests: xsk: decrease batch size Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 12/16] selftests: xsk: remove cleanup at end of program Magnus Karlsson
2021-08-19  9:28   ` Maciej Fijalkowski
2021-08-19 10:02     ` Magnus Karlsson [this message]
2021-08-17  9:27 ` [PATCH bpf-next v2 13/16] selftests: xsk: generate packet directly in umem Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 14/16] selftests: xsk: generate packets from specification Magnus Karlsson
2021-08-19 10:10   ` Maciej Fijalkowski
2021-08-19 10:38     ` Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 15/16] selftests: xsk: make enums lower case Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 16/16] selftests: xsk: preface options with opt Magnus Karlsson
2021-08-19 10:13 ` [PATCH bpf-next v2 00/16] selftests: xsk: various simplifications Maciej Fijalkowski

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=CAJ8uoz28+uM9OMq0JKi2bOXR0F9E66LFuYahnTMUaxnRJDtRMQ@mail.gmail.com \
    --to=magnus.karlsson@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=ciara.loftus@intel.com \
    --cc=daniel@iogearbox.net \
    --cc=jonathan.lemon@gmail.com \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=yhs@fb.com \
    /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).