On Mon, Jun 04, 2018 at 09:58:49PM +0300, Julian Anastasov wrote:
>
> Hello,
>
> On Mon, 4 Jun 2018, Simon Horman wrote:
>
> > On Sat, Jun 02, 2018 at 09:50:01PM +0300, Julian Anastasov 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" ?
>
> Because I was not sure how many flags we can add in
> the future. ipvsadm.c has state[16] in print_conn. May be
> we can use ASSURED for now.
Understood. If space is limited, and can't be easily expanded then
I have no objection to "A". But perhaps a comment explaining this
space limitation would be useful.
> > > +};
> > >
> > > /*
> > > * register an ipvs protocol
> > > @@ -193,12 +198,20 @@ ip_vs_create_timeout_table(int *table, int size)
> > > }
> > >
> > >
> > > -const char * ip_vs_state_name(__u16 proto, int state)
> > > +const char *ip_vs_state_name(const struct ip_vs_conn *cp)
> > > {
> > > - struct ip_vs_protocol *pp = ip_vs_proto_get(proto);
> > > + unsigned int state = cp->state;
> > > + struct ip_vs_protocol *pp;
> > > +
> > > + if (cp->flags & IP_VS_CONN_F_TEMPLATE) {
> > >
> > > + if (state >= IP_VS_CTPL_S_LAST)
> > > + return "ERR!";
> > > + return ip_vs_ctpl_state_name_table[state] ? : "?";
> > > + }
> > > + pp = ip_vs_proto_get(cp->protocol);
> > > if (pp == NULL || pp->state_name == NULL)
> > > - return (IPPROTO_IP == proto) ? "NONE" : "ERR!";
> > > + return (cp->protocol == IPPROTO_IP) ? "NONE" : "ERR!";
> > > return pp->state_name(state);
> > > }
> >
> > 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.
>
> Good idea
>
> > > diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > > index 3250c4a1..13826ee 100644
> > > --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > > +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> > > @@ -461,6 +461,13 @@ set_sctp_state(struct ip_vs_proto_data *pd, struct ip_vs_conn *cp,
> > > cp->flags &= ~IP_VS_CONN_F_INACTIVE;
> > > }
> > > }
> > > + 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?
>
> Sure
>
> > > 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'll add it as separate patch early in the patchset.
>
> Thanks for the comments! I'll accumulate more feedback and
> will post 2nd version when net-next opens again.
Great, thanks!
--
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
