Date:	Wed, 27 Aug 2008 11:09:42 -0700
From:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:	Thomas Graf <tgraf@...g.ch>
CC:	David Miller <davem@...emloft.net>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"jeff@...zik.org" <jeff@...zik.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"shemminger@...l.org" <shemminger@...l.org>,
	"kaber@...sh.net" <kaber@...sh.net>
Subject: RE: [PATCH 2/3] netlink: nla_parse_nested_compat was not parsing
 nested attributes

>This patch was necessary as patch 1 broke the kernel ABI. It not only
>resulted in leftover warnings, it resulted in the interpreation of
>data with a 4 bytes offset which caused random configuration mess.
>
>"Correct" nested netlink attributes are not supposed to be parsed
>using nla_parse_nested_compat(), nla_parse_nested_compat() is
>exclusively for attributes which contain a list of nested attributes
>without a container attribute.
>

First of all the first patch didn't break the kernel ABI, it broke netem.  The wrong function call was used and as a result you were seeing parsing errors.  Instead of either updating tc to generate the correct format to match the call or reverting the previous patch so that it was parsed correctly the decision was made to fix the call.  We had been using the nested compat attributes in the prio qdisc for some time and that patch had no effect on us.  It wasn't until you "corrected" the kernel ABI in your patch that things were broken.

If you insisit that you are parsing the correct message format then I have another pair or patches I would like to recommend you provide us since your original patch is incomplete.  One to fix nla_nest_compat_start and nla_nest_compat_end so that they can correctly generate messages that can be parsed by the "Correct" nla_parse_nested_compat.  The second to fix iproute so that the libnetlink.c can generate/read "Correct" compat attributes as well.  The only downside will be that iproute will not be compatible with any of the older kernels that are using nested compat attributes since we will have changed the kernel ABI to match your "Correct" format.

>> Patches 3 & 4 haven't been applied yet and are under review:
>>
>> 3.  "[PATCH 2/3] netlink: nla_parse_nested_compat was not
>parsing nested attributes"
>(http://marc.info/?l=linux-netdev&m=121936623112195&w=2) from
>me.  This reverts patch 2.
>>
>> 4.  "[PATCH] IPROUTE: correct nla nested message generated
>by netem_parse_opt"
>(http://marc.info/?l=linux-netdev&m=121936623112195&w=2) from
>me.  This patch changes iproute2 netem_parse_opt to generate a
>correctly formatted set of nested compat attributes that can
>be parsed after the introduction of patch 1.
>
>You can't change ABI! What your patches do is to generate new
>style nested
>attributes and change nla_parse_nested_compat() to follow the same
>semantisc as nla_parse_nested(). The _compat() is there for a reason.

So let me see if I understand this correctly.  Reverting your patch which changed the ABI in the first place is me changing it?  So what was it you did in the first place to support the custom format generated by netem then?

We should either revert 1 & 2, or add 3 & 4 so that the kernel ABI can read the same messages it generates.  If you want to provide your own patches that fix the kernel/iproute as I described above I would be perfectly happy with that too.  I honestly don't care how it gets solved as long as this gets fixed.

Alex

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists