From: "Duyck, Alexander H" <alexander.h.duyck@intel.com>
To: Thomas Graf <tgraf@suug.ch>, Alexander Duyck <alexander.duyck@gmail.com>
CC: Stephen Hemminger <shemminger@vyatta.com>,
       "Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel.com>,
       "davem@davemloft.net" <davem@davemloft.net>,
       "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
       "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
       "akpm@linux-foundation.org" <akpm@linux-foundation.org>
Date: Wed, 27 Aug 2008 09:30:17 -0700
Subject: RE: [PATCH] IPROUTE: correct nla nested message generated by
 netem_parse_opt
Thread-Topic: [PATCH] IPROUTE: correct nla nested message generated by
 netem_parse_opt
Thread-Index: AckIUvfJsL15AJNAScWe3WGYxfx3wQADFNyg
Message-ID: <80769D7B14936844A23C0C43D9FBCF0F1506159B@orsmsx501.amr.corp.intel.com>
References: <20080822005011.4615.98303.stgit@jtkirshe-mobile.jf.intel.com>
 <20080821211941.66090862@speedy>
 <5f2db9d90808211937o1d9b33t9ad933938872e08d@mail.gmail.com>
 <20080827144122.GT20815@postel.suug.ch>
In-Reply-To: <20080827144122.GT20815@postel.suug.ch>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2865
Lines: 59

>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.

>There is two ways of sending a fixed struct + attributes inside an
>attribute:
>
>a) (old and outdated method)
>  attr foo
>    fixed struct
>    [nested attr 1]
>    [nested attr 2]
>
>This format can be parsed with nla_parse_nested_compat(attr foo)
>
>b) (new method)
>  attr foo
>    fixed struct
>    attr container
>      [nested attr 1]
>      [nested attr 2]
>
>This format is parsed with nla_parse_nested(attr container)
>

I think you will find that format a only existed in netem prior to your changes in nla_parse_nested_compat, and the problem is option b, which was a nested compat attribute, can no longer be parsed at all.  Basically what was generated before was an attribute with a nested attribute following it as data.  Now what you have is an attribute followed by a series of other attributes.

Also you are incorrect about nla_parse_nested.  The layout for it is something more like:

C) (nested format)
        attr foo
          [nested attr 1]
          [nested attr 2]

This is the format parsed by nla_parse_nested.  Basically what happened is that when you fixed things for netem you broke them for anyone else who might use the same message format.  If you take a look at nla_nest_compat_start and nla_nest_compat_end you will see that the message generated matches format b, but now you are parsing format a.  If the community insists that I can't change the kernel ABI back I am perfectly fine with that, but I think somebody needs to explain to me why we are handling two different formats and fix things so that the format is consistent.

Thanks,

Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/