On Fri, Oct 20, 2023 at 09:19:21AM -0400, Ihar Hrachyshka wrote:
> On Fri, Oct 20, 2023 at 8:40 AM Simon Horman <ho...@ovn.org> wrote:
>
> > On Thu, Oct 19, 2023 at 09:00:39AM -0400, Ihar Hrachyshka wrote:
> > > On 10/19/23 7:14 AM, Simon Horman wrote:
> > > > On Thu, Oct 19, 2023 at 02:57:26AM +0000, Ihar Hrachyshka wrote:
> > > > > The command reads a flow string and an optional additional payload on
> > > > > stdin and produces a hex-string representation of the corresponding
> > > > > frame on stdout. It may receive more than a single input line.
> > > > >
> > > > > The command may be useful in tests, where we may need to produce hex
> > > > > frame payloads to compare observed frames against.
> > > > >
> > > > > As an example of the tool use, a single test case is converted to it.
> > > > > The test uses both normal and --bad-csum behavior of the tool,
> > > > > confirming it works as advertised.
> > > > >
> > > > > Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> > > > ...
> > > >
> > > > > @@ -3318,7 +3320,7 @@ packet_expand(struct dp_packet *p, const
> > struct flow *flow, size_t size)
> > > > > * from 'l7'. */
> > > > > void
> > > > > flow_compose(struct dp_packet *p, const struct flow *flow,
> > > > > - const void *l7, size_t l7_len)
> > > > > + const void *l7, size_t l7_len, bool bad_csum)
> > > > > {
> > > > > /* Add code to this function (or its callees) for emitting new
> > fields or
> > > > > * protocols. (This isn't essential, so it can be skipped for
> > initial
> > > > > @@ -3370,7 +3372,12 @@ flow_compose(struct dp_packet *p, const
> > struct flow *flow,
> > > > > /* Checksum has already been zeroed by put_zeros call. */
> > > > > ip->ip_csum = csum(ip, sizeof *ip);
> > > > >
> > > > > - dp_packet_ol_set_ip_csum_good(p);
> > > > > + if (bad_csum) {
> > > > > + ip->ip_csum++;
> > > > Hi Ihar,
> > > >
> > > > I am curious to know why '++'.
> > >
> > > I'm producing a checksum that is invalid. Since Internet checksum is
> > "16-bit
> > > ones' complement of the ones' complement sum", I assume that flipping any
> > > bit in it will result in an invalid result. Is it not the case? (++
> > could be
> > > anything else that guarantees a bit flip in the field. Nothing special
> > about
> > > ++ itself.)
> >
> > Yes, that is correct AFAIK.
> >
> > > And yes, I see that gcc is warning about this line in CI. So this line
> > may
> > > need adjustment for this reason.
> >
> > Thanks
> >
> > > > I am also wondering if we could re-cast this as 'skip-csum-verify' or
> > > > similar, rather than 'bad-csum'. After all, as it's not verified it
> > might
> > > > actually be correct.
> > > I am struggling to see how this could be "correct" after ++ on the final
> > > (valid) checksum. It may be that I miss something basic about how
> > checksums
> > > work though, let me know.
> >
> > I guess my question is really this: what is the goal and use-case for
> > this option?
> >
> >
> The patch includes an example test case that is converted to the tool. The
> test case 1) generates a packet with a garbled checksum; 2) pushes it
> through datapath; 3) confirms that the output has a proper checksum
> (fixed). That's why I needed a way to generate a "broken IPv4 checksum"
> packet - to confirm the behavior.
Thanks, I understand now. And I agree that 'bad-csum' is an appropriate
name for this option. Sorry for the noise.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
