On Fri, Oct 20, 2023 at 8:40 AM Simon Horman <[email protected]> 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 <[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? > > 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. > > > > + } 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
