Re: [PATCH iproute2-next v3 2/2] tc: implement support for terse dump

[Vlad Buslov <vlad@buslov.dev>]

On Sat 17 Oct 2020 at 14:20, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> On 2020-10-16 12:42 p.m., Vlad Buslov wrote:
>
>>
>> All action print callbacks have arg==NULL check and return at the
>> beginning. To print action type we need either to have dedicated
>> 'brief_dump' callback instead of reusing print_aop() or extend/refactor
>> print_aop() implementation for all actions to always print the type
>> before checking the arg. What do you suggest?
>>
>
> Either one sounds appealing - the refactoring feels simpler
> as opposed to a->terse_print().

With such refactoring we action type will be printed before some basic
validation which can lead to outputting the type together with error
message. Consider tunnel key action output callback as example:

static int print_tunnel_key(struct action_util *au, FILE *f, struct rtattr *arg)
{
	struct rtattr *tb[TCA_TUNNEL_KEY_MAX + 1];
	struct tc_tunnel_key *parm;

	if (!arg)
		return 0;

	parse_rtattr_nested(tb, TCA_TUNNEL_KEY_MAX, arg);

	if (!tb[TCA_TUNNEL_KEY_PARMS]) {
		fprintf(stderr, "Missing tunnel_key parameters\n");
		return -1;
	}
	parm = RTA_DATA(tb[TCA_TUNNEL_KEY_PARMS]);

	print_string(PRINT_ANY, "kind", "%s ", "tunnel_key");

If print "kind" call is moved before checking the arg it will always be
printed, even when immediately followed by "Missing tunnel_key
parameters\n" string. Is this a concern?

>
> BTW: the action index, unless i missed something, is not transported
> from the kernel for terse option. It is an important parameter
> when actions are shared by filters (since they will have the same
> index).
> Am i missing something?

Yes, tc_action_ops->dump(), which outputs action index among other data,
is not called at all by terse dump.

>
> cheers,
> jamal