Re: [PATCH] can: isotp: return -ECOMM on FC timeout on TX path

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 28.04.21 11:26, Oliver Hartkopp wrote:
> On 28.04.21 11:12, Marc Kleine-Budde wrote:
>> On 28.04.2021 11:09:14, Marc Kleine-Budde wrote:

>>> @@ -756,6 +762,10 @@ static enum hrtimer_restart 
>>> isotp_tx_timer_handler(struct hrtimer *hrtimer)
>>
>>>                sk->sk_err = ECOMM;
>>>           if (!sock_flag(sk, SOCK_DEAD))
>>>               sk->sk_error_report(sk);
>>
>> I think the idea is that sk_error_report takes care of propagation of
>> the error to the user space. I don't know why it's not working as
>> expected.
> 
> Yes, I fact I was thinking about this question too.
> 
>>
>>> +        /* set error flag in order to consume it later in the
>>> +         * isotp_sendmsg function */
>>> +        so->tx.error = ISOTP_ERR_FC_TIMEOUT;
>>> +
>>>           /* reset tx state */
>>>           so->tx.state = ISOTP_IDLE;
>>>           wake_up_interruptible(&so->wait);
>>> @@ -954,6 +964,13 @@ static int isotp_sendmsg(struct socket *sock, 
>>> struct msghdr *msg, size_t size)
>>>       if (wait_tx_done) {
>>>           /* wait for complete transmission of current pdu */
>>>           wait_event_interruptible(so->wait, so->tx.state == 
>>> ISOTP_IDLE);
>>> +
>>> +        /* check if an error has been raised in the timer
>>> +         * function handler */
>>> +        if (so->tx.error == ISOTP_ERR_FC_TIMEOUT) {
>>> +            so->tx.error = ISOTP_ERR_NO_ERROR;
>>> +            return -ECOMM;
> 
> Maybe the way to trigger the sk_error_report(sk) we might return '-1' 
> while the error is then propagated inside errno.

I added some debug print in isotpsend:

diff --git a/isotpsend.c b/isotpsend.c
index 3ea574c..c2937fa 100644
--- a/isotpsend.c
+++ b/isotpsend.c
@@ -45,10 +45,11 @@
  #include <libgen.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <string.h>
  #include <unistd.h>
+#include <errno.h>

  #include <net/if.h>
  #include <sys/ioctl.h>
  #include <sys/socket.h>
  #include <sys/types.h>
@@ -252,10 +253,11 @@ int main(int argc, char **argv)
                     buf[buflen] = ((buflen % 0xFF) + 1) & 0xFF;
      }


      retval = write(s, buf, buflen);
+    printf("retval %d errno %d\n", retval, errno);
      if (retval < 0) {
             perror("write");
             return retval;
      }


$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
43.269173590
retval 44 errno 0
44.271162277

So it waits for the timeout as required by the '-b' option - but the 
errno is not set :-/

> 
> But I haven't checked that myself so far.
> 
> The reason why this return value is not that important is that ISO-TP is 
> an (unreliable!) UDP like protocol where the application checks for an 
> application response timeout.
> 
> Having the information of a FF timeout only makes sense when 
> CAN_ISOTP_WAIT_TX_DONE is enabled to assign the timeout to a that 
> specific PDU.
> 
> Best,
> Oliver
> 
>>> +        }
>>>       }
>>>       return size;
>>> @@ -1371,6 +1388,9 @@ static int isotp_init(struct sock *sk)
>>>       so->tx.state = ISOTP_IDLE;
>>>       hrtimer_init(&so->rxtimer, CLOCK_MONOTONIC, 
>>> HRTIMER_MODE_REL_SOFT);
>>> +
>>> +    so->tx.error = ISOTP_ERR_NO_ERROR;
>>> +    so->rx.error = ISOTP_ERR_NO_ERROR;
>>>       so->rxtimer.function = isotp_rx_timer_handler;
>>>       hrtimer_init(&so->txtimer, CLOCK_MONOTONIC, 
>>> HRTIMER_MODE_REL_SOFT);
>>>       so->txtimer.function = isotp_tx_timer_handler;
>>
>>
>> Marc
>>