linux-kernel - Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human readable format via %pt

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]

Message-ID: <1519221748.10722.34.camel@linux.intel.com>
Date:   Wed, 21 Feb 2018 16:02:28 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Rasmus Villemoes <rasmus.villemoes@...vas.dk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
        linux-rtc@...r.kernel.org, Arnd Bergmann <arnd@...db.de>,
        Joe Perches <joe@...ches.com>,
        Mark Salyzyn <salyzyn@...roid.com>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Guan Xuetao <gxt@...c.pku.edu.cn>,
        Ingo Molnar <mingo@...nel.org>,
        Jason Wessel <jason.wessel@...driver.com>,
        Jonathan Corbet <corbet@....net>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH v2 01/21] lib/vsprintf: Print time and date in human
 readable format via %pt

On Wed, 2018-02-21 at 10:33 +0100, Geert Uytterhoeven wrote:
> Hi Andy,
> 
> On Tue, Feb 20, 2018 at 10:43 PM, Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > There are users which print time and date represented by content of
> > struct rtc_time in human readable format.
> > 
> > Instead of open coding that each time introduce %ptR[dt][rv]
> > specifier.
> 
> Thanks for your patch!
> 
> > Note, users have to select PRINTK_PEXT_TIMEDATE option in a Kconfig.
> 
> Is it worthwhile making this an option?

People were complaining before

https://lists.01.org/pipermail/kbuild-all/2017-June/034950.html

> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -412,6 +412,37 @@ Examples::
> > 
> >  Passed by reference.
> > 
> > +Time and date
> > +-------------
> > +
> > +::
> > +
> > +       %pt[R]          YYYY-mm-dd HH:MM:SS
> > +       %pt[R]d         YYYY-mm-dd
> > +       %pt[R]t         HH:MM:SS
> 
> [R] suggests the "R" is optional?
> But if it's missing, it prints the hex pointer value?

Yes.

> > +       %pt[R][dt]
> 
> What's the purpose of this?

A place holder to extend.

> > +
> > +  R for struct rtc_time
> > +
> > +Note, users have to select PRINTK_PEXT_TIMEDATE option in a
> > Kconfig.
> > +
> > +struct rtc_time
> > +~~~~~~~~~~~~~~~
> > +
> > +::
> > +
> > +       %ptR[dt][rv]
> 
> What's the purpose of this paragraph, compared to the previous one?

This is first batch to make it working for struct rtc_time. We have
several users (and I have some local patches WIP) to print time64_t /
timespec64 which would use different letters and paragraphs to explain.

I could remove it and return like it was in v1 (with the exception for
new R letter added).

TBH, I don't see much consensus among developers on this topic.
I wouldn't like to send a new version until it would be a consensus.

> > +static noinline_for_stack
> > +char *date_str(char *buf, char *end, const struct rtc_time *tm,
> > bool v, bool r)
> > +{
> > +       int year = tm->tm_year + (r ? 0 : 1900);
> > +       int mon = tm->tm_mon + (r ? 0 : 1);
> > +
> > +       if (unlikely(v && (unsigned int)tm->tm_year > 200))
> > +               buf = string(buf, end, "****", default_str_spec);
> > +       else
> > +               buf = number(buf, end, year, default_dec04_spec);
> > +
> > +       if (buf < end)
> > +               *buf = '-';
> 
> Instead of all these checks to avoid overflowing the passed buffer, it
> may be simpler to format everything in a fixed-size buffer on the
> stack,
> and copy whatever will fit in the target buffer at the end.

I dropped that idea since the most heavier call is number().
We still need to do several of them one way or the other.

So, I really don't see much benefit of doing your way.

> > +static noinline_for_stack
> > +char *rtc_str(char *buf, char *end, const struct rtc_time *tm,
> > const char *fmt)
> > +{
> > +       bool have_t = true, have_d = true;
> > +       bool validate = false;
> > +       bool raw = false;
> > +       int count = 1;
> > +       bool found;
> > +
> > +       switch (fmt[++count]) {
> > +       case 'd':
> > +               have_t = false;
> > +               break;
> > +       case 't':
> > +               have_d = false;
> > +               break;
> > +       }
> > +
> > +       /* No %pt[dt] supplied */
> > +       if (have_d && have_t)
> > +               --count;
> 
> First increment count, then rollback.
> What about:
> 
>     switch (fmt[count]) {
>     case 'd':
>             have_t = false;
>             count++;
>             break;
>     case 't':
>             have_d = false;
>             count++;
>             break;
>     }

Or simple:

 default:
  --count;
  break;

?

I really need to come up with the next pile for time64_t which I suppose
will require rethinking of format parsing and printing functions here.

-- 
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy