[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] xen/console: Fix incorrect format tags for struct tm members
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |