Re: [PATCH v2 06/14] net-lwip: add ping command

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

> +#include <net-lwip.h>
> +#include <time.h>
> +
> +#define PING_DELAY_MS 1000
> +#define PING_TIMEOUT_MS 10000
> +/* Additional data size to include in the packet */
> +#define PING_DATA_SIZE 32
> +/* Ping identifier - must fit on a u16_t */
> +#define PING_ID 0xAFAF
> +
> +static const ip_addr_t *ping_target;
> +static struct raw_pcb *ping_pcb;
> +static u16_t ping_seq_num;

As a general note, u8_t u16_t etc are lwip constructs.
Can we not use them and instead use the original definition?
uint8_t etc. I am not sure introducing another define is what we want. Tom?

> +static bool ping_target_alive;
> +
> +static u8_t ping_recv(void *arg, struct raw_pcb *pcb, struct pbuf *p,
> +		      const ip_addr_t *addr)
> +{
> +	struct icmp_echo_hdr *iecho;
> +
> +	if (addr->addr != ping_target->addr)
> +		return 0;
> +
> +	if ((p->tot_len >= (IP_HLEN + sizeof(struct icmp_echo_hdr))) &&
> +	    pbuf_remove_header(p, IP_HLEN) == 0) {
> +		iecho = (struct icmp_echo_hdr *)p->payload;
> +
> +		if ((iecho->id == PING_ID) &&
> +		    (iecho->seqno == lwip_htons(ping_seq_num))) {
> +			ping_target_alive = true;
> +			printf("host %s is alive\n", ipaddr_ntoa(addr));
> +			pbuf_free(p);
> +			return 1; /* eat the packet */
> +		}
> +		/* not eaten, restore original packet */
> +		pbuf_add_header(p, IP_HLEN);
> +	}
> +
> +	return 0; /* don't eat the packet */
> +}
> +
> +static int ping_raw_init(void)
> +{
> +	ping_pcb = raw_new(IP_PROTO_ICMP);
> +	if (!ping_pcb)
> +		return -ENOMEM;
> +
> +	raw_recv(ping_pcb, ping_recv, NULL);

Instead of defining a global variable ping_target_alive can we instead pass
the ptr of void *recv_arg? We can then fill in that private ptr with the
status

> +	raw_bind(ping_pcb, IP_ADDR_ANY);
> +
> +	return 0;
> +}
> +
> +static void ping_raw_stop(void)
> +{
> +	if (ping_pcb != NULL) {
> +		raw_remove(ping_pcb);
> +		ping_pcb = NULL;
> +	}
> +}
> +
> +static void ping_prepare_echo(struct icmp_echo_hdr *iecho, u16_t len)
> +{
> +	size_t i;
> +	size_t data_len = len - sizeof(struct icmp_echo_hdr);
> +
> +	ICMPH_TYPE_SET(iecho, ICMP_ECHO);
> +	ICMPH_CODE_SET(iecho, 0);
> +	iecho->chksum = 0;
> +	iecho->id = PING_ID;
> +	iecho->seqno = lwip_htons(++ping_seq_num);
> +
> +	/* Fill the additional data buffer with some data */
> +	for(i = 0; i < data_len; i++) {
> +		((char *)iecho)[sizeof(struct icmp_echo_hdr) + i] = (char)i;
> +	}

is this additional data used? And if they are shouldn't we care about
endianess?

> +
> +	iecho->chksum = inet_chksum(iecho, len);
> +}
> +
> +static void ping_send_icmp(struct raw_pcb *raw, const ip_addr_t *addr)
> +{
> +	struct pbuf *p;
> +	struct icmp_echo_hdr *iecho;
> +	size_t ping_size = sizeof(struct icmp_echo_hdr) + PING_DATA_SIZE;
> +
> +	p = pbuf_alloc(PBUF_IP, (u16_t)ping_size, PBUF_RAM);
> +	if (!p)
> +		return;
> +
> +	if ((p->len == p->tot_len) && (p->next == NULL)) {
> +		iecho = (struct icmp_echo_hdr *)p->payload;
> +		ping_prepare_echo(iecho, (u16_t)ping_size);
> +		raw_sendto(raw, p, addr);
> +	}
> +
> +	pbuf_free(p);
> +}
> +
> +static void ping_send(void *arg)
> +{
> +	struct raw_pcb *pcb = (struct raw_pcb *)arg;
> +
> +	ping_send_icmp(pcb, ping_target);
> +	sys_timeout(PING_DELAY_MS, ping_send, ping_pcb);
> +}
> +
> +static int ping_loop(const ip_addr_t* addr)
> +{
> +	ulong start;
> +	int ret;
> +
> +	printf("Using %s device\n", eth_get_name());
> +
> +	ret = ping_raw_init();
> +	if (ret < 0)
> +		return ret;
> +	ping_target = addr;
> +
> +	start = get_timer(0);
> +	ping_send(ping_pcb);
> +
> +	do {
> +		eth_rx();

eth_rx() has a ret value. Don't we have to check it here?

> +		if (ping_target_alive)
> +			break;
> +		sys_check_timeouts();
> +		if (ctrlc()) {
> +			printf("\nAbort\n");
> +			break;
> +		}
> +	} while (get_timer(start) < PING_TIMEOUT_MS);
> +
> +	sys_untimeout(ping_send, ping_pcb);
> +	ping_raw_stop();
> +	ping_target = NULL;
> +
> +	if (ping_target_alive) {
> +		ping_target_alive = false;
> +		return 0;
> +	}
> +	printf("ping failed; host %s is not alive\n", ipaddr_ntoa(addr));
> +	return -1;
> +}
> +
> +int do_ping(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	ip_addr_t addr;
> +
> +	if (argc < 2)
> +		return CMD_RET_USAGE;
> +
> +	if (!ipaddr_aton(argv[1], &addr))
> +		return CMD_RET_USAGE;
> +
> +	if (ping_loop(&addr) < 0)
> +		return CMD_RET_FAILURE;
> +
> +	return CMD_RET_SUCCESS;
> +}
> --
> 2.40.1
>

Regards
/Ilias