[U-Boot] [PATCH 10/28] net: cosmetic: Improve variable names and code readability
Simon Glass
sjg at chromium.org
Tue Jan 24 07:19:14 CET 2012
Hi Joe,
On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger <joe.hershberger at ni.com> wrote:
> Rename parameter len to payload_len in NetSendUDPPacket: this name
> more explicitly claims that it does not include the header size
> Rename CDPHandler to CDPReceive: this is not called as a handler,
> so don't name it that way
> Rename OPT_SIZE to OPT_FIELD_SIZE: clearer constant name and also
> remove related BOOTP_SIZE which was unused and doesn't take into
> account VLAN packets
> Rename tmp to reply_ip_addr in arp.c
> Alphabetize includes in net.c
> Replace magic numbers in arp.c with constants
> Add a more explicit comment about 802.2
Yes but why lump all of these together? It would benefit from 3-4
separate commits IMO.
Regards,
Simon
>
> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
> Cc: Joe Hershberger <joe.hershberger at gmail.com>
> Cc: Wolfgang Denk <wd at denx.de>
> ---
> include/net.h | 11 +++++++-
> net/arp.c | 38 ++++++++++++++++----------------
> net/bootp.c | 10 ++++----
> net/bootp.h | 7 ++---
> net/cdp.c | 2 +-
> net/cdp.h | 2 +-
> net/net.c | 67 +++++++++++++++++++++++++++++---------------------------
> 7 files changed, 73 insertions(+), 64 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index 9de1181..add2080 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -169,7 +169,8 @@ struct Ethernet_t {
> };
>
> #define ETHER_HDR_SIZE 14 /* Ethernet header size */
> -#define E802_HDR_SIZE 22 /* 802 ethernet header size */
> + /* 802.2 + SNAP + ethernet header size */
> +#define E802_HDR_SIZE (ETHER_HDR_SIZE + 8)
>
> /*
> * Ethernet header
> @@ -231,7 +232,9 @@ struct ARP_t {
> # define ARP_ETHER 1 /* Ethernet hardware address */
> ushort ar_pro; /* Format of protocol address */
> uchar ar_hln; /* Length of hardware address */
> +# define ARP_HLEN 6
> uchar ar_pln; /* Length of protocol address */
> +# define ARP_PLEN 4
> ushort ar_op; /* Operation */
> # define ARPOP_REQUEST 1 /* Request to resolve address */
> # define ARPOP_REPLY 2 /* Response to previous request */
> @@ -245,6 +248,10 @@ struct ARP_t {
> * specific hardware/protocol combinations.
> */
> uchar ar_data[0];
> +#define ar_sha ar_data[0]
> +#define ar_spa ar_data[ARP_HLEN]
> +#define ar_tha ar_data[ARP_HLEN + ARP_PLEN]
> +#define ar_tpa ar_data[ARP_HLEN + ARP_PLEN + ARP_HLEN]
> #if 0
> uchar ar_sha[]; /* Sender hardware address */
> uchar ar_spa[]; /* Sender protocol address */
> @@ -431,7 +438,7 @@ extern void NetSendPacket(uchar *, int);
>
> /* Transmit UDP packet, performing ARP request if needed */
> extern int NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport,
> - int sport, int len);
> + int sport, int payload_len);
>
> /* Processes a received packet */
> extern void NetReceive(volatile uchar *, int);
> diff --git a/net/arp.c b/net/arp.c
> index 96ffb85..456decd 100644
> --- a/net/arp.c
> +++ b/net/arp.c
> @@ -63,16 +63,16 @@ void ArpRequest(void)
>
> arp->ar_hrd = htons(ARP_ETHER);
> arp->ar_pro = htons(PROT_IP);
> - arp->ar_hln = 6;
> - arp->ar_pln = 4;
> + arp->ar_hln = ARP_HLEN;
> + arp->ar_pln = ARP_PLEN;
> arp->ar_op = htons(ARPOP_REQUEST);
>
> /* source ET addr */
> - memcpy(&arp->ar_data[0], NetOurEther, 6);
> + memcpy(&arp->ar_sha, NetOurEther, ARP_HLEN);
> /* source IP addr */
> - NetWriteIP((uchar *) &arp->ar_data[6], NetOurIP);
> + NetWriteIP(&arp->ar_spa, NetOurIP);
> /* dest ET addr = 0 */
> - memset(&arp->ar_data[10], '\0', 6);
> + memset(&arp->ar_tha, 0, ARP_HLEN);
> if ((NetArpWaitPacketIP & NetOurSubnetMask) !=
> (NetOurIP & NetOurSubnetMask)) {
> if (NetOurGatewayIP == 0) {
> @@ -85,7 +85,7 @@ void ArpRequest(void)
> NetArpWaitReplyIP = NetArpWaitPacketIP;
> }
>
> - NetWriteIP((uchar *) &arp->ar_data[16], NetArpWaitReplyIP);
> + NetWriteIP(&arp->ar_tpa, NetArpWaitReplyIP);
> (void) eth_send(NetTxPacket, (pkt - NetTxPacket) + ARP_HDR_SIZE);
> }
>
> @@ -116,7 +116,7 @@ void ArpTimeoutCheck(void)
> void ArpReceive(struct Ethernet_t *et, struct IP_UDP_t *ip, int len)
> {
> struct ARP_t *arp;
> - IPaddr_t tmp;
> + IPaddr_t reply_ip_addr;
> uchar *pkt;
>
> /*
> @@ -139,15 +139,15 @@ void ArpReceive(struct Ethernet_t *et, struct IP_UDP_t *ip, int len)
> return;
> if (ntohs(arp->ar_pro) != PROT_IP)
> return;
> - if (arp->ar_hln != 6)
> + if (arp->ar_hln != ARP_HLEN)
> return;
> - if (arp->ar_pln != 4)
> + if (arp->ar_pln != ARP_PLEN)
> return;
>
> if (NetOurIP == 0)
> return;
>
> - if (NetReadIP(&arp->ar_data[16]) != NetOurIP)
> + if (NetReadIP(&arp->ar_tpa) != NetOurIP)
> return;
>
> switch (ntohs(arp->ar_op)) {
> @@ -157,10 +157,10 @@ void ArpReceive(struct Ethernet_t *et, struct IP_UDP_t *ip, int len)
> pkt = (uchar *)et;
> pkt += NetSetEther(pkt, et->et_src, PROT_ARP);
> arp->ar_op = htons(ARPOP_REPLY);
> - memcpy(&arp->ar_data[10], &arp->ar_data[0], 6);
> - NetCopyIP(&arp->ar_data[16], &arp->ar_data[6]);
> - memcpy(&arp->ar_data[0], NetOurEther, 6);
> - NetCopyIP(&arp->ar_data[6], &NetOurIP);
> + memcpy(&arp->ar_tha, &arp->ar_sha, ARP_HLEN);
> + NetCopyIP(&arp->ar_tpa, &arp->ar_spa);
> + memcpy(&arp->ar_sha, NetOurEther, ARP_HLEN);
> + NetCopyIP(&arp->ar_spa, &NetOurIP);
> (void) eth_send((uchar *)et,
> (pkt - (uchar *)et) + ARP_HDR_SIZE);
> return;
> @@ -173,28 +173,28 @@ void ArpReceive(struct Ethernet_t *et, struct IP_UDP_t *ip, int len)
> #ifdef CONFIG_KEEP_SERVERADDR
> if (NetServerIP == NetArpWaitPacketIP) {
> char buf[20];
> - sprintf(buf, "%pM", arp->ar_data);
> + sprintf(buf, "%pM", arp->ar_sha);
> setenv("serveraddr", buf);
> }
> #endif
>
> - tmp = NetReadIP(&arp->ar_data[6]);
> + reply_ip_addr = NetReadIP(&arp->ar_spa);
>
> /* matched waiting packet's address */
> - if (tmp == NetArpWaitReplyIP) {
> + if (reply_ip_addr == NetArpWaitReplyIP) {
> debug("Got ARP REPLY, set eth addr (%pM)\n",
> arp->ar_data);
>
> /* save address for later use */
> memcpy(NetArpWaitPacketMAC,
> - &arp->ar_data[0], 6);
> + &arp->ar_sha, ARP_HLEN);
>
> #ifdef CONFIG_NETCONSOLE
> NetGetHandler()(0, 0, 0, 0, 0);
> #endif
> /* modify header, and transmit it */
> memcpy(((struct Ethernet_t *)NetArpWaitTxPacket)->
> - et_dest, NetArpWaitPacketMAC, 6);
> + et_dest, NetArpWaitPacketMAC, ARP_HLEN);
> (void) eth_send(NetArpWaitTxPacket,
> NetArpWaitTxPacketSize);
>
> diff --git a/net/bootp.c b/net/bootp.c
> index e95419f..2be8083 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -73,7 +73,7 @@ static int BootpCheckPkt(uchar *pkt, unsigned dest, unsigned src, unsigned len)
>
> if (dest != PORT_BOOTPC || src != PORT_BOOTPS)
> retval = -1;
> - else if (len < sizeof(struct Bootp_t) - OPT_SIZE)
> + else if (len < sizeof(struct Bootp_t) - OPT_FIELD_SIZE)
> retval = -2;
> else if (bp->bp_op != OP_BOOTREQUEST &&
> bp->bp_op != OP_BOOTREPLY &&
> @@ -369,8 +369,8 @@ static int DhcpExtended(u8 *e, int message_type, IPaddr_t ServerID,
>
> *e++ = 57; /* Maximum DHCP Message Size */
> *e++ = 2;
> - *e++ = (576 - 312 + OPT_SIZE) >> 8;
> - *e++ = (576 - 312 + OPT_SIZE) & 0xff;
> + *e++ = (576 - 312 + OPT_FIELD_SIZE) >> 8;
> + *e++ = (576 - 312 + OPT_FIELD_SIZE) & 0xff;
>
> if (ServerID) {
> int tmp = ntohl(ServerID);
> @@ -520,8 +520,8 @@ static int BootpExtended(u8 *e)
>
> *e++ = 57; /* Maximum DHCP Message Size */
> *e++ = 2;
> - *e++ = (576 - 312 + OPT_SIZE) >> 16;
> - *e++ = (576 - 312 + OPT_SIZE) & 0xff;
> + *e++ = (576 - 312 + OPT_FIELD_SIZE) >> 16;
> + *e++ = (576 - 312 + OPT_FIELD_SIZE) & 0xff;
> #endif
>
> #if defined(CONFIG_BOOTP_SUBNETMASK)
> diff --git a/net/bootp.h b/net/bootp.h
> index 1cf9a02..ecbcc4d 100644
> --- a/net/bootp.h
> +++ b/net/bootp.h
> @@ -20,13 +20,13 @@
> */
> #if defined(CONFIG_CMD_DHCP)
> /* Minimum DHCP Options size per RFC2131 - results in 576 byte pkt */
> -#define OPT_SIZE 312
> +#define OPT_FIELD_SIZE 312
> #if defined(CONFIG_BOOTP_VENDOREX)
> extern u8 *dhcp_vendorex_prep(u8 *e); /*rtn new e after add own opts. */
> extern u8 *dhcp_vendorex_proc(u8 *e); /*rtn next e if mine,else NULL */
> #endif
> #else
> -#define OPT_SIZE 64
> +#define OPT_FIELD_SIZE 64
> #endif
>
> struct Bootp_t {
> @@ -48,11 +48,10 @@ struct Bootp_t {
> uchar bp_chaddr[16]; /* Client hardware address */
> char bp_sname[64]; /* Server host name */
> char bp_file[128]; /* Boot file name */
> - char bp_vend[OPT_SIZE]; /* Vendor information */
> + char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */
> };
>
> #define BOOTP_HDR_SIZE sizeof(struct Bootp_t)
> -#define BOOTP_SIZE (ETHER_HDR_SIZE + IP_UDP_HDR_SIZE + BOOTP_HDR_SIZE)
>
> /**********************************************************************/
> /*
> diff --git a/net/cdp.c b/net/cdp.c
> index d617f18..38b79bd 100644
> --- a/net/cdp.c
> +++ b/net/cdp.c
> @@ -245,7 +245,7 @@ CDPDummyHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src,
> }
>
> void
> -CDPHandler(const uchar *pkt, unsigned len)
> +CDPReceive(const uchar *pkt, unsigned len)
> {
> const uchar *t;
> const ushort *ss;
> diff --git a/net/cdp.h b/net/cdp.h
> index fef744e..88191c4 100644
> --- a/net/cdp.h
> +++ b/net/cdp.h
> @@ -12,7 +12,7 @@
> #define __CDP_H__
>
> void CDPStart(void);
> -void CDPHandler(const uchar *pkt, unsigned len);
> +void CDPReceive(const uchar *pkt, unsigned len);
>
> #endif /* __CDP_H__ */
>
> diff --git a/net/net.c b/net/net.c
> index 023802d..9bf74d5 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -75,32 +75,32 @@
>
>
> #include <common.h>
> -#include <watchdog.h>
> #include <command.h>
> #include <net.h>
> -#include "arp.h"
> -#include "bootp.h"
> -#include "tftp.h"
> -#ifdef CONFIG_CMD_RARP
> -#include "rarp.h"
> -#endif
> -#include "nfs.h"
> -#ifdef CONFIG_STATUS_LED
> +#if defined(CONFIG_STATUS_LED)
> #include <status_led.h>
> #include <miiphy.h>
> #endif
> -#if defined(CONFIG_CMD_SNTP)
> -#include "sntp.h"
> -#endif
> +#include <watchdog.h>
> +#include "arp.h"
> +#include "bootp.h"
> #if defined(CONFIG_CMD_CDP)
> #include "cdp.h"
> #endif
> #if defined(CONFIG_CMD_DNS)
> #include "dns.h"
> #endif
> +#include "nfs.h"
> #if defined(CONFIG_CMD_PING)
> #include "ping.h"
> #endif
> +#if defined(CONFIG_CMD_RARP)
> +#include "rarp.h"
> +#endif
> +#if defined(CONFIG_CMD_SNTP)
> +#include "sntp.h"
> +#endif
> +#include "tftp.h"
>
> DECLARE_GLOBAL_DATA_PTR;
>
> @@ -598,7 +598,8 @@ NetSendPacket(uchar *pkt, int len)
> }
>
> int
> -NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len)
> +NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport,
> + int payload_len)
> {
> uchar *pkt;
>
> @@ -624,14 +625,14 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len)
> pkt = NetArpWaitTxPacket;
> pkt += NetSetEther(pkt, NetArpWaitPacketMAC, PROT_IP);
>
> - NetSetIP(pkt, dest, dport, sport, len);
> + NetSetIP(pkt, dest, dport, sport, payload_len);
> memcpy(pkt + IP_UDP_HDR_SIZE, (uchar *)NetTxPacket +
> (pkt - (uchar *)NetArpWaitTxPacket) +
> - IP_UDP_HDR_SIZE, len);
> + IP_UDP_HDR_SIZE, payload_len);
>
> /* size of the waiting packet */
> NetArpWaitTxPacketSize = (pkt - NetArpWaitTxPacket) +
> - IP_UDP_HDR_SIZE + len;
> + IP_UDP_HDR_SIZE + payload_len;
>
> /* and do the ARP request */
> NetArpWaitTry = 1;
> @@ -644,8 +645,9 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport, int len)
>
> pkt = (uchar *)NetTxPacket;
> pkt += NetSetEther(pkt, ether, PROT_IP);
> - NetSetIP(pkt, dest, dport, sport, len);
> - eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_UDP_HDR_SIZE + len);
> + NetSetIP(pkt, dest, dport, sport, payload_len);
> + eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_UDP_HDR_SIZE +
> + payload_len);
>
> return 0; /* transmitted */
> }
> @@ -859,9 +861,9 @@ NetReceive(volatile uchar *inpkt, int len)
> {
> struct Ethernet_t *et;
> struct IP_UDP_t *ip;
> - IPaddr_t tmp;
> + IPaddr_t dst_ip;
> IPaddr_t src_ip;
> - int x;
> + int eth_proto;
> #if defined(CONFIG_CMD_CDP)
> int iscdp;
> #endif
> @@ -896,20 +898,21 @@ NetReceive(volatile uchar *inpkt, int len)
> if (mynvlanid == (ushort)-1)
> mynvlanid = VLAN_NONE;
>
> - x = ntohs(et->et_protlen);
> + eth_proto = ntohs(et->et_protlen);
>
> debug("packet received\n");
>
> - if (x < 1514) {
> + if (eth_proto < 1514) {
> /*
> - * Got a 802 packet. Check the other protocol field.
> + * Got a 802.2 packet. Check the other protocol field.
> + * XXX VLAN over 802.2+SNAP not implemented!
> */
> - x = ntohs(et->et_prot);
> + eth_proto = ntohs(et->et_prot);
>
> ip = (struct IP_UDP_t *)(NetRxPacket + E802_HDR_SIZE);
> len -= E802_HDR_SIZE;
>
> - } else if (x != PROT_VLAN) { /* normal packet */
> + } else if (eth_proto != PROT_VLAN) { /* normal packet */
> ip = (struct IP_UDP_t *)(NetRxPacket + ETHER_HDR_SIZE);
> len -= ETHER_HDR_SIZE;
>
> @@ -932,17 +935,17 @@ NetReceive(volatile uchar *inpkt, int len)
>
> cti = ntohs(vet->vet_tag);
> vlanid = cti & VLAN_IDMASK;
> - x = ntohs(vet->vet_type);
> + eth_proto = ntohs(vet->vet_type);
>
> ip = (struct IP_UDP_t *)(NetRxPacket + VLAN_ETHER_HDR_SIZE);
> len -= VLAN_ETHER_HDR_SIZE;
> }
>
> - debug("Receive from protocol 0x%x\n", x);
> + debug("Receive from protocol 0x%x\n", eth_proto);
>
> #if defined(CONFIG_CMD_CDP)
> if (iscdp) {
> - CDPHandler((uchar *)ip, len);
> + CDPReceive((uchar *)ip, len);
> return;
> }
> #endif
> @@ -955,7 +958,7 @@ NetReceive(volatile uchar *inpkt, int len)
> return;
> }
>
> - switch (x) {
> + switch (eth_proto) {
>
> case PROT_ARP:
> ArpReceive(et, ip, len);
> @@ -994,10 +997,10 @@ NetReceive(volatile uchar *inpkt, int len)
> return;
> }
> /* If it is not for us, ignore it */
> - tmp = NetReadIP(&ip->ip_dst);
> - if (NetOurIP && tmp != NetOurIP && tmp != 0xFFFFFFFF) {
> + dst_ip = NetReadIP(&ip->ip_dst);
> + if (NetOurIP && dst_ip != NetOurIP && dst_ip != 0xFFFFFFFF) {
> #ifdef CONFIG_MCAST_TFTP
> - if (Mcast_addr != tmp)
> + if (Mcast_addr != dst_ip)
> #endif
> return;
> }
> --
> 1.6.0.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list