RE: [PATCH net] sctp: fix a success return may hide an error

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: Xin Long
> Sent: 13 August 2016 08:48
> >
> > This style of error handling is dangerous.  The first error can be
> > lost.
> >
> > For example, if sctp_outq_flush_rtx() earlier in this function returns
> > an error, it will be lost if any invocation of the function
> > sctp_packet_transmit() at the end function signals an error.
> >
> > I think you should always preserve the first error that is recorded
> > into 'error'.
> >
> > I also wonder about why sctp_outq_flush_rtx() errors are completely
> > ignored and don't influence the control flow here in any way.
> 
> Yes, the first error can be lost.
> Here we just keep the last error. We don't really have to return the
> first error or return it on the first failure.
> 
> [1]
> Both sctp_outq_flush_rtx and sctp_packet_transmit can ONLY
> return one error (-ENOMEM), as sctp_outq_flush_rtx also calls
> sctp_packet_transmit.

What is the effect of the error?
If it is 'just' equivalent to a lost ethernet packet (and the skb (etc)
is freed) then the protocol will recover.
If it is anything else then the error path is probably wrong.

Also after one error is it actually worth trying to send anything else
at all? ISTM that the code should either:
1) wait for resources and retry.
2) discard the entire queue (freeing resource) and hope the protocol
   timers will recover.

> [2]
> It's the original codes that it doesn't return immediately when
> sctp_outq_flush_rtx returns error. I guess it just doesn't want
> to stop flushing out transport_list only because it fail to flush
> rtx.
> even sctp_packet_transmit_chunk in sctp_outq_flush also just
> put the error into sk->sk_err, instread of returning immediately.
> 
> So we cannot return the err at the first failure as [2], the error
> here is always -ENOMEM as [1].
> I think to return the last error here is ok, at least  not dangerous,
> can also fix the issue "a success return may hide an error" with
> clear codes. :)

Which code looks at sk->sk_err?
It doesn't look right to be setting an error code on the socket due
a transmit packet discard.

	David

��.n��������+%������w��{.n�����{������ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Linux Networking Development]     [Linux OMAP]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux