[U-Boot] [PATCH 11/28] net: Refactor IP, UPD, and ICMP header writing functions
Simon Glass
sjg at chromium.org
Tue Jan 24 07:51:04 CET 2012
Hi Joe,
On Thu, Jan 19, 2012 at 4:53 PM, Joe Hershberger <joe.hershberger at ni.com> wrote:
> ICMP (ping) was reimplementing IP header code... it now shares code.
>
> 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 | 4 +++-
> net/bootp.c | 6 +++---
> net/net.c | 47 ++++++++++++++++++++++++++++-------------------
> net/ping.c | 52 +++++++++++++++++++++++-----------------------------
> 4 files changed, 57 insertions(+), 52 deletions(-)
To be honest these sorts of cleanup are great, but they are very hard
to review unless you know the code well. In particular your commit
message makes me imagine that you are cutting out duplicate code, but
it is changing a bit also.
>
> diff --git a/include/net.h b/include/net.h
> index add2080..19e9463 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -421,7 +421,9 @@ extern int NetEthHdrSize(void);
> extern int NetSetEther(uchar *, uchar *, uint);
>
> /* Set IP header */
> -extern void NetSetIP(uchar *, IPaddr_t, int, int, int);
> +extern void NetSetIPHeader(uchar *pkt, IPaddr_t dest, IPaddr_t source);
> +extern void NetSetUDPHeader(uchar *pkt, IPaddr_t dest, int dport,
> + int sport, int len);
>
> /* Checksum */
> extern int NetCksumOk(uchar *, int); /* Return true if cksum OK */
> diff --git a/net/bootp.c b/net/bootp.c
> index 2be8083..0c2af48 100644
> --- a/net/bootp.c
> +++ b/net/bootp.c
> @@ -619,7 +619,7 @@ BootpRequest(void)
> * determined.
> * C. Hallinan, DS4.COM, Inc.
> */
> - /* NetSetIP(pkt, 0xFFFFFFFFL, PORT_BOOTPS, PORT_BOOTPC,
> + /* NetSetUDPHeader(pkt, 0xFFFFFFFFL, PORT_BOOTPS, PORT_BOOTPC,
> sizeof (struct Bootp_t)); */
> iphdr = pkt; /* We need this later for NetSetUDPHeader() */
> pkt += IP_UDP_HDR_SIZE;
> @@ -663,7 +663,7 @@ BootpRequest(void)
> pktlen = ((int)(pkt-NetTxPacket)) + BOOTP_HDR_SIZE -
> sizeof(bp->bp_vend) + ext_len;
> iplen = BOOTP_HDR_SIZE - sizeof(bp->bp_vend) + ext_len;
> - NetSetIP(iphdr, 0xFFFFFFFFL, PORT_BOOTPS, PORT_BOOTPC, iplen);
> + NetSetUDPHeader(iphdr, 0xFFFFFFFFL, PORT_BOOTPS, PORT_BOOTPC, iplen);
> NetSetTimeout(SELECT_TIMEOUT, BootpTimeout);
>
> #if defined(CONFIG_CMD_DHCP)
> @@ -844,7 +844,7 @@ static void DhcpSendRequestPkt(struct Bootp_t *bp_offer)
> pktlen = ((int)(pkt-NetTxPacket)) + BOOTP_HDR_SIZE -
> sizeof(bp->bp_vend) + extlen;
> iplen = BOOTP_HDR_SIZE - sizeof(bp->bp_vend) + extlen;
> - NetSetIP(iphdr, 0xFFFFFFFFL, PORT_BOOTPS, PORT_BOOTPC, iplen);
> + NetSetUDPHeader(iphdr, 0xFFFFFFFFL, PORT_BOOTPS, PORT_BOOTPC, iplen);
>
> debug("Transmitting DHCPREQUEST packet: len = %d\n", pktlen);
> #ifdef CONFIG_BOOTP_DHCP_REQUEST_DELAY
> diff --git a/net/net.c b/net/net.c
> index 9bf74d5..a47b215 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -625,7 +625,7 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport,
> pkt = NetArpWaitTxPacket;
> pkt += NetSetEther(pkt, NetArpWaitPacketMAC, PROT_IP);
>
> - NetSetIP(pkt, dest, dport, sport, payload_len);
> + NetSetUDPHeader(pkt, dest, dport, sport, payload_len);
> memcpy(pkt + IP_UDP_HDR_SIZE, (uchar *)NetTxPacket +
> (pkt - (uchar *)NetArpWaitTxPacket) +
> IP_UDP_HDR_SIZE, payload_len);
> @@ -645,7 +645,7 @@ NetSendUDPPacket(uchar *ether, IPaddr_t dest, int dport, int sport,
>
> pkt = (uchar *)NetTxPacket;
> pkt += NetSetEther(pkt, ether, PROT_IP);
> - NetSetIP(pkt, dest, dport, sport, payload_len);
> + NetSetUDPHeader(pkt, dest, dport, sport, payload_len);
> eth_send(NetTxPacket, (pkt - NetTxPacket) + IP_UDP_HDR_SIZE +
> payload_len);
>
> @@ -1250,40 +1250,49 @@ NetSetEther(uchar *xet, uchar * addr, uint prot)
> }
>
> void
> -NetSetIP(uchar *xip, IPaddr_t dest, int dport, int sport, int len)
> +NetSetIPHeader(uchar *pkt, IPaddr_t dest, IPaddr_t source)
> {
> - struct IP_UDP_t *ip = (struct IP_UDP_t *)xip;
> + struct IP_UDP_t *ip = (struct IP_UDP_t *)pkt;
>
> /*
> - * If the data is an odd number of bytes, zero the
> - * byte after the last byte so that the checksum
> - * will work.
> - */
> - if (len & 1)
> - xip[IP_UDP_HDR_SIZE + len] = 0;
> -
> - /*
> - * Construct an IP and UDP header.
> - * (need to set no fragment bit - XXX)
> + * Construct an IP header.
> */
> /* IP_HDR_SIZE / 4 (not including UDP) */
> ip->ip_hl_v = 0x45;
> ip->ip_tos = 0;
> - ip->ip_len = htons(IP_UDP_HDR_SIZE + len);
> + ip->ip_len = htons(IP_HDR_SIZE);
> ip->ip_id = htons(NetIPID++);
> ip->ip_off = htons(IP_FLAGS_DFRAG); /* Don't fragment */
> ip->ip_ttl = 255;
> - ip->ip_p = 17; /* UDP */
> ip->ip_sum = 0;
> /* already in network byte order */
> - NetCopyIP((void *)&ip->ip_src, &NetOurIP);
> - /* - "" - */
> + NetCopyIP((void *)&ip->ip_src, &source);
> + /* already in network byte order */
> NetCopyIP((void *)&ip->ip_dst, &dest);
> +}
> +
> +void
> +NetSetUDPHeader(uchar *pkt, IPaddr_t dest, int dport, int sport, int len)
> +{
> + struct IP_UDP_t *ip = (struct IP_UDP_t *)pkt;
> +
> + /*
> + * If the data is an odd number of bytes, zero the
> + * byte after the last byte so that the checksum
> + * will work.
> + */
> + if (len & 1)
> + pkt[IP_UDP_HDR_SIZE + len] = 0;
> +
> + NetSetIPHeader(pkt, dest, NetOurIP);
> + ip->ip_len = htons(IP_UDP_HDR_SIZE + len);
> + ip->ip_p = IPPROTO_UDP;
> + ip->ip_sum = ~NetCksum((uchar *)ip, IP_HDR_SIZE >> 1);
> +
> ip->udp_src = htons(sport);
> ip->udp_dst = htons(dport);
> ip->udp_len = htons(UDP_HDR_SIZE + len);
> ip->udp_xsum = 0;
> - ip->ip_sum = ~NetCksum((uchar *)ip, IP_HDR_SIZE / 2);
> }
>
> void copy_filename(char *dst, const char *src, int size)
> diff --git a/net/ping.c b/net/ping.c
> index 34e287c..356f107 100644
> --- a/net/ping.c
> +++ b/net/ping.c
> @@ -16,11 +16,31 @@ static ushort PingSeqNo;
> /* The ip address to ping */
> IPaddr_t NetPingIP;
>
> +static void SetICMPHeader(uchar *pkt, IPaddr_t dest)
> +{
> + /*
> + * Construct an IP and ICMP header.
> + */
> + struct IP_UDP_t *ip = (struct IP_UDP_t *)pkt;
> + struct ICMP_t *icmp = (struct ICMP_t *)&ip->udp_src;
> +
> + NetSetIPHeader(pkt, dest, NetOurIP);
> +
> + ip->ip_len = htons(IP_ICMP_HDR_SIZE);
> + ip->ip_p = IPPROTO_ICMP;
> + ip->ip_sum = ~NetCksum((uchar *)ip, IP_HDR_SIZE >> 1);
Is >> 1 better than / 2 ?
> +
> + icmp->type = ICMP_ECHO_REQUEST;
> + icmp->code = 0;
> + icmp->checksum = 0;
> + icmp->un.echo.id = 0;
> + icmp->un.echo.sequence = htons(PingSeqNo++);
> + icmp->checksum = ~NetCksum((uchar *)icmp, ICMP_HDR_SIZE >> 1);
> +}
> +
> static int PingSend(void)
> {
> static uchar mac[6];
> - struct IP_UDP_t *ip;
> - ushort *s;
> uchar *pkt;
>
> /* XXX always send arp request */
> @@ -35,33 +55,7 @@ static int PingSend(void)
> pkt = NetArpWaitTxPacket;
> pkt += NetSetEther(pkt, mac, PROT_IP);
>
> - ip = (struct IP_UDP_t *)pkt;
> -
> - /*
> - * Construct an IP and ICMP header.
> - * (need to set no fragment bit - XXX)
> - */
> - /* IP_HDR_SIZE / 4 (not including UDP) */
> - ip->ip_hl_v = 0x45;
> - ip->ip_tos = 0;
> - ip->ip_len = htons(IP_HDR_SIZE + 8);
> - ip->ip_id = htons(NetIPID++);
> - ip->ip_off = htons(IP_FLAGS_DFRAG); /* Don't fragment */
> - ip->ip_ttl = 255;
> - ip->ip_p = 0x01; /* ICMP */
> - ip->ip_sum = 0;
> - /* already in network byte order */
> - NetCopyIP((void *)&ip->ip_src, &NetOurIP);
> - /* - "" - */
> - NetCopyIP((void *)&ip->ip_dst, &NetPingIP);
> - ip->ip_sum = ~NetCksum((uchar *)ip, IP_HDR_SIZE / 2);
> -
> - s = &ip->udp_src; /* XXX ICMP starts here */
> - s[0] = htons(0x0800); /* echo-request, code */
> - s[1] = 0; /* checksum */
> - s[2] = 0; /* identifier */
> - s[3] = htons(PingSeqNo++); /* sequence number */
> - s[1] = ~NetCksum((uchar *)s, 8/2);
> + SetICMPHeader(pkt, NetPingIP);
>
> /* size of the waiting packet */
> NetArpWaitTxPacketSize =
> --
> 1.6.0.2
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Regards,
Simon
More information about the U-Boot
mailing list