LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [PATCH net-next 1/2] ipvs: add assured state for conn templates

To: Simon Horman <horms@xxxxxxxxxxxx>
Subject: Re: [PATCH net-next 1/2] ipvs: add assured state for conn templates
Cc: lvs-devel@xxxxxxxxxxxxxxx, mkoutny@xxxxxxxx, mkubecek@xxxxxxxx
From: Julian Anastasov <ja@xxxxxx>
Date: Fri, 6 Jul 2018 08:32:39 +0300 (EEST)
        Hello,

On Mon, 4 Jun 2018, Simon Horman wrote:

> > +/* States for conn templates: NONE or letters separated with "," */
> > +static const char *ip_vs_ctpl_state_name_table[IP_VS_CTPL_S_LAST] = {
> > +   [IP_VS_CTPL_S_NONE]                     = "NONE",
> > +   [IP_VS_CTPL_S_ASSURED]                  = "A",
> 
> "A" seems a bit cryptic, why not "ASSURED" ?

        I posted v2 which includes this change.

> I'd slightly prefer if refactoring ip_vs_state_name to not take a state
> parameter was a separate patch. Its a nice cleanup. But it doesn't seem
> related to the rest of what is going on here.

        Done

> > +           if (next_state == IP_VS_SCTP_S_ESTABLISHED) {
> > +                   struct ip_vs_conn *ct = cp->control;
> > +
> > +                   if (ct && (ct->flags & IP_VS_CONN_F_TEMPLATE) &&
> > +                       !(ct->state & IP_VS_CTPL_S_ASSURED))
> > +                           ct->state |= IP_VS_CTPL_S_ASSURED;
> > +           }
> 
> The logic above seems to be replicated several times below.
> Could we have a helper function?

        Done

> >  static int __udp_init(struct netns_ipvs *ipvs, struct ip_vs_proto_data *pd)
> > diff --git a/net/netfilter/ipvs/ip_vs_sync.c 
> > b/net/netfilter/ipvs/ip_vs_sync.c
> > index 001501e..24891bd 100644
> > --- a/net/netfilter/ipvs/ip_vs_sync.c
> > +++ b/net/netfilter/ipvs/ip_vs_sync.c
> > @@ -1003,11 +1003,10 @@ static void ip_vs_process_message_v0(struct 
> > netns_ipvs *ipvs, const char *buffer
> >                             continue;
> >                     }
> >             } else {
> > -                   /* protocol in templates is not used for state/timeout 
> > */
> > -                   if (state > 0) {
> > -                           IP_VS_DBG(2, "BACKUP v0, Invalid template state 
> > %u\n",
> > -                                   state);
> > -                           state = 0;
> > +                   if (state >= IP_VS_CTPL_S_LAST) {
> > +                           IP_VS_DBG(7, "BACKUP v0, Invalid tpl state 
> > %u\n",
> > +                                     state);
> 
> Not strictly related to this patch, but should these debug messages
> be rate limited in some way?

        I didn't included any changes to rate limit the
debug messages. It can be done with additional patch, for
all such messages. Not sure, may be we can add some macro
to both rate limit and check debugging level, even a
per-backup macro would be useful, it can also include the
IP address of sender.

Regards

--
Julian Anastasov <ja@xxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [PATCH net-next 1/2] ipvs: add assured state for conn templates, Julian Anastasov <=