question about nla_nest_cancel

question about nla_nest_cancel

[Julia Lawall]

I find numerous occurrences of code like the following, in which nest ends 
up with the value NULL and then nla_nest_cancel is called with nest as the 
second argument.  But nla_nest_cancel just calls nlmsg_trim with the same 
second argument, and nlmsg_trim does nothing if its second argument is 
NULL.  Is there any reason to keep these calls?

thanks,
julia



static int tbf_dump(struct Qdisc *sch, struct sk_buff *skb)
{
	struct tbf_sched_data *q = qdisc_priv(sch);
	struct nlattr *nest;
	struct tc_tbf_qopt opt;

	nest = nla_nest_start(skb, TCA_OPTIONS);
	if (nest == NULL)
		goto nla_put_failure;

	opt.limit = q->limit;
	opt.rate = q->R_tab->rate;
	if (q->P_tab)
		opt.peakrate = q->P_tab->rate;
	else
		memset(&opt.peakrate, 0, sizeof(opt.peakrate));
	opt.mtu = q->mtu;
	opt.buffer = q->buffer;
	NLA_PUT(skb, TCA_TBF_PARMS, sizeof(opt), &opt);

	nla_nest_end(skb, nest);
	return skb->len;

nla_put_failure:
	nla_nest_cancel(skb, nest);
	return -1;
}

Re: question about nla_nest_cancel

[Kurt Van Dijck]

On Thu, Jan 27, 2011 at 06:08:34PM +0100, Julia Lawall wrote:
> 
> I find numerous occurrences of code like the following, in which nest ends 
> up with the value NULL and then nla_nest_cancel is called with nest as the 
> second argument.  But nla_nest_cancel just calls nlmsg_trim with the same 
> second argument, and nlmsg_trim does nothing if its second argument is 
> NULL.  Is there any reason to keep these calls?
I just learned this:
nla_nest_start() adds data to the skb.
nla_nest_end() 'commits' the proper length.
nla_nest_cancel() reverts skb to the state before nla_nest_start(),
as if nothing happened.

Kurt

Re: question about nla_nest_cancel

[Julia Lawall]

On Thu, 27 Jan 2011, Kurt Van Dijck wrote:

> On Thu, Jan 27, 2011 at 06:08:34PM +0100, Julia Lawall wrote:
> > 
> > I find numerous occurrences of code like the following, in which nest ends 
> > up with the value NULL and then nla_nest_cancel is called with nest as the 
> > second argument.  But nla_nest_cancel just calls nlmsg_trim with the same 
> > second argument, and nlmsg_trim does nothing if its second argument is 
> > NULL.  Is there any reason to keep these calls?
> I just learned this:
> nla_nest_start() adds data to the skb.
> nla_nest_end() 'commits' the proper length.
> nla_nest_cancel() reverts skb to the state before nla_nest_start(),
> as if nothing happened.

Yes, I can see this as well.  But in this case, it seems to me taht 
nothing has happened, because nla_nest_star has returned NULL?

julia

Re: question about nla_nest_cancel

[Ben Pfaff]

Julia Lawall <julia@diku.dk> writes:

> I find numerous occurrences of code like the following, in which nest ends 
> up with the value NULL and then nla_nest_cancel is called with nest as the 
> second argument.  But nla_nest_cancel just calls nlmsg_trim with the same 
> second argument, and nlmsg_trim does nothing if its second argument is 
> NULL.  Is there any reason to keep these calls?

I think that you are missing that NLA_PUT() contains an internal
"goto nla_put_failure;".  If that branch is taken, then
nla_nest_cancel() trims off the nested attribute.  So just
removing the call to nla_nest_cancel() would change behavior in
that case.
-- 
Ben Pfaff 
http://benpfaff.org

Re: question about nla_nest_cancel

[Julia Lawall]

On Thu, 27 Jan 2011, Ben Pfaff wrote:

> Julia Lawall <julia@diku.dk> writes:
> 
> > I find numerous occurrences of code like the following, in which nest ends 
> > up with the value NULL and then nla_nest_cancel is called with nest as the 
> > second argument.  But nla_nest_cancel just calls nlmsg_trim with the same 
> > second argument, and nlmsg_trim does nothing if its second argument is 
> > NULL.  Is there any reason to keep these calls?
> 
> I think that you are missing that NLA_PUT() contains an internal
> "goto nla_put_failure;".  If that branch is taken, then
> nla_nest_cancel() trims off the nested attribute.  So just
> removing the call to nla_nest_cancel() would change behavior in
> that case.

Indeed.  Thank you for the explanation.

julia