| Message ID | 20220610083358.101412-4-michal.orzel@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | small fixes | expand |
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. Jan > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -844,7 +844,7 @@ static void printk_start_of_line(const char *prefix) > /* nothing */; > else if ( mode == TSM_DATE ) > { > - snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u %02u:%02u:%02u] ", > + snprintf(tstr, sizeof(tstr), "[%04d-%02d-%02d %02d:%02d:%02d] ", > 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday, > tm.tm_hour, tm.tm_min, tm.tm_sec); > break; > @@ -852,7 +852,7 @@ static void printk_start_of_line(const char *prefix) > else > { > snprintf(tstr, sizeof(tstr), > - "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ", > + "[%04d-%02d-%02d %02d:%02d:%02d.%03"PRIu64"] ", > 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday, > tm.tm_hour, tm.tm_min, tm.tm_sec, nsec / 1000000); > break;
Hi Jan, On 10.06.2022 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. > The reason why I did not do this is to be coherent with Linux kernel (include/linux/time.h). I believe the time.h code in Xen comes from Linux. So if the Linux time.h defines these fields as integers and uses proper %d format specifiers (unlike Xen), I think my solution is better. > Jan Cheers, Michal
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? Juergen
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. Jan
On 10.06.2022 11:50, Michal Orzel wrote: > On 10.06.2022 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. >> > > The reason why I did not do this is to be coherent with Linux kernel (include/linux/time.h). > I believe the time.h code in Xen comes from Linux. > > So if the Linux time.h defines these fields as integers and uses proper %d format specifiers (unlike Xen), > I think my solution is better. One can view it that way, sure. I don't. But I also won't nak this change. Jan
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. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> Cheers, Stefano
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@kernel.org> > > Cheers, > > Stefano Cheers, Michal
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index f9937c5134..beb44fe06f 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -844,7 +844,7 @@ static void printk_start_of_line(const char *prefix) /* nothing */; else if ( mode == TSM_DATE ) { - snprintf(tstr, sizeof(tstr), "[%04u-%02u-%02u %02u:%02u:%02u] ", + snprintf(tstr, sizeof(tstr), "[%04d-%02d-%02d %02d:%02d:%02d] ", 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec); break; @@ -852,7 +852,7 @@ static void printk_start_of_line(const char *prefix) else { snprintf(tstr, sizeof(tstr), - "[%04u-%02u-%02u %02u:%02u:%02u.%03"PRIu64"] ", + "[%04d-%02d-%02d %02d:%02d:%02d.%03"PRIu64"] ", 1900 + tm.tm_year, tm.tm_mon + 1, tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec, nsec / 1000000); break;
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. Signed-off-by: Michal Orzel <michal.orzel@arm.com> --- xen/drivers/char/console.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)