diff mbox

[dpdk-dev,v3,1/6] net: changed arp_hdr struct declaration

Message ID 1424366779-14256-2-git-send-email-michalx.k.jastrzebski@intel.com (mailing list archive)
State Changes Requested, archived
Headers

Commit Message

Michal Jastrzebski Feb. 19, 2015, 5:26 p.m. UTC 
  From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>

Changed MAC address type from uint8_t[6] to struct ether_addr and IP
address type from uint8_t[4] to uint32_t. Also removed union from
arp_hdr struct. Updated test-pmd to match new arp_hdr version.

Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
---
 app/test-pmd/icmpecho.c  |   27 ++++++++++-----------------
 lib/librte_net/rte_arp.h |   13 ++++++-------
 2 files changed, 16 insertions(+), 24 deletions(-)

Comments

Thomas Monjalon Feb. 20, 2015, 2:30 p.m. UTC | #1 
2015-02-19 18:26, Michal Jastrzebski:
> From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> 
> Changed MAC address type from uint8_t[6] to struct ether_addr and IP
> address type from uint8_t[4] to uint32_t. Also removed union from
> arp_hdr struct. Updated test-pmd to match new arp_hdr version.
> 
> Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>

Why?
"Changed A to B" is not a sufficient explanation.
Maciej Gajdzica Feb. 20, 2015, 2:54 p.m. UTC | #2 
> 2015-02-19 18:26, Michal Jastrzebski:
> > From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> >
> > Changed MAC address type from uint8_t[6] to struct ether_addr and IP
> > address type from uint8_t[4] to uint32_t. Also removed union from
> > arp_hdr struct. Updated test-pmd to match new arp_hdr version.
> >
> > Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> 
> Why?
> "Changed A to B" is not a sufficient explanation.

Hi Thomas

I changed commit message to this:

Changed MAC address type from uint8_t[6] to struct ether_addr and IP 
address type from uint8_t[4] to uint32_t to make it consistent with other
DPDK code using MAC and IP addresses. It allows us to use is_same_ether_addr
and ether_addr_copy functions on MAC addresses in ARP header.  Also
removed union from arp_hdr struct to make calls to arp_data items
shorter. Updated test-pmd to match new arp_hdr version.

Is that sufficient?

Best regards,
Maciek
--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
Thomas Monjalon Feb. 20, 2015, 3:22 p.m. UTC | #3 
2015-02-20 14:54, Gajdzica, MaciejX T:
> > 2015-02-19 18:26, Michal Jastrzebski:
> > > From: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> > >
> > > Changed MAC address type from uint8_t[6] to struct ether_addr and IP
> > > address type from uint8_t[4] to uint32_t. Also removed union from
> > > arp_hdr struct. Updated test-pmd to match new arp_hdr version.
> > >
> > > Signed-off-by: Maciej Gajdzica <maciejx.t.gajdzica@intel.com>
> > 
> > Why?
> > "Changed A to B" is not a sufficient explanation.
> 
> Hi Thomas
> 
> I changed commit message to this:
> 
> Changed MAC address type from uint8_t[6] to struct ether_addr and IP 
> address type from uint8_t[4] to uint32_t to make it consistent with other
> DPDK code using MAC and IP addresses. It allows us to use is_same_ether_addr
> and ether_addr_copy functions on MAC addresses in ARP header.  Also
> removed union from arp_hdr struct to make calls to arp_data items
> shorter. Updated test-pmd to match new arp_hdr version.
> 
> Is that sufficient?

Yes it's far better! Thanks

One day we could have a script to run before sending a patch.
It would make some smoky tests and ask:
"did you explain why you make this change?" ;)

> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Please try to remove this footer.
This email was distributed by the mailing list engine, despite it's prohibited.
I don't want to go in jailhouse ;)
diff mbox

Patch

