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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 28.04.21 12:47, Marc Kleine-Budde wrote:
> On 28.04.2021 12:24:32, Oliver Hartkopp wrote:
>>> 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);
> 
> Note: in user space errno is only valid if retval is "-1"...

Ok, just returned '-1' in that case of blocking write (which runs into 
the timeout) ...

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 9f94ad3caee9..d5d541b4fed5 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -952,10 +952,11 @@ 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);
+               return -1;
         }

         return size;
  }

And now got this:

$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
22.411570468
retval -1 errno 1
write: Operation not permitted

So still nothing to see from "sk->sk_err = ECOMM;"

But when adding 'return -ECOMM' at the above section it works like this:

$ date +%S.%N && ./isotpsend vcan0 -s 123 -d 321 -D 44 -b && date +%S.%N
58.453452222
retval -1 errno 70
write: Communication error on send

This is obviously the expected behaviour for user space:

- get '-1' in the case of socket related errors
- get the details from errno

Maybe all that sk_err stuff is only relevant for listen/accept 
constructions for connection oriented sockets and can be removed inside 
isotp.c ...

Regards,
Oliver

> 
>>       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 :-/
> 
> I'm not sure what we have to return in the kernel...
> 
> Marc
>