LVS
lvs-devel
Google
 
Web LinuxVirtualServer.org

Re: [v2 PATCH 3/4] IPVS: Backup, Adding Version 1 receive capability

To: Simon Horman <horms@xxxxxxxxxxxx>
Subject: Re: [v2 PATCH 3/4] IPVS: Backup, Adding Version 1 receive capability
Cc: Hans Schillstrom <hans.schillstrom@xxxxxxxxxxxx>, lvs-devel@xxxxxxxxxxxxxxx, ja@xxxxxx, wensong@xxxxxxxxxxxx, daniel.lezcano@xxxxxxx
From: Hans Schillstrom <hans@xxxxxxxxxxxxxxx>
Date: Sat, 30 Oct 2010 13:49:14 +0200
On Saturday, October 30, 2010 08:47:10 Simon Horman wrote:
> > *v2
> >  A new option format added as with opt,opt-len,data
> >  as a general way to add options to a conn entry.
> >  timeout is now in seconds
> >  fwmark is not in ip_vs_conn_param any more.
> >  Mask for flags received by backup.
> >  Basically all changes implements Julians comments.
> 
> 
> I think that you need the patch
> "IPVS: Add persistence engine to connection entry" that I posted,
> or something similar in your series in order for connection
> templates with created by synchronisation to have the correct
> persistence engine associated with them.
> 
Thanks
Yes I need that one.

[ snip ]
> 
> Please align line-wrapped parameters against the opening (
> 
>       ip_vs_conn_fill_param(af, sc->protocol,
>                             (const union nf_inet_addr *)&sc->caddr,
>                             sc->cport,
>                             (const union nf_inet_addr *)&sc->vaddr,
>                             sc->vport, p);
> 
> Also several more times below.
> This also applies to function definitions.
> 
> Actually, I think p_vs_conn_fill_param_sync_v0() can be
> replaced by a direct call to ip_vs_conn_fill_param(). But
> my comments regarding formatting still apply elsewhere.
> 
I do agree

[ snip ]
> >     return 0;
> >  }
> >  
> >  /*
> > - *      Process received multicast message and create the corresponding
> > - *      ip_vs_conn entries.
> > + *  fill_param used by version 1
> >   */
> 
> I realise that the commenting syntax used throughout IPVS
> is a) inconsistent and b) for the most part does not comply with
> any variant of the style guidelines. However, for the record
> I believe that the preferred commenting style for code in net/
> and thus IPVS is:
> 
> /* This is a one line comment */
> 
> /* This is a
>  * multi-line comment
>  */
> 
I'll do my home work and hold on tight to the coding rules :-)

[ snip ]
> > @@ -505,103 +626,214 @@ static void ip_vs_process_message(const char 
> > *buffer, const size_t buflen)
> >                     }
> >             }
> >  
> > -           {
> > -                   if (ip_vs_conn_fill_param_sync(AF_INET, s->protocol,
> > -                                         (union nf_inet_addr *)&s->caddr,
> > -                                         s->cport,
> > -                                         (union nf_inet_addr *)&s->vaddr,
> > -                                         s->vport, &param)) {
> > -                           pr_err("ip_vs_conn_fill_param_sync failed");
> > -                           return;
> > -                   }
> > -                   if (!(flags & IP_VS_CONN_F_TEMPLATE))
> > -                           cp = ip_vs_conn_in_get(&param);
> > -                   else
> > -                           cp = ip_vs_ct_in_get(&param);
> > +           if (ip_vs_conn_fill_param_sync_v0(AF_INET, s, &param)) {
> > +                   pr_err("ip_vs_conn_fill_param_sync failed");
> > +                   return;
> >             }
> 
> I think we can just replace this with a direct call to
> ip_vs_conn_fill_param() and avoid the need for error checking.
> 
> The idea of the existing ip_vs_conn_fill_param_sync() was as a place holder.
> But this patch fills in the relevant bits in the v1 version of that function.

OK I will have a look at this
[ snip ]

> > +
> > +   /* SyncID sanity check */
> > +   if (ip_vs_backup_syncid != 0 && m2->syncid != ip_vs_backup_syncid) {
> > +           IP_VS_DBG(7, "Ignoring incoming msg with syncid = %d\n",
> > +                     m2->syncid);
> > +           return;
> > +   }
> > +   /* Prepare ptrs for version 1 or 2 message */
> > +   if ( m2->version==SYNC_PROTO_VER && m2->reserverd==0 && m2->spare==0) {
> 
> There shouldn't be a space between '(' and 'm2'.
> There should be a space either side of each '=='.
OK 
I'll break the line
> 
> > +          p = (char *)buffer + sizeof(struct ip_vs_sync_mesg_v2);
> 
> This cast seems nasty to me. Is it there to un-const buffer?
> If so I think it would be better just remove the const qualification
> from the parameter, as its clearly not being used in a const way.
> 
> The patch "IPVS: buffer argument to ip_vs_process_message() should not be 
> const"
> from my prototype series was my attempt to address this.
> 
OK, 

Thanks
 Hans
--
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>