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 <sstabell...@kernel.org> > > Cheers, > > Stefano Cheers, Michal
