Re: [PATCH v3 05/12] net-lwip: add ping command

[Jerome Forissier <jerome.forissier@linaro.org>]

On 6/6/24 19:45, Ilias Apalodimas wrote:
> On Thu, 6 Jun 2024 at 20:02, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> On Thu, 6 Jun 2024 at 19:53, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>>
>>> [...]
>>>
>>>> +static int ping_raw_init(void *recv_arg)
>>>> +{
>>>> +       ping_pcb = raw_new(IP_PROTO_ICMP);
>>>> +       if (!ping_pcb)
>>>> +               return -ENOMEM;
>>>> +
>>>> +       raw_recv(ping_pcb, ping_recv, recv_arg);
>>>> +       raw_bind(ping_pcb, IP_ADDR_ANY);
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>> +static void ping_raw_stop(void)
>>>> +{
>>>> +       if (ping_pcb != NULL) {
>>>
>>> nits, but we usually do if (!ping_pcb) for NULL pointers and variables
>>> that have a value of 0.
>>> Please change it in other files as well
>>>
>>>
>>>> +               raw_remove(ping_pcb);
>>>> +               ping_pcb = NULL;
>>>> +       }
>>>> +}
>>>> +
>>>> +static void ping_prepare_echo(struct icmp_echo_hdr *iecho)
>>>> +{
>>>> +       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);
>>>> +
>>>> +       iecho->chksum = inet_chksum(iecho, sizeof(*iecho));
>>>> +}
>>>> +
>>>> +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);
>>>> +
>>>> +       p = pbuf_alloc(PBUF_IP, (u16_t)ping_size, PBUF_RAM);
>>>> +       if (!p)
>>>> +               return;
>>>> +
>>>> +       if ((p->len == p->tot_len) && (p->next == NULL)) {
>>>
>>> && !p->next
>>>
>>>> +               iecho = (struct icmp_echo_hdr *)p->payload;
>>>> +               ping_prepare_echo(iecho);
>>>> +               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)
>>>> +{
>>>> +       bool alive;
>>>> +       ulong start;
>>>> +       int ret;
>>>> +
>>>> +       printf("Using %s device\n", eth_get_name());
>>>> +
>>>> +       ret = ping_raw_init(&alive);
>>>> +       if (ret < 0)
>>>> +               return ret;
>>>> +       ping_target = addr;
>>>> +       ping_seq_num = 0;
>>>> +
>>>> +       start = get_timer(0);
>>>> +       ping_send(ping_pcb);
>>>> +
>>>> +       do {
>>>> +               eth_rx();
>>>> +               if (alive)
>>>> +                       break;
>>>> +               sys_check_timeouts();
>>>> +               if (ctrlc()) {
>>>> +                       printf("\nAbort\n");
>>>> +                       break;
>>>> +               }
>>>> +       } while (get_timer(start) < PING_TIMEOUT_MS);
>>>
>>> I am a bit confused about what happens here.
>>> ping_send() will send the packet, but it will also schedule itself to
>>> rerun after 1 ms and send another ping?
>>>
>>>> +
>>>> +       sys_untimeout(ping_send, ping_pcb);
>>>
>>> So we need the sys_untimeout() because we queued 2 pings? Because
>>> sys_timeout() is supposed to be an one shot.
>>
>> Ah nvm, I misread that. We always reschedule ping_send_icmp(), that's
>> why we have to delete it here
> 
> Ok so looking at it a bit more. Why do we have to schedule contunuous pings?
> We just have to send one packet and wait for the response no?

We could send just one packet as NET does, but if there's packet loss then
the ping command will report the host is unreachable. I think it is safer
to allow for a few retries for better reliability. The code for one packet
would be marginally simpler anyways. What I can do however is use a send
counter rather than a timeout.

Thanks,
-- 
Jerome