Patchwork [08/10] 802.3ad: remove public lacpdu_header

login
register
mail settings
Submitter holger@eitzenberger.org
Date Dec. 23, 2008, 10:01 p.m.
Message ID <20081223220600.479443201@jonathan.eitzenberger.org>
Download mbox | patch
Permalink /patch/15487/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

holger@eitzenberger.org - Dec. 23, 2008, 10:01 p.m.
It's strictly not necessary to define lacpdu_header in an include
file because it's only used in ad_lacpdu_send().  Remove that definition
and use an unnamed structure instead just for our purpose.  Which also
gives a chance to use shorter names which are still descriptive.

As the port is left unmodified it's now const.

Also cleanup that function a bit by removing useless braces around
one-liners and turn C++ style comments into C comments.

Signed-off-by: Holger Eitzenberger <holger@eitzenberger.org>
David Miller - Dec. 26, 2008, 9:43 p.m.
From: Holger Eitzenberger <holger@eitzenberger.org>
Date: Tue, 23 Dec 2008 23:01:09 +0100

> -	skb = dev_alloc_skb(length);
> -	if (!skb) {
> +	skb = dev_alloc_skb(len);
> +	if (skb == NULL)

"!skb" is preferred to the long-winded "skb == NULL", so
I'm tossing this patch and the rest which touch similar
areas.

Please don't mix cleanups and other changes unless you are %100
confident they will pass the most stringent review, otherwise your
patches will be rejected unnecessarily.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Dec. 26, 2008, 9:45 p.m.
From: David Miller <davem@davemloft.net>
Date: Fri, 26 Dec 2008 13:43:49 -0800 (PST)

> From: Holger Eitzenberger <holger@eitzenberger.org>
> Date: Tue, 23 Dec 2008 23:01:09 +0100
> 
> > -	skb = dev_alloc_skb(length);
> > -	if (!skb) {
> > +	skb = dev_alloc_skb(len);
> > +	if (skb == NULL)
> 
> "!skb" is preferred to the long-winded "skb == NULL", so
> I'm tossing this patch and the rest which touch similar
> areas.

Also, I want to mention that your C++ comment conversions are not in
the proper coding style as well.  You are doing:

	/* something written
	   here */

instead of the proper:

	/* something written
	 * here
	 */

Just FYI...
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
holger@eitzenberger.org - Dec. 29, 2008, 4:15 p.m.
On Fri, Dec 26, 2008 at 01:45:37PM -0800, David Miller wrote:

> > > -	skb = dev_alloc_skb(length);
> > > -	if (!skb) {
> > > +	skb = dev_alloc_skb(len);
> > > +	if (skb == NULL)
> > 
> > "!skb" is preferred to the long-winded "skb == NULL", so
> > I'm tossing this patch and the rest which touch similar
> > areas.
> 
> Also, I want to mention that your C++ comment conversions are not in
> the proper coding style as well.  You are doing:

As I'm currently offline most of the time I'll resend once I'm back
home.

Thanks for your feedback!

 /holger

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: bonding-2.6/drivers/net/bonding/bond_3ad.c
===================================================================
--- bonding-2.6.orig/drivers/net/bonding/bond_3ad.c
+++ bonding-2.6/drivers/net/bonding/bond_3ad.c
@@ -98,7 +98,6 @@  static const int ad_delta_in_ticks = (AD
 static const u8 lacpdu_mcast_addr[ETH_ALEN] = MULTICAST_LACPDU_ADDR;
 
 // ================= main 802.3ad protocol functions ==================
-static int ad_lacpdu_send(struct port *port);
 static int ad_marker_send(struct port *port, struct bond_marker *marker);
 static void ad_mux_machine(struct port *port);
 static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port);
@@ -820,17 +819,19 @@  static inline void __update_lacpdu_from_
  * Returns:   0 on success
  *          < 0 on error
  */
-static int ad_lacpdu_send(struct port *port)
+static int ad_lacpdu_send(const struct port *port)
 {
 	struct slave *slave = port->slave;
 	struct sk_buff *skb;
-	struct lacpdu_header *lacpdu_header;
-	int length = sizeof(struct lacpdu_header);
+	struct {
+		struct ethhdr hdr;
+		struct lacpdu lacpdu;
+	} __packed *pdu;
+	int len = sizeof(*pdu);
 
-	skb = dev_alloc_skb(length);
-	if (!skb) {
+	skb = dev_alloc_skb(len);
+	if (skb == NULL)
 		return -ENOMEM;
-	}
 
 	skb->dev = slave->dev;
 	skb_reset_mac_header(skb);
@@ -838,15 +839,15 @@  static int ad_lacpdu_send(struct port *p
 	skb->protocol = PKT_TYPE_LACPDU;
 	skb->priority = TC_PRIO_CONTROL;
 
-	lacpdu_header = (struct lacpdu_header *)skb_put(skb, length);
+	pdu = (void *)skb_put(skb, len);
 
-	memcpy(lacpdu_header->hdr.h_dest, lacpdu_mcast_addr, ETH_ALEN);
+	memcpy(pdu->hdr.h_dest, lacpdu_mcast_addr, ETH_ALEN);
 	/* Note: source addres is set to be the member's PERMANENT address,
 	   because we use it to identify loopback lacpdus in receive. */
-	memcpy(lacpdu_header->hdr.h_source, slave->perm_hwaddr, ETH_ALEN);
-	lacpdu_header->hdr.h_proto = PKT_TYPE_LACPDU;
+	memcpy(pdu->hdr.h_source, slave->perm_hwaddr, ETH_ALEN);
+	pdu->hdr.h_proto = PKT_TYPE_LACPDU;
 
-	lacpdu_header->lacpdu = port->lacpdu; // struct copy
+	memcpy(&pdu->lacpdu, &port->lacpdu, sizeof(pdu->lacpdu));
 
 	dev_queue_xmit(skb);
 
Index: bonding-2.6/drivers/net/bonding/bond_3ad.h
===================================================================
--- bonding-2.6.orig/drivers/net/bonding/bond_3ad.h
+++ bonding-2.6/drivers/net/bonding/bond_3ad.h
@@ -136,11 +136,6 @@  typedef struct lacpdu {
 	u8 reserved_50[50];	     // = 0
 } lacpdu_t;
 
-typedef struct lacpdu_header {
-	struct ethhdr hdr;
-	struct lacpdu lacpdu;
-} lacpdu_header_t;
-
 // Marker Protocol Data Unit(PDU) structure(43.5.3.2 in the 802.3ad standard)
 typedef struct bond_marker {
 	u8 subtype;		 //  = 0x02  (marker PDU)