Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210506112007.1666738-1-mailhol.vincent@wanadoo.fr>
 <20210506112007.1666738-2-mailhol.vincent@wanadoo.fr> <20210506085035.2fc33bf3@hermes.local>
In-Reply-To: <20210506085035.2fc33bf3@hermes.local>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Fri, 7 May 2021 19:35:14 +0900
Message-ID: <CAMZ6RqLcbF2riaMiOPHD4T-A0yQmrr--4moPT1iLz11h2yf3xA@mail.gmail.com>
Subject: Re: [RFC PATCH v1 1/1] iplink_can: add new CAN FD bittiming
 parameters: Transmitter Delay Compensation (TDC)
To:     Stephen Hemminger <stephen@networkplumber.org>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>,
        linux-can <linux-can@vger.kernel.org>,
        Oliver Hartkopp <socketcan@hartkopp.net>,
        netdev <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri. 7 Mai 2021 at 00:50, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Thu,  6 May 2021 20:20:07 +0900
> Vincent Mailhol <mailhol.vincent@wanadoo.fr> 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