On Tue, 2010-10-12 at 19:24 -0400, Eric Paris wrote:
> On Tue, 2010-10-12 at 19:14 -0400, Paul Moore wrote:
> > > -ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
> > > +ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
> > > {
> > > - NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark));
> > > - return 0;
> > > + struct nlattr *nest_secctx;
> > > + int len, ret;
> > > + char *secctx;
> > > +
> > > + ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = -1;
> > > + nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
> > > + if (!nest_secctx)
> > > + goto nla_put_failure;
> > >
> > > + NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
> > > + nla_nest_end(skb, nest_secctx);
> > > +
> > > + ret = 0;
> > > nla_put_failure:
> > > - return -1;
> > > + security_release_secctx(secctx, len);
> > > + return ret;
> > > }
> >
> > All the ret assignments bother me, I also don't think "nla_put_failure"
> > is a good choice since we run this code both on success and failure -
> > how about this:
>
> #define NLA_PUT(skb, attrtype, attrlen, data) \
> do { \
> if (unlikely(nla_put(skb, attrtype, attrlen, data) < 0)) \
> goto nla_put_failure; \
> } while(0)
>
> #define NLA_PUT_STRING(skb, attrtype, value) \
> NLA_PUT(skb, attrtype, strlen(value) + 1, value)
>
> so we can't get rid of the nla_put_failure tag and it also means your
> ret values aren't quite right, we have to set ret = -1 before the
> NLA_PUT_STRING()....
Ah, yes, forgot about those stupid macros and their pre-defined goto
labels ... I'm sure someone had a good reason for doing it that way, but
I've never been a fan of that approach (case in point). I'd ask you to
use the "normal" nla_put_*() functions but I see that the rest of the
file uses the macros so it probably isn't worth it ... oh well.
Reviewed-by: Paul Moore <paul.moore@xxxxxx>
--
paul moore
linux @ hp
--
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