diff --git a/app/test-pmd/icmpecho.c b/app/test-pmd/icmpecho.c
index 08ea01d..010c5a9 100644
--- a/app/test-pmd/icmpecho.c
+++ b/app/test-pmd/icmpecho.c
@@ -371,18 +371,14 @@  reply_to_icmp_echo_rqsts(struct fwd_stream *fs)
 				continue;
 			}
 			if (verbose_level > 0) {
-				memcpy(&eth_addr,
-				       arp_h->arp_data.arp_ip.arp_sha, 6);
+				ether_addr_copy(&arp_h->arp_data.arp_sha, &eth_addr);
 				ether_addr_dump("        sha=", &eth_addr);
-				memcpy(&ip_addr,
-				       arp_h->arp_data.arp_ip.arp_sip, 4);
+				ip_addr = arp_h->arp_data.arp_sip;
 				ipv4_addr_dump(" sip=", ip_addr);
 				printf("\n");
-				memcpy(&eth_addr,
-				       arp_h->arp_data.arp_ip.arp_tha, 6);
+				ether_addr_copy(&arp_h->arp_data.arp_tha, &eth_addr);
 				ether_addr_dump("        tha=", &eth_addr);
-				memcpy(&ip_addr,
-				       arp_h->arp_data.arp_ip.arp_tip, 4);
+				ip_addr = arp_h->arp_data.arp_tip;
 				ipv4_addr_dump(" tip=", ip_addr);
 				printf("\n");
 			}
@@ -402,17 +398,14 @@  reply_to_icmp_echo_rqsts(struct fwd_stream *fs)
 					&eth_h->s_addr);
 
 			arp_h->arp_op = rte_cpu_to_be_16(ARP_OP_REPLY);
-			memcpy(&eth_addr, arp_h->arp_data.arp_ip.arp_tha, 6);
-			memcpy(arp_h->arp_data.arp_ip.arp_tha,
-			       arp_h->arp_data.arp_ip.arp_sha, 6);
-			memcpy(arp_h->arp_data.arp_ip.arp_sha,
-			       &eth_h->s_addr, 6);
+			ether_addr_copy(&arp_h->arp_data.arp_tha, &eth_addr);
+			ether_addr_copy(&arp_h->arp_data.arp_sha, &arp_h->arp_data.arp_tha);
+			ether_addr_copy(&eth_addr, &arp_h->arp_data.arp_sha);
 
 			/* Swap IP addresses in ARP payload */
-			memcpy(&ip_addr, arp_h->arp_data.arp_ip.arp_sip, 4);
-			memcpy(arp_h->arp_data.arp_ip.arp_sip,
-			       arp_h->arp_data.arp_ip.arp_tip, 4);
-			memcpy(arp_h->arp_data.arp_ip.arp_tip, &ip_addr, 4);
+			ip_addr = arp_h->arp_data.arp_sip;
+			arp_h->arp_data.arp_sip = arp_h->arp_data.arp_tip;
+			arp_h->arp_data.arp_tip = ip_addr;
 			pkts_burst[nb_replies++] = pkt;
 			continue;
 		}
diff --git a/lib/librte_net/rte_arp.h b/lib/librte_net/rte_arp.h
index c7b0e51..72108a1 100644
--- a/lib/librte_net/rte_arp.h
+++ b/lib/librte_net/rte_arp.h
@@ -39,6 +39,7 @@ 
  */
 
 #include <stdint.h>
+#include <rte_ether.h>
 
 #ifdef __cplusplus
 extern "C" {
@@ -48,10 +49,10 @@  extern "C" {
  * ARP header IPv4 payload.
  */
 struct arp_ipv4 {
-	uint8_t  arp_sha[6]; /* sender hardware address */
-	uint8_t  arp_sip[4]; /* sender IP address */
-	uint8_t  arp_tha[6]; /* target hardware address */
-	uint8_t  arp_tip[4]; /* target IP address */
+	struct ether_addr  arp_sha;	/* sender hardware address */
+	uint32_t  arp_sip;			/* sender IP address */
+	struct ether_addr  arp_tha;	/* target hardware address */
+	uint32_t  arp_tip;			/* target IP address */
 } __attribute__((__packed__));
 
 /**
@@ -72,9 +73,7 @@  struct arp_hdr {
 #define	ARP_OP_INVREQUEST 8 /* request to identify peer */
 #define	ARP_OP_INVREPLY   9 /* response identifying peer */
 
-	union {
-		struct arp_ipv4 arp_ip;
-	} arp_data;
+	struct arp_ipv4 arp_data;
 } __attribute__((__packed__));
 
 #ifdef __cplusplus