On Fri. 7 Mai 2021 at 00:50, Stephen Hemminger
<stephen@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, 6 May 2021 20:20:07 +0900
> Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> wrote:
>
> > + if (tb[IFLA_CAN_TDC_TDCV] && tb[IFLA_CAN_TDC_TDCO] &&
> > + tb[IFLA_CAN_TDC_TDCF]) {
> > + __u32 *tdcv = RTA_DATA(tb[IFLA_CAN_TDC_TDCV]);
> > + __u32 *tdco = RTA_DATA(tb[IFLA_CAN_TDC_TDCO]);
> > + __u32 *tdcf = RTA_DATA(tb[IFLA_CAN_TDC_TDCF]);
> > +
> > + if (is_json_context()) {
> > + open_json_object("tdc");
> > + print_int(PRINT_JSON, "tdcv", NULL, *tdcv);
> > + print_int(PRINT_JSON, "tdco", NULL, *tdco);
> > + print_int(PRINT_JSON, "tdcf", NULL, *tdcf);
> > + close_json_object();
> > + } else {
> > + fprintf(f, "\n tdcv %d tdco %d tdcf %d",
> > + *tdcv, *tdco, *tdcf);
> > + }
> > + }
> > +
>
> The most common pattern in iproute2 is to let json/non-json be decided
> inside the print routine. I search for all instances of fprintf as
> indication of broken code. Also these are not signed values so please
> print unsigned. The code should use print_nl() to handle the single line
> case. Also, there is helper to handle
Thanks for pointing out the issues! For my defence, all the
existing code of ip/iplink_can.c shares the exact same issues and
I just repeated those without using my brain...
I just sent a new v2 which fixes your remarks on both the
existing code and the new code introduced in my patch.
> Something like:
> __u32 tdc = rte_getattr_u32(tb[IFLA_CAN_TDC_TDCV]);
>
> open_json_object("tdc");
> print_nl();
> print_u32(PRINT_ANY, "tdcv", " tdcv %u", tdcv);
There is no print_u32 in iproute2. I guess you meant print_uint(). :)
Yours sincerely,
Vincent
