Re: [PATCH] IPROUTE: correct nla nested message generated by netem_parse_opt

[Stephen Hemminger <shemminger@vyatta.com>]

On Wed, 27 Aug 2008 22:47:50 +0200
Thomas Graf <tgraf@suug.ch> wrote:

> Please learn to use you enter key. Thank you.
> 
> * Duyck, Alexander H <alexander.h.duyck@intel.com> 2008-08-27 09:30
> > >How was it ever supposed to work? The code looked like the following
> > >prior to commit b03f4672007e533c8dbf0965f995182586216bf1 which made
> > >it used nla_parse_nested_compat()
> > >
> > >-       /* Handle nested options after initial queue options.
> > >-        * Should have put all options in nested format but
> > >too late now.
> > >-        */
> > >-       if (nla_len(opt) > sizeof(*qopt)) {
> > >-               struct nlattr *tb[TCA_NETEM_MAX + 1];
> > >-               if (nla_parse(tb, TCA_NETEM_MAX,
> > >-                             nla_data(opt) + sizeof(*qopt),
> > >-                             nla_len(opt) - sizeof(*qopt), NULL))
> > >
> > >nla_parse_nested_compat() now does exactly what the above code does. So
> > >in what way has the kernel ABI changed?
> > 
> > This is exactly what I am talking about.  Someone assumed it was just manually coding the nested compat format and it wasn't.  It was using its own custom message format.  The change you made changed the kernel ABI to support that custom format instead fixing the message sent to fit the kernel ABI format.
> 
> For the last time, the message format *CANNOT* be changed, the ABI has to
> be stable. I didn't change the ABI, I _restored the ABI to the original
> state after it was broken by the initial nla_parse_nested_compat()
> implementation which contained a bug.
> 
> It's actually trivial to figure this out in 5 minutes using
> git-whatchanged, not sure why you didn't do it.
> 
> 1092cb219774a82b1f16781aec7b8d4ec727c981
> [NETLINK]: attr: add nested compat attribute type
> 
> Introduced nla_parse_nested_compat() for use in netem, unfortunately
> it contained a bug which made it behave incorrectly and broke netem.
> 
> b9a2f2e450b0f770bb4347ae8d48eb2dea701e24
> netlink: Fix nla_parse_nested_compat() to call nla_parse() directly
> 
> This fixed nla_parse_nested_compat() in the way it was supposed
> to be from the beginning. It restored the original ABI.
> 
> Unfortunately, commit 1e90474c377e92db7262a8968a45c1dd980ca9e5 made
> prio use nla_parse_nested_compat() which it shouldn't have as it 
> does not use the same format. It worked due to the bug in
> nla_parse_nested_compat() and then broke when nla_parse_nested_compat()
> was fixed. Since prio was removed, this is no longer a problem.
> 
> Ever since, netem is the only user of nla_parse_nested_compat() so
> I have no idea what you mean when you say I broke it for everybody
> else.
> 
> To put it very very simple, users of the message format as found in
> netem is supposed to use nla_parse_nested_compat(), everybody else
> is supposed to be using nla_parse_nested().
> 
> I won't bother to comment on the rest, since it shoudl be answered with
> this or simply didn't make any sense.

Thomas is right. The ABI was established incrementally and does not use
pure nested format. The original format was just one structure, then
as additional features were added, additional attributes were added.