Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields

[Min Hu (Connor)]

Hi, Morten and all,
        I have a questions which has bothering me for a long time.
        What's the difference between API and ABI？
        Why does this patch does not breake ABI， but break API(maybe)?
        
        Hope for your reply, thanks.

在 2021/5/27 23:56, Morten Brørup 写道:

From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Gregory Etelson
Sent: Thursday, 27 May 2021 17.29

and version fields

RTE IPv4 header definition combines the `version' and `ihl'  fields
into a single structure member.
This patch introduces dedicated structure members for both `version'
and `ihl' IPv4 fields. Separated header fields definitions allow to
create simplified code to match on the IHL value in a flow rule.
The original `version_ihl' structure member is kept for backward
compatibility.

Signed-off-by: Gregory Etelson <getel...@nvidia.com>
---
  app/test/test_flow_classify.c |  8 ++++----
  lib/net/rte_ip.h              | 16 +++++++++++++++-
  2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/app/test/test_flow_classify.c
b/app/test/test_flow_classify.c
index 951606f248..4f64be5357 100644
--- a/app/test/test_flow_classify.c
+++ b/app/test/test_flow_classify.c
@@ -95,7 +95,7 @@ static struct rte_acl_field_def
ipv4_defs[NUM_FIELDS_IPV4] = {
   *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
   */
  static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
-       { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
+       { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
          RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}
  };
  static const struct rte_flow_item_ipv4 ipv4_mask_24 = {
@@ -131,7 +131,7 @@ static struct rte_flow_item  end_item = {
RTE_FLOW_ITEM_TYPE_END,
   *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
   */
  static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
-       { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
+       { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
          RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}
  };

@@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
RTE_FLOW_ITEM_TYPE_TCP,
   *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
   */
  static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
-       { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
-       RTE_IPV4(15, 16, 17, 18)}
+       { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
+       RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
  };

  static struct rte_flow_item_sctp sctp_spec_1 = {
diff --git a/lib/net/rte_ip.h b/lib/net/rte_ip.h
index 4b728969c1..684bb028b2 100644
--- a/lib/net/rte_ip.h
+++ b/lib/net/rte_ip.h
@@ -38,7 +38,21 @@ extern "C" {
   * IPv4 Header
   */
  struct rte_ipv4_hdr {
-       uint8_t  version_ihl;           /**< version and header length */
+       __extension__
+       union {
+               uint8_t version_ihl;    /**< version and header length */
+               struct {
+#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
+                       uint8_t ihl:4;
+                       uint8_t version:4;
+#elif RTE_BYTE_ORDER == RTE_BIG_ENDIAN
+                       uint8_t version:4;
+                       uint8_t ihl:4;
+#else
+#error "setup endian definition"
+#endif
+               };
+       };
        uint8_t  type_of_service;       /**< type of service */
        rte_be16_t total_length;        /**< length of packet */
        rte_be16_t packet_id;           /**< packet ID */
--
2.31.1

This does not break the ABI, but it could be discussed if it breaks the API due 
to the required structure initialization changes shown in test_flow_classify.c. 
I think this patch is an improvement, and that such structure modifications 
should be generally accepted, so:

Acked-by: Morten Brørup <m...@smartsharesystems.com>

.