Am 20.08.20 um 03:54 schrieb HAGIO KAZUHITO(萩尾 一仁):
> Hi Mathias,
>
> Thanks for the review!
>
> -----Original Message-----
>> Hi Kazuhito,
>>
>> Am 19.08.20 um 08:32 schrieb HAGIO KAZUHITO(萩尾 一仁):
>>> Currently it's not easy to distinguish which time zone the output of
>>> DATE uses:
>>>
>>> crash> sys | grep DATE
>>> DATE: Thu Nov 29 06:44:02 2018
>>>
>>> Let's introduce ctime_tz() function like ctime() but explicitly appends
>>> the time zone to its result string.
>>>
>>> DATE: Thu Nov 29 06:44:02 JST 2018
>>>
>>> Resolves: https://github.com/crash-utility/crash/issues/62
>>> Suggested-by: Jacob Wen <jian.w....@oracle.com>
>>> Signed-off-by: Kazuhito Hagio <k-hagio...@nec.com>
>>> ---
>>> defs.h | 1 +
>>> help.c | 3 +--
>>> kernel.c | 16 ++++++----------
>>> tools.c | 28 ++++++++++++++++++++++++++++
>>> 4 files changed, 36 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/defs.h b/defs.h
>>> index 17e98763362b..e7dec7c672a6 100644
>>> --- a/defs.h
>>> +++ b/defs.h
>>> @@ -5096,6 +5096,7 @@ char *strdupbuf(char *);
>>> void sigsetup(int, void *, struct sigaction *, struct sigaction *);
>>> #define SIGACTION(s, h, a, o) sigsetup(s, h, a, o)
>>> char *convert_time(ulonglong, char *);
>>> +char *ctime_tz(time_t *);
>>> void stall(ulong);
>>> char *pages_to_size(ulong, char *);
>>> int clean_arg(void);
>>> diff --git a/help.c b/help.c
>>> index b707cfa58826..d3427a36829f 100644
>>> --- a/help.c
>>> +++ b/help.c
>>> @@ -9299,8 +9299,7 @@ display_README(void)
>>> fprintf(fp, " GNU gdb %s\n", pc->gdb_version);
>>> } else if (STREQ(README[i], README_DATE)) {
>>> time(&time_now);
>>> - fprintf(fp, " DATE: %s\n",
>>> - strip_linefeeds(ctime(&time_now)));
>>> + fprintf(fp, " DATE: %s\n",
>>> ctime_tz(&time_now));
>>> } else if (STREQ(README[i], README_HELP_MENU)) {
>>> display_help_screen(" ");
>>> } else if (STREQ(README[i], README_GPL_INFO)) {
>>> diff --git a/kernel.c b/kernel.c
>>> index f179375f2d3d..92bfe6329900 100644
>>> --- a/kernel.c
>>> +++ b/kernel.c
>>> @@ -225,10 +225,9 @@ kernel_init()
>>> get_xtime(&kt->date);
>>> if (CRASHDEBUG(1))
>>> fprintf(fp, "xtime timespec.tv_sec: %lx: %s\n",
>>> - kt->date.tv_sec,
>>> strip_linefeeds(ctime(&kt->date.tv_sec)));
>>> + kt->date.tv_sec, ctime_tz(&kt->date.tv_sec));
>>> if (kt->flags2 & GET_TIMESTAMP) {
>>> - fprintf(fp, "%s\n\n",
>>> - strip_linefeeds(ctime(&kt->date.tv_sec)));
>>> + fprintf(fp, "%s\n\n", ctime_tz(&kt->date.tv_sec));
>>> clean_exit(0);
>>> }
>>>
>>> @@ -5178,7 +5177,7 @@ dump_log_entry(char *logptr, int msg_flags)
>>> rem = (ulonglong)ts_nsec % (ulonglong)1000000000;
>>> if (msg_flags & SHOW_LOG_CTIME) {
>>> time_t t = kt->boot_date.tv_sec + nanos;
>>> - sprintf(buf, "[%s] ", strip_linefeeds(ctime(&t)));
>>> + sprintf(buf, "[%s] ", ctime_tz(&t));
>>> }
>>> else
>>> sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
>>> @@ -5553,8 +5552,7 @@ display_sys_stats(void)
>>>
>>> if (ACTIVE())
>>> get_xtime(&kt->date);
>>> - fprintf(fp, " DATE: %s\n",
>>> - strip_linefeeds(ctime(&kt->date.tv_sec)));
>>> + fprintf(fp, " DATE: %s\n", ctime_tz(&kt->date.tv_sec));
>>> fprintf(fp, " UPTIME: %s\n", get_uptime(buf, NULL));
>>> fprintf(fp, "LOAD AVERAGE: %s\n", get_loadavg(buf));
>>> fprintf(fp, " TASKS: %ld\n", RUNNING_TASKS());
>>> @@ -6043,10 +6041,8 @@ dump_kernel_table(int verbose)
>>> kt->source_tree : "(not used)");
>>> if (!(pc->flags & KERNEL_DEBUG_QUERY) && ACTIVE())
>>> get_xtime(&kt->date);
>>> - fprintf(fp, " date: %s\n",
>>> - strip_linefeeds(ctime(&kt->date.tv_sec)));
>>> - fprintf(fp, " boot_ date: %s\n",
>>> - strip_linefeeds(ctime(&kt->boot_date.tv_sec)));
>>> + fprintf(fp, " date: %s\n", ctime_tz(&kt->date.tv_sec));
>>> + fprintf(fp, " boot_date: %s\n",
>>> ctime_tz(&kt->boot_date.tv_sec));
>>> fprintf(fp, " proc_version: %s\n",
>>> strip_linefeeds(kt->proc_version));
>>> fprintf(fp, " new_utsname: \n");
>>> fprintf(fp, " .sysname: %s\n", uts->sysname);
>>> diff --git a/tools.c b/tools.c
>>> index 85c81668e40e..4654e7458a3e 100644
>>> --- a/tools.c
>>> +++ b/tools.c
>>> @@ -6318,6 +6318,34 @@ convert_time(ulonglong count, char *buf)
>>> }
>>>
>>> /*
>>> + * Convert a calendar time into a null-terminated string like ctime(), but
>>> + * the result string contains the time zone string and does not ends with a
>>> + * linefeed ('\n').
>>> + *
>>> + * NOTE: The return value points to a statically allocated string which is
>>> + * overwritten by subsequent calls.
>>> + */
>>> +char *
>>> +ctime_tz(time_t *timep)
>>> +{
>>> + static char buf[64];
>>> + struct tm *tm;
>>> + size_t size;
>>> +
>>> + tm = localtime(timep);
>>
>>> + if (!tm) {
>>> + snprintf(buf, sizeof(buf), "%ld", *timep);
>>> + return buf;
>>> + }
>>
>> I don't think this is especially useful, better fall back to ctime() in
>> this case, but see below.
>
> but if tm is NULL, then the fallback ctime() also will return NULL
> because ctime(t) is equivalent to asctime(localtime(t)).
> Aren't "UNIX seconds since epoch" better than "(null)" ?
Valid point! Agreed.
>>> +
>>> + size = strftime(buf, sizeof(buf), "%a %b %e %T %Z %Y", tm);
>>> + if (!size)
>>> + return strip_linefeeds(ctime(timep));
>>
>> Maybe you should mention in the leading comment that this function falls
>> back to ctime() in case it fails to generate a timestamp with the time
>> zone info?
>
> Agreed, will do in v2.
Thanks,
Mathias
>
> Thanks,
> Kazu
>
>>
>>> +
>>> + return buf;
>>> +}
>>> +
>>> +/*
>>> * Stall for a number of microseconds.
>>> */
>>> void
>>
>> So, to simplify ctime_tz() and avoid the "UNIX seconds since epoch" case
>> I'd suggest something like this instead:
>>
>> char *
>> ctime_tz(time_t *timep)
>> {
>> static char buf[64];
>> struct tm *tm;
>>
>> tm = localtime(timep);
>> if (tm && strftime(buf, sizeof(buf), "%a %b %e %T %Z %Y", tm))
>> return buf;
>>
>> return strip_linefeeds(ctime(timep));
>> }
>>
>> Beside that, patch looks good to me!
>>
>> Thanks,
>> Mathias
--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility
