On 17-04-19 11:54 AM, Jiri Pirko wrote:
Wed, Apr 19, 2017 at 05:37:15PM CEST, j...@mojatatu.com wrote:
On 17-04-19 09:13 AM, Jiri Pirko wrote:
Wed, Apr 19, 2017 at 03:03:59PM CEST, j...@mojatatu.com wrote:
On 17-04-19 08:36 AM, Jiri Pirko wrote:
Wed, Apr 19, 2017 at 01:57:29PM CEST, j...@mojatatu.com wrote:
From: Jamal Hadi Salim <j...@mojatatu.com>
include/uapi/linux/rtnetlink.h | 21 +++++++++++++++++++--
net/sched/act_api.c | 43 ++++++++++++++++++++++++++++++++----------
3 files changed, 53 insertions(+), 12 deletions(-)
+#define TCAA_MAX (__TCAA_MAX - 1)
#define TA_RTA(r) ((struct rtattr*)(((char*)(r)) + NLMSG_ALIGN(sizeof(struct
tcamsg))))
#define TA_PAYLOAD(n) NLMSG_PAYLOAD(n,sizeof(struct tcamsg))
-#define TCA_ACT_TAB 1 /* attr type must be >=1 */
-#define TCAA_MAX 1
+#define TCA_ACT_TAB TCAA_ACT_TAB
This is mess. What does "TCAA" stand for?
TC Actions Attributes. What would you call it? I could have
called it TCA_ROOT etc. But maybe a comment to just call it
TC Actions Attributes would be enough?
TCA_DUMP_X
it is only for dumping. Naming it "attribute" seems weird. Same as if
you have: int variable_something;
Jiri, this is not just for dumping. We are describing high level
attributes for tc actions.
This is already present:
enum {
TCA_ACT_UNSPEC,
TCA_ACT_KIND,
TCA_ACT_OPTIONS,
TCA_ACT_INDEX,
TCA_ACT_STATS,
TCA_ACT_PAD,
TCA_ACT_COOKIE,
__TCA_ACT_MAX
};
This is nested inside the dump message (TCAA_MAX, TCA_ACT_TAB)
Looks like you are mixing these 2.
No - That space is per action. The space i am defining is
above that in the the hierarchy. There used to be only
one attribute there (TCA_ACT_TAB) and now we are making
it more generic.
It is a _lot_ of code to change! Note:
This is all the UAPI visible code (the same coding style for 20 years).
I am worried about this part.
We'll see. Lets do it in a sensitive way, in steps. But for new things,
I think it is good not to stick with old and outlived habits.
I know you have the muscle to get it done - so fine, i will start
with this one change.
Netlink is TLV, should be used as TLV. I don't see how you can run out
any space. You tend to use Netlink in some weird hybrid mode, with only
argument being space. I think that couple of bytes wasted is not
a problem at all...
You are not making sense to me still.
What you describe as "a couple of bytes" adds up when you have
a large number of objects. I am trying to cut down on data back
and forth from user/kernel and a bitmap is a well understood entity.
Even if i did use a TLV - when i am representing flags which require one
bit each - it makes sense to use a bitmask encapsulated in a TLV. Not
to waste a TLV per bit.
cheers,
jamal
