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

[Jerome Forissier]

On 6/6/24 11:10, Ilias Apalodimas wrote:
>> +#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?

I think it makes sense to use the more standard type (uint16_t etc.) when
defining variables like here. However I'd rather keep the lwIP types in
the function definitions to reflect the function prototype exactly.

>> +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

Sure. Fixed in v3.
 
>> +	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?

It's a random payload for the ICMP packet. TBH it's totally useless here,
I will just remove the additional data in v3.

> 
>> +
>> +	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?

We would not do anything different on error because an error
might be transient, we don't want too much complexity. Letting the
loop run until success (which is determined by the proper ICMP
echo reply being received) or timeout or Ctrl-C is good enough.

BTW net_loop() in net/net.c doesn't check the return status,
either.

> 
>> +		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