Re: [PATCH] ipvs: Fix reuse connection if real server is dead

[Julian Anastasov <ja@xxxxxx>]

        Hello,

On Sat, 11 May 2013, Grzegorz Łyczba wrote:

> From: Grzegorz Lyczba <grzegorz.lyczba@xxxxxxxxx>
> 
> Expire cached connection for new TCP/SCTP connection if real server is down
> 
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@xxxxxxxxx>

        The change looks correct but you have to fix some
coding style issues. Otherwise, patch will be rejected.
You can also check Documentation/CodingStyle

# scripts/checkpatch.pl /tmp/patch.diff
WARNING: braces {} are not necessary for single statement blocks
#25: FILE: net/netfilter/ipvs/ip_vs_core.c:1012:
+               if (th == NULL) {
+                       return false;
+               }

WARNING: braces {} are not necessary for single statement blocks
#34: FILE: net/netfilter/ipvs/ip_vs_core.c:1021:
+               if (sch == NULL) {
+                       return false;
+               }

total: 0 errors, 2 warnings, 48 lines checked

> ---
>  net/netfilter/ipvs/ip_vs_core.c |   36 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..9ee8b70 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff 
> *skb, int nh_len)
>       return th->rst;
>  }
>  
> +static inline bool is_new_conn(const struct sk_buff *skb,
> +         struct ip_vs_iphdr *iph)

        Above "struct" should be at the same column as "const"
in first argument, like this:

static inline bool is_new_conn(const struct sk_buff *skb,
                               struct ip_vs_iphdr *iph)

> +{
> +     switch (iph->protocol) {

        I'm still not sure if this construct is allowed:

        case ...: {
        }

        May be it should be:

        case ...:
                {
                }
        case ...

        but CodingStyle does not include rules about it.
If in doubt, you can use 'if' instead of 'switch' without
adding new indentation.

> +     case IPPROTO_TCP: {
> +             struct tcphdr _tcph, *th;

        empty line is needed here

> +             th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
> +             if (th == NULL) {
> +                     return false;

        extra braces

> +             }
> +             return th->syn;
> +     }
> +     case IPPROTO_SCTP: {
> +             sctp_chunkhdr_t *sch, schunk;

        empty line is needed here

> +             sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> +                             sizeof(schunk), &schunk);
> +             if (sch == NULL) {
> +                     return false;

        extra braces

> +             }
> +             return (sch->type == SCTP_CID_INIT);
> +     }
> +     default:
> +             return false;
> +     }
> +}
> +
> +
>  /* Handle response packets: rewrite addresses and send away...
>   */
>  static unsigned int
> @@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, 
> int af)
>        * Check if the packet belongs to an existing connection entry
>        */
>       cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> +     if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
> +         !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&

        Lets move '!iph.fragoffs' after the
'unlikely(!atomic_read(&cp->dest->weight))' because fragoffs
is usually 0. Only IPv6 can see fragments at that point.

> +         is_new_conn(skb, &iph)) {
> +             ip_vs_conn_expire_now(cp);
> +             __ip_vs_conn_put(cp);
> +             cp = NULL;
> +     }
> +
>       if (unlikely(!cp) && !iph.fragoffs) {
>               /* No (second) fragments need to enter here, as nf_defrag_ipv6
>                * replayed fragment zero will already have created the cp
> -- 
> 1.7.5.4

Regards

--
Julian Anastasov <ja@xxxxxx>