netdev - Re: [PATCH iproute2-next 5.16 v6 3/5] iplink_can: use PRINT

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] [day] [month] [year] [list]

Date:   Mon, 10 Oct 2022 20:04:12 +0900
From:   Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
Cc:     netdev@...r.kernel.org, linux-can@...r.kernel.org
Subject: Re: [PATCH iproute2-next 5.16 v6 3/5] iplink_can: use PRINT_ANY to
 factorize code and fix signedness

On Fri. 7 Oct. 2022 at 16:56, Marc Kleine-Budde <mkl@...gutronix.de> wrote:
> On 04.11.2021 01:44:26, Vincent Mailhol wrote:
> > Current implementation heavily relies on some "if (is_json_context())"
> > switches to decide the context and then does some print_*(PRINT_JSON,
> > ...) when in json context and some fprintf(...) else.
> >
> > Furthermore, current implementation uses either print_int() or the
> > conversion specifier %d to print unsigned integers.
> >
> > This patch factorizes each pairs of print_*(PRINT_JSON, ...) and
> > fprintf() into a single print_*(PRINT_ANY, ...) call. While doing this
> > replacement, it uses proper unsigned function print_uint() as well as
> > the conversion specifier %u when the parameter is an unsigned integer.
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@...adoo.fr>
>
> [...]
>
> >       if (tb[IFLA_CAN_TERMINATION_CONST] && tb[IFLA_CAN_TERMINATION]) {
> > @@ -538,29 +483,21 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
> >                       sizeof(*trm_const);
> >               int i;
> >
> > -             if (is_json_context()) {
> > -                     print_hu(PRINT_JSON, "termination", NULL, *trm);
> > -                     open_json_array(PRINT_JSON, "termination_const");
> > -                     for (i = 0; i < trm_cnt; ++i)
> > -                             print_hu(PRINT_JSON, NULL, NULL, trm_const[i]);
> > -                     close_json_array(PRINT_JSON, NULL);
> > -             } else {
> > -                     fprintf(f, "\n    termination %hu [ ", *trm);
> > -
> > -                     for (i = 0; i < trm_cnt - 1; ++i)
> > -                             fprintf(f, "%hu, ", trm_const[i]);
> > -
> > -                     fprintf(f, "%hu ]", trm_const[i]);
>                                         ^
> > -             }
> > +             can_print_nl_indent();
> > +             print_hu(PRINT_ANY, "termination", " termination %hu [ ", *trm);
>
> Always '['
>
> > +             open_json_array(PRINT_JSON, "termination_const");
> > +             for (i = 0; i < trm_cnt; ++i)
> > +                     print_hu(PRINT_ANY, NULL,
> > +                              i < trm_cnt - 1 ? "%hu, " : "%hu",
> > +                              trm_const[i]);
> > +             close_json_array(PRINT_JSON, " ]");
>
> ']' only for JSON.

Thanks for the report

Actually, the second argument of close_json_array() is what should be
printed for the normal (non-json) output. Here it is correctly set to
" ]". *However*, this parameter gets ignored because the first
argument is PRINT_JSON instead of PRINT_ANY.

This is a lack of testing on my side (I do not have hardware which
supports the switchable termination resistors). After investigation,
the bitrate and dbitrate also have the same issue.

This is fixed in
https://lore.kernel.org/linux-can/20221010110118.66116-1-mailhol.vincent@wanadoo.fr/

> >       }
>
> I just noticed that the non JSON output for termination is missing the
> closing ']'. See the output in the documentation update by Daniel:
>
> | https://lore.kernel.org/all/4514353.LvFx2qVVIh@daniel6430

Yours sincerely,
Vincent Mailhol