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 <[email protected]>
> > ...
> >
> > > @@ -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?
> > > + } else {
> > > + dp_packet_ol_set_ip_csum_good(p);
> > > + }
> > > +
> > > pseudo_hdr_csum = packet_csum_pseudoheader(ip);
> > > flow_compose_l4_csum(p, flow, pseudo_hdr_csum);
> > > } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
> > ...
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
