Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Stefano,

On 11.06.2022 01:35, Stefano Stabellini wrote:
> On Fri, 10 Jun 2022, Jan Beulich wrote:
>> On 10.06.2022 11:51, Juergen Gross wrote:
>>> On 10.06.22 11:44, Jan Beulich wrote:
>>>> On 10.06.2022 10:33, Michal Orzel wrote:
>>>>> All the members of struct tm are defined as integers but the format tags
>>>>> used in console driver for snprintf wrongly expect unsigned values. Fix
>>>>> the tags to expect integers.
>>>>
>>>> Perhaps do things the other way around - convert field types to unsigned
>>>> unless negative values can be stored there? This would match our general
>>>> aim of using unsigned types when only non-negative values can be held in
>>>> variables / parameters / fields.
>>>
>>> Don't you think keeping struct tm in sync with the Posix definition should
>>> be preferred here?
>>
>> Not necessarily, no. It's not just POSIX which has a (imo bad) habit of
>> using plain "int" even for values which can never go negative.
> 
> I committed the other two patches in the series because already acked
> and straightforward.
> 
> In this case, I think the straightforward/mechanical fix is the one
> Michal suggested in this patch: fixing %u to be %d. We could of course
> consider changing the definition of tm, and there are valid reasons to
> do that as Jan pointed out, but I think this patch is valid as-in
> anyway.
> 
> So I am happy to give my reviewed-by for this version of the patch, and
> we can still consider changing tm to use unsigned if someone feels like
> proposing a patch for that.

It is not true that the members of struct tm cannot go negative.
Examples:
1) tm_year is used to store a number of years elapsed since 1900.
To express years before 1900, this value must be negative.
2) tm_isdst is used to inform whether DST is in effect. Negative value (-1)
means that no information is available.

The above rules are taken into account in a gmtime definition (common/time.c).

The signed/unsigned debate also applies to a calendar time defintion.
The concept of negative value is used to express the time before epoch.

Xen is using signed s_time_t for system time all over the codebase without
any valid reason other than to be coherent with POSIX definition of time_t.

> 
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> 
> Cheers,
> 
> Stefano

Cheers,
Michal