On do, 24 jan 2019 09:24:06 +0100, Oleksij Rempel wrote:
>
> On 23.01.19 20:30, Kurt Van Dijck wrote:
> >On wo, 23 jan 2019 13:01:11 +0100, Oleksij Rempel wrote:
> >>This is dangerous behavior, especially if it is done by default. If we
> >>still needed it, it can be added after upstreaming.
> >
> >In a true (?) j1939 program, you don't care about the precise packet size
> >you're sending. If you then send 2 subsequent packets, 1 with TP and 1
> >without, then the 2nd will likely arrive sooner than the first, which
> >is probably not what you want.
> >
> >The piece of code you're about to remove increments will set
> >jsk->skb_pending only if it was 0, atomically.
> >If it wasn't 0, then send() will block or fail.
> >
> >Can you explain how this is dangerous?
> >
> >IMHO, it is not done by default, but ony when MSG_SYN is given during send().
> >How do you think that synchronous send is done by default?
>
> With the code you are describing, the behavior of send() is broken.
>
> a sequence of send call will be pushed to the bus in unpredictable order.
> And to get it in predicable order we should run in full blocking mode.
This part
> >>- ret = wait_event_interruptible(jsk->waitq,
> >>- j1939_sock_pending_add_first(&jsk->sk));
implements the 'true blocking' mode, it waits until the previous one has
been sent.
MSG_DONTWAIT returns -EAGAIN if it is not first
or at least, it should behave so.
Looking at the code, it seems that the latest version has the check
inverted.
> >>- if (msg->msg_flags & MSG_DONTWAIT) {
> >>- ret = j1939_sock_pending_add_first(&jsk->sk);
> >>- if (ret > 0)
> >>- ret = -EAGAIN;
Strange, it once worked. I'm not in the possibility to verify it.
>
> IMHO, proper way:
> - implement internal queue to guarantee the order of packets within one socket.
I learned this thing: avoid to be intelligent in kernel. do exactly what
you're asked for and fail if the slightest thing go wrong, so userspace
can do something smart with it.
> - if packet should be run without order of with different PRIO, send it over different socket.
True.
It makes little sense to mix MSG_SYN with !MSG_SYN on 1 socket,
but 'not being smart' isn't a real argument to forbid it either.
Kind regards,
Kurt
