Re: [ovs-dev] [PATCH v5] ovs-ofctl: Implement compose-packet --bare [--bad-csum].

2023-11-16 Thread Ilya Maximets
On 11/15/23 17:42, Simon Horman wrote:
> On Tue, Nov 14, 2023 at 05:59:37PM +, Ihar Hrachyshka wrote:
>> With --bare, it will produce a bare hexified payload with no spaces or
>> offset indicators inserted, which is useful in tests to produce frames
>> to pass to e.g. `ovs-ofctl receive`.
>>
>> With --bad-csum, it will produce a frame that has an invalid IP checksum
>> (applicable to IPv4 only because IPv6 doesn't have checksums.)
>>
>> The command is now more 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 --bare and --bad-csum behaviors of the
>> command, confirming they work as advertised.
>>
>> Signed-off-by: Ihar Hrachyshka 
> 
> Thanks Ihar,
> 
> I have checked and as far as I can tell this addresses
> the concerns raised by Ilya for v5.
> 
> Acked-by: Simon Horman 

Thanks, Ihar and Simon!  Applied.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH v5] ovs-ofctl: Implement compose-packet --bare [--bad-csum].

2023-11-15 Thread Simon Horman
On Tue, Nov 14, 2023 at 05:59:37PM +, Ihar Hrachyshka wrote:
> With --bare, it will produce a bare hexified payload with no spaces or
> offset indicators inserted, which is useful in tests to produce frames
> to pass to e.g. `ovs-ofctl receive`.
> 
> With --bad-csum, it will produce a frame that has an invalid IP checksum
> (applicable to IPv4 only because IPv6 doesn't have checksums.)
> 
> The command is now more 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 --bare and --bad-csum behaviors of the
> command, confirming they work as advertised.
> 
> Signed-off-by: Ihar Hrachyshka 

Thanks Ihar,

I have checked and as far as I can tell this addresses
the concerns raised by Ilya for v5.

Acked-by: Simon Horman 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


[ovs-dev] [PATCH v5] ovs-ofctl: Implement compose-packet --bare [--bad-csum].

2023-11-14 Thread Ihar Hrachyshka
With --bare, it will produce a bare hexified payload with no spaces or
offset indicators inserted, which is useful in tests to produce frames
to pass to e.g. `ovs-ofctl receive`.

With --bad-csum, it will produce a frame that has an invalid IP checksum
(applicable to IPv4 only because IPv6 doesn't have checksums.)

The command is now more 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 --bare and --bad-csum behaviors of the
command, confirming they work as advertised.

Signed-off-by: Ihar Hrachyshka 
---
v1: - initial version.
v2: - convert from appctl to ovstest.
- add --bad-csum support.
- drop separate test case and man for the tool.
v3: - fix build warnings on restricted ovs_be16 ++.
v4: - migrate to compose-packet.
v5: - rename --hexified into --bare.
- some other minor nits in test code, comments, commit msg etc.
---
 lib/flow.c   | 17 +++--
 lib/flow.h   |  2 +-
 lib/netdev-dummy.c   |  4 +--
 ofproto/ofproto-dpif-trace.c |  2 +-
 ofproto/ofproto-dpif.c   |  4 +--
 tests/dpif-netdev.at | 45 --
 utilities/ovs-ofctl.c| 47 ++--
 7 files changed, 81 insertions(+), 40 deletions(-)

diff --git a/lib/flow.c b/lib/flow.c
index fe226cf0f..b8f99f66b 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -3306,6 +3306,8 @@ packet_expand(struct dp_packet *p, const struct flow 
*flow, size_t size)
  * (This is useful only for testing, obviously, and the packet isn't really
  * valid.  Lots of fields are just zeroed.)
  *
+ * If 'bad_csum' is true, the final IP checksum is invalid.
+ *
  * For packets whose protocols can encapsulate arbitrary L7 payloads, 'l7' and
  * 'l7_len' determine that payload:
  *
@@ -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,18 @@ 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) {
+/*
+ * Internet checksum is a sum complement to zero, so any other
+ * value will result in an invalid checksum. Here, we flip one
+ * bit.
+ */
+ip->ip_csum ^= (OVS_FORCE ovs_be16) 0x1;
+dp_packet_ip_checksum_bad(p);
+} 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)) {
diff --git a/lib/flow.h b/lib/flow.h
index a9d026e1c..75a9be3c1 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -127,7 +127,7 @@ void flow_set_mpls_bos(struct flow *, int idx, uint8_t 
stack);
 void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse);

 void flow_compose(struct dp_packet *, const struct flow *,
-  const void *l7, size_t l7_len);
+  const void *l7, size_t l7_len, bool bad_csum);
 void packet_expand(struct dp_packet *, const struct flow *, size_t size);

 bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t *nw_proto,
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index fe82317d7..8c6e6d448 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -1769,7 +1769,7 @@ eth_from_flow_str(const char *s, size_t packet_size,

 packet = dp_packet_new(0);
 if (packet_size) {
-flow_compose(packet, flow, NULL, 0);
+flow_compose(packet, flow, NULL, 0, false);
 if (dp_packet_size(packet) < packet_size) {
 packet_expand(packet, flow, packet_size);
 } else if (dp_packet_size(packet) > packet_size){
@@ -1777,7 +1777,7 @@ eth_from_flow_str(const char *s, size_t packet_size,
 packet = NULL;
 }
 } else {
-flow_compose(packet, flow, NULL, 64);
+flow_compose(packet, flow, NULL, 64, false);
 }

 ofpbuf_uninit(_key);
diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
index 527e2f17e..b86e7fe07 100644
--- a/ofproto/ofproto-dpif-trace.c
+++ b/ofproto/ofproto-dpif-trace.c
@@ -440,7 +440,7 @@ parse_flow_and_packet(int argc, const char *argv[],
 if (generate_packet) {
 /* Generate a packet, as requested. */
 packet = dp_packet_new(0);
-flow_compose(packet, flow, l7, l7_len);
+flow_compose(packet,