On Thu, 27 Jun 2013, Dash Four wrote:
> Jozsef Kadlecsik wrote:
> > Get rid of the compatibility functions. Or keep the original function names
> > with the extended arguments: there's no stable API.
> >
> As you've probably guessed, I kept the old functions and their parameters in
> order to preserve the existing interface API, since any changes I make to
> these will break existing code using them. If there is no objection and there
> is no such requirement, I'll get rid of them in the next release of the
> patches - just let me know.
Yes, there's no such requirement and remove the backward compatibility
part.
> > > #define _IP_SET_MODULE_DESC(a, b, c) \
> > > MODULE_DESCRIPTION(a " type of IP sets, revisions " b "-" c)
> > > @@ -361,6 +365,25 @@ static inline int nla_put_ipaddr6(struct sk_buff
> > > *skb,
> > > int type,
> > > }
> > >
> > > /* Get address from skbuff */
> > > +static inline bool
> > > +ipv4addrptr(const struct sk_buff *skb, bool inner, bool src, __be32
> > > *addr)
> > > +{
> > > + struct iphdr *ih = ip_hdr(skb);
> > > + unsigned int protooff = ip_hdrlen(skb);
> > > +
> > > + if (ih == NULL ||
> > >
> >
> > Why do you have to check the IP header?
> >
> Simply playing it safe. I am referencing it later on in the code and if that
> pointer is NULL for whatever reason, it will trigger a null-pointer
> dereference, hence the check above.
The return value of ip[6]_hdr was never checked in ipset code yet, because
it doesn't return NULL at the point where these code parts are called.
It's an unnecessary checking.
> > > + (inner &&
> > > + (ih->protocol != IPPROTO_ICMP ||
> > > + !ip_set_get_icmpv4_inner_hdr(skb, &protooff, &ih))))
> > > + goto err;
> > >
>
> > Also, there's no need for the goto here: there's no other error path.
> >
> I disagree. By having "return false" (or "return 0", "return -1" and so on)
> instead of "goto err" it is not immediately apparent to someone who
> studies/reviews/uses the code that this is an error condition/path. I have
> been in that situation many times when I have to go back and look at a
> particular function call to see what "return false" or "return 0" actually
> means.
>
> By including "goto err" instead of multiple "return false" statement, that
> makes it instantly obvious what the purpose of that statement is without
> having to look elsewhere.
I see the point, to self-document the code, with the price of more lines.
It's a little bit overdoing in my opinion: pretty apparent which is the
error path and which is not. But this is highly a personal taste, I won't
press it.
Best regards,
Jozsef
-
E-mail : kadlec@xxxxxxxxxxxxxxxxx, kadlecsik.jozsef@xxxxxxxxxxxxx
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : Wigner Research Centre for Physics, Hungarian Academy of Sciences
H-1525 Budapest 114, POB. 49, Hungary
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
