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.
>
> > +};
> >
> > /*
> > * 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.
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
