Re: tcp syn cache new and old inp variables

2016-06-27 Thread Jeremie Courreges-Anglas
Alexander Bluhm  writes:

> Hi,
>
> The variable swapping between inp, newinp and oldinpcb in syn_cache_get()
> is overly complicated.  Simplify the code without functional change.

This looks much less confusing indeed.  Note that this conflicts with
your last diff to fix inp_hops.

What I don't like is the introduction of ISSET() in this file.  Wouldn't
it be better to stay consistent?  Or is ISSET() considered an
improvement desirable enough to break consistency?

(I personnally don't find that ISSET() helps much, but YMMV).

> ok?

Looks correct and works fine here, ok jca@

> bluhm
>
> Index: netinet/tcp_input.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
> retrieving revision 1.319
> diff -u -p -r1.319 tcp_input.c
> --- netinet/tcp_input.c   9 Jun 2016 23:09:51 -   1.319
> +++ netinet/tcp_input.c   26 Jun 2016 23:47:27 -
> @@ -3627,7 +3627,7 @@ syn_cache_get(struct sockaddr *src, stru
>  {
>   struct syn_cache *sc;
>   struct syn_cache_head *scp;
> - struct inpcb *inp = NULL;
> + struct inpcb *inp, *oldinp;
>   struct tcpcb *tp = NULL;
>   struct mbuf *am;
>   int s;
> @@ -3670,7 +3670,8 @@ syn_cache_get(struct sockaddr *src, stru
>   if (so == NULL)
>   goto resetandabort;
>  
> - inp = sotoinpcb(oso);
> + oldinp = sotoinpcb(oso);
> + inp = sotoinpcb(so);
>  
>  #ifdef IPSEC
>   /*
> @@ -3678,30 +3679,18 @@ syn_cache_get(struct sockaddr *src, stru
>* from the old pcb. Ditto for any other
>* IPsec-related information.
>*/
> - {
> -   struct inpcb *newinp = sotoinpcb(so);
> -   memcpy(newinp->inp_seclevel, inp->inp_seclevel,
> -   sizeof(inp->inp_seclevel));
> - }
> + memcpy(inp->inp_seclevel, oldinp->inp_seclevel,
> + sizeof(oldinp->inp_seclevel));
>  #endif /* IPSEC */
>  #ifdef INET6
>   /*
>* inp still has the OLD in_pcb stuff, set the
>* v6-related flags on the new guy, too.
>*/
> - {
> -   int flags = inp->inp_flags;
> -   struct inpcb *oldinpcb = inp;
> -
> -   inp = sotoinpcb(so);
> -   inp->inp_flags |= (flags & INP_IPV6);
> -   if ((inp->inp_flags & INP_IPV6) != 0) {
> - inp->inp_ipv6.ip6_hlim =
> -   oldinpcb->inp_ipv6.ip6_hlim;
> -   }
> + inp->inp_flags |= (oldinp->inp_flags & INP_IPV6);
> + if (ISSET(inp->inp_flags, INP_IPV6)) {
> + inp->inp_ipv6.ip6_hlim = oldinp->inp_ipv6.ip6_hlim;
>   }
> -#else /* INET6 */
> - inp = sotoinpcb(so);
>  #endif /* INET6 */
>  
>  #if NPF > 0
>

-- 
jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE



tcp syn cache new and old inp variables

2016-06-26 Thread Alexander Bluhm
Hi,

The variable swapping between inp, newinp and oldinpcb in syn_cache_get()
is overly complicated.  Simplify the code without functional change.

ok?

bluhm

Index: netinet/tcp_input.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_input.c,v
retrieving revision 1.319
diff -u -p -r1.319 tcp_input.c
--- netinet/tcp_input.c 9 Jun 2016 23:09:51 -   1.319
+++ netinet/tcp_input.c 26 Jun 2016 23:47:27 -
@@ -3627,7 +3627,7 @@ syn_cache_get(struct sockaddr *src, stru
 {
struct syn_cache *sc;
struct syn_cache_head *scp;
-   struct inpcb *inp = NULL;
+   struct inpcb *inp, *oldinp;
struct tcpcb *tp = NULL;
struct mbuf *am;
int s;
@@ -3670,7 +3670,8 @@ syn_cache_get(struct sockaddr *src, stru
if (so == NULL)
goto resetandabort;
 
-   inp = sotoinpcb(oso);
+   oldinp = sotoinpcb(oso);
+   inp = sotoinpcb(so);
 
 #ifdef IPSEC
/*
@@ -3678,30 +3679,18 @@ syn_cache_get(struct sockaddr *src, stru
 * from the old pcb. Ditto for any other
 * IPsec-related information.
 */
-   {
- struct inpcb *newinp = sotoinpcb(so);
- memcpy(newinp->inp_seclevel, inp->inp_seclevel,
- sizeof(inp->inp_seclevel));
-   }
+   memcpy(inp->inp_seclevel, oldinp->inp_seclevel,
+   sizeof(oldinp->inp_seclevel));
 #endif /* IPSEC */
 #ifdef INET6
/*
 * inp still has the OLD in_pcb stuff, set the
 * v6-related flags on the new guy, too.
 */
-   {
- int flags = inp->inp_flags;
- struct inpcb *oldinpcb = inp;
-
- inp = sotoinpcb(so);
- inp->inp_flags |= (flags & INP_IPV6);
- if ((inp->inp_flags & INP_IPV6) != 0) {
-   inp->inp_ipv6.ip6_hlim =
- oldinpcb->inp_ipv6.ip6_hlim;
- }
+   inp->inp_flags |= (oldinp->inp_flags & INP_IPV6);
+   if (ISSET(inp->inp_flags, INP_IPV6)) {
+   inp->inp_ipv6.ip6_hlim = oldinp->inp_ipv6.ip6_hlim;
}
-#else /* INET6 */
-   inp = sotoinpcb(so);
 #endif /* INET6 */
 
 #if NPF > 0