On Wed, Feb 08, 2023 at 07:29:27AM +0100, Ales Musil wrote:
> On Tue, Feb 7, 2023 at 11:54 AM Simon Horman <[email protected]>
> wrote:
>
> > On Mon, Feb 06, 2023 at 12:46:10PM +0100, Ales Musil wrote:
> > > The inner header was not handled properly.
> > > Simplify the code which allows proper handling
> > > of the inner headers.
> > >
> > > Reported-at: https://bugzilla.redhat.com/2137754
> > > Signed-off-by: Ales Musil <[email protected]>
> >
> > Nice clean-up too :)
> >
> > Reviewed-by: Simon Horman <[email protected]>
> >
>
> Thank you for the review.
>
>
> >
> > > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > > index 550b2be9b..3162924ca 100644
> > > --- a/lib/conntrack.c
> > > +++ b/lib/conntrack.c
...
> > > +
> > > + if (key->dl_type == htons(ETH_TYPE_IP)) {
> > > + nat_packet_ipv4(pkt, &rev_key, nat_action);
> > >
> > > - reverse_pat_packet(pkt, conn);
> > > + struct icmp_header *icmp = (struct icmp_header *) l4;
> > > icmp->icmp_csum = 0;
> >
> > nit: not for this patch, but is the above line necessary?
> >
>
> It actually is, because the checksum is part of the data for which the
> checksum
> is computed. It is a bit confusing, but aligned with the specification:
>
> "For purposes of computing the checksum, the value of the checksum field is
> zero." [0]
>
> [0] https://datatracker.ietf.org/doc/html/rfc791
Yeah, right.
I was looking for that, but couldn't see it yesterday.
Now I do :)
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
