netdev - Re: [PATCH iproute2-next v2 2/8] dcb: app: modify dcb-app print functions for dcb-rewr reuse

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, 31 May 2023 13:26:06 +0200
From: Petr Machata <petrm@...dia.com>
To: Daniel Machon <daniel.machon@...rochip.com>
CC: Petr Machata <petrm@...dia.com>, <netdev@...r.kernel.org>,
	<dsahern@...nel.org>, <stephen@...workplumber.org>,
	<UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH iproute2-next v2 2/8] dcb: app: modify dcb-app print
 functions for dcb-rewr reuse


Daniel Machon <daniel.machon@...rochip.com> writes:

>> Daniel Machon <daniel.machon@...rochip.com> writes:
>> 
>> > Where dcb-app requires protocol to be the printed key, dcb-rewr requires
>> > it to be the priority. Adapt existing dcb-app print functions for this.
>> >
>> > dcb_app_print_filtered() has been modified, to take two callbacks; one
>> > for printing the entire string (pid and prio), and one for the pid type
>> > (dec, hex, dscp, pcp). This saves us for making one dedicated function
>> > for each pid type for both app and rewr.
>> >
>> > dcb_app_print_key_*() functions have been renamed to
>> > dcb_app_print_pid_*() to align with new situation. Also, none of them
>> > will print the colon anymore.
>> >
>> > Signed-off-by: Daniel Machon <daniel.machon@...rochip.com>
>> 
>> There are about four patches included in this one patch: the %d->%u
>> change, the colon shenanigans, the renaming, and prototype change of
>> dcb_app_print_filtered().
>> 
>> I think the code is OK, but I would appreciate splitting into a patch
>> per feature.
>
> Actually, IMHO, the 'colon shenanigans' does not make much sense in a
> patch of its own, since its not really a standalone feature, but rather
> some change that is required for the prototype change of
> dcb_app_print_filtered? 

It doesn't make sense on its own, but it also does not get sent on its
own, but together with the following patches. Its role is to set stage.

dcb_app_print_filtered() should first stop assuming the callback prints
a colon. So you have one patch where the colon moves from the callback
to _print_filtered(). That's a clean, behavior-neutral change that
should be trivial to review. Then next patch introduces the callback.
Which at that point should likewise be tidy, focused and easy to review,
because it will only deal with the new callback.