netdev - Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Jan 2022 08:52:30 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Stephen Hemminger <stephen@...workplumber.org>,
        Andrea Claudi <aclaudi@...hat.com>
Cc:     netdev@...r.kernel.org, Wen Liang <wenliang@...hat.com>,
        David Ahern <dsahern@...nel.org>,
        Victor Nogueira <victor@...atatu.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Davide Caratti <dcaratti@...hat.com>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Vlad Buslov <vladbu@...dia.com>
Subject: Re: [PATCH iproute2 v3 1/2] tc: u32: add support for json output

On 2022-01-24 19:43, Stephen Hemminger wrote:
> On Mon, 24 Jan 2022 22:30:21 +0100
> Andrea Claudi <aclaudi@...hat.com> wrote:
> 
>> On Mon, Jan 24, 2022 at 10:50:16AM -0800, Stephen Hemminger wrote:
>>> On Mon, 24 Jan 2022 19:25:06 +0100
>>> Andrea Claudi <aclaudi@...hat.com> wrote:
>>>    
>>>> On Thu, Jan 06, 2022 at 02:30:13PM -0800, Stephen Hemminger wrote:
>>>>> On Thu,  6 Jan 2022 13:45:51 -0500
>>>>> Wen Liang <liangwen12year@...il.com> wrote:
>>>>>      
>>>>>>   	} else if (sel && sel->flags & TC_U32_TERMINAL) {
>>>>>> -		fprintf(f, "terminal flowid ??? ");
>>>>>> +		print_bool(PRINT_ANY, "terminal_flowid", "terminal flowid ??? ", true);
>>>>>
>>>>> This looks like another error (ie to stderr) like the earlier case
>>>>>     
>>>>

>>>
>>> Just always put the same original message on stderr.
>>>    
>>
>> Let me phrase my case better: I think the "terminal flowid" message
>> should not be on stderr, as I don't think this is an error message.
>>
>> Indeed, "terminal flowid ???" is printed every time we use "action" or
>> "policy" (see my previous email for details), even when no error is
>> present and cls_u32 is working ok. In these cases, not having a flowid
>> is legitimate and not an error.
>>
>> As this is the case, I think the proper course of action is to have this
>> message printed out only in non-json output to preserve the same output
>> of older iproute versions. It would be even better if we decide to
>> remove this message altogether, as it is not adding any valuable info to
>> the user.
> 
> Agree, I have never used this obscure corner of u32 so will defer to others.

Andrea is correct: it is not an error and not deserving to be in stderr.
And it is _not_ an obscure case just for u32. The classid/flowid is a
classifier concept - in most cases is used to select a queue upon
a filter  match but could also be used to uniquely identify a flow.
Consider it a mini-action which identifies the flow. It is omni-present.
In this case u32 is only reporting it wasnt set. It doesnt affect
the datapath functionality. So u32 is doing the right thing. Flower is
a big culprit in that when you visit the googles hardly any example
shows this field being set and the iproute2 side of flower
code just ignores it.

> But the existing message is unhelpful and looks like a bug.
> The output should be clear and correct for both json and non-json cases;
> and any ??? kind of output should be reserved for cases where some bogus
> result is being returned by the kernel.  Some version skew, or partial
> result of previous operation maybe.

Makes sense in particular if we have formal output format like json.
If this breaks tdc it would be worth to fix tdc (and not be backward
compatible)

So: Since none of the tc maintainers was Cced in this thread, can we
please impose a rule where any changes to tc subdir needs to have tdc
tests run (and hopefully whoever is making the change will be gracious
to contribute an additional testcase)?
Do you need a patch for that in some documentation?

cheers,
jamal