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

[Andy Shevchenko]

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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
Intel Finland Oy