netdev - Re: [bpf-next V4 PATCH 2/8] bpf: devmap prepare xdp frames for bulking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 May 2018 12:38:42 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     netdev@...r.kernel.org, Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Christoph Hellwig <hch@...radead.org>,
        BjörnTöpel <bjorn.topel@...el.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        makita.toshiaki@....ntt.co.jp, brouer@...hat.com
Subject: Re: [bpf-next V4 PATCH 2/8] bpf: devmap prepare xdp frames for
 bulking

On Wed, 23 May 2018 11:54:38 +0200
Daniel Borkmann <daniel@...earbox.net> wrote:

> > +	for (i = 0; i < bq->count; i++) {
> > +		struct xdp_frame *xdpf = bq->q[i];
> > +		int err;
> > +
> > +		err = dev->netdev_ops->ndo_xdp_xmit(dev, xdpf);
> > +		if (err) {
> > +			drops++;
> > +			xdp_return_frame(xdpf);
> > +		}
> > +		processed++;  
> 
> This sort of thing makes it really hard to review. 'processed' and
> 'drops' are not read anywhere in this function. So I need to go and
> check all the other patches whether there's further logic involved here
> or not. I had to review your series after applying all patches in a
> local branch, please never do this. Add the logic in a patch where it's
> self-contained and obvious to review.

Sorry, 'processed' and 'drops' were used by the tracepoint that Alexei
asked me to split into another (next patch).  And I can see that I have
renamed 'processed' to 'sent' in the next tracepoint patch, which makes
reviewing even harder sorry.  Those lines should have been moved to the
tracepoint patch. My mistake when splitting up the patches.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer