> On 7 Aug 2022, at 23:18, Alexander Bluhm <[email protected]> wrote:
>
> On Sun, Aug 07, 2022 at 03:53:27AM +0300, Vitaliy Makkoveev wrote:
>> Please note, the `so_pcb' can't be NULL. We nullify it only on dead
>> socket, which should not be passed to (*pr_userreq)(). The newly created
>> socket has `so_pcb' set to NULL only until we pass it to (*pr_attach)()
>> and we don't use sockets if attach failed. So I use KASSERT() instead of
>> pcb != NULL check.
>
> For TCP this is not true. A reset packet in tcp_input() calls
> tcp_drop() -> tcp_close() -> in_pcbdetach() -> so->so_pcb = NULL
> This cannot happen in any TCP state. But KASSERT(inp != NULL) in
> tcp_disconnect() wrong. On the other hand if (tp == NULL) is not
> necessary everywhere.
Thanks, will fix it.
>
>> - if (error)
>> - break;
>
>> + if (error != 0)
>> + return (error);
>
> I prefer the first idiom. If there is an error, do something. We
> should not change the style in opposite direction. This will prevent
> consistency.
>
I’m not entirely understand you. When we have something to do in the
error path, I use "goto out” and it works like the first idiom.
+int
+tcp_connect(struct socket *so, struct mbuf *nam, struct proc *p)
+{
[ skip ]
+#ifdef INET6
+ if (inp->inp_flags & INP_IPV6) {
+ struct sockaddr_in6 *sin6;
+
+ if ((error = in6_nam2sin6(nam, &sin6)))
+ goto out;
+ if (IN6_IS_ADDR_UNSPECIFIED(&sin6->sin6_addr) ||
+ IN6_IS_ADDR_MULTICAST(&sin6->sin6_addr)) {
+ error = EINVAL;
+ goto out;
+ }
+ error = in6_pcbconnect(inp, nam);
+ } else
+#endif /* INET6 */
+ {
+ struct sockaddr_in *sin;
+
+ if ((error = in_nam2sin(nam, &sin)))
+ goto out;
+ if ((sin->sin_addr.s_addr == INADDR_ANY) ||
+ (sin->sin_addr.s_addr == INADDR_BROADCAST) ||
+ IN_MULTICAST(sin->sin_addr.s_addr) ||
+ in_broadcast(sin->sin_addr, inp->inp_rtableid)) {
+ error = EINVAL;
+ goto out;
+ }
+ error = in_pcbconnect(inp, nam);
+ }
+ if (error)
+ goto out;
+
[ skip ]
+
+out:
+ if (otp)
+ tcp_trace(TA_USER, ostate, tp, otp, NULL, PRU_CONNECT, 0);
+
+ return (error);
+}
But we also have the error paths where we have nothing to do. Do you
mean you prefer goto instead of return in the cases like this?
+int
+rip6_connect(struct socket *so, struct mbuf *nam, struct proc *p)
+{
[ skip ]
+ error = in6_nam2sin6(nam, &addr);
+ if (error != 0)
+ return (error);
+ /* Source address selection. XXX: need pcblookup? */
+ error = in6_pcbselsrc(&in6a, addr, in6p, in6p->inp_outputopts6);
+ if (error != 0)
+ return (error);
+
+ in6p->inp_laddr6 = *in6a;
+ in6p->inp_faddr6 = addr->sin6_addr;
+ soisconnected(so);
+
+ return (0);
+}
+int
+rip6_connect(struct socket *so, struct mbuf *nam, struct proc *p)
+{
[ skip ]
+ error = in6_nam2sin6(nam, &addr);
+ if (error != 0)
+ goto out;
+ /* Source address selection. XXX: need pcblookup? */
+ error = in6_pcbselsrc(&in6a, addr, in6p, in6p->inp_outputopts6);
+ if (error != 0)
+ goto out;
+
+ in6p->inp_laddr6 = *in6a;
+ in6p->inp_faddr6 = addr->sin6_addr;
+ soisconnected(so);
+
+out:
+ return (error);
+}
The error path of the original *_usrreq() is commonn for all commands
and contains mbufs release, because they could be wrongly passed for
all commands, so we silently release them to prevent memory leak:
foo_usrreq()
{
switch (req) {
case PRU_ACCEPT:
error = EOPNOTSUPP;
break;
}
if (req != PRU_RCVD && req != PRU_RCVOOB && req != PRU_SENSE) {
m_freem(control);
m_freem(m);
}
return (error);
}
But in this diff such mbufs can't be passed to the wrong handlers. So
in the cases like rip6_connect() the first idiom looks strange.
