Re: [PATCH v2 1/2] datetime: Support dates outside of 1901..2038 range

[Daniel Kiper]

On Sat, Aug 17, 2024 at 08:30:22PM +0300, Vladimir Serbinenko wrote:
> Signed-off-by: Vladimir Serbinenko <phcoder@gmail.com>
> ---
>  grub-core/lib/datetime.c | 31 ++++++++++++++++++++++++-------
>  include/grub/datetime.h  | 15 +++++++--------
>  2 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/grub-core/lib/datetime.c b/grub-core/lib/datetime.c
> index 9120128ca..e4bdc5c4f 100644
> --- a/grub-core/lib/datetime.c
> +++ b/grub-core/lib/datetime.c
> @@ -59,6 +59,8 @@ grub_get_weekday_name (struct grub_datetime *datetime)
>  #define SECPERDAY (24*SECPERHOUR)
>  #define DAYSPERYEAR 365
>  #define DAYSPER4YEARS (4*DAYSPERYEAR+1)
> +#define DAYSPER100YEARS (100*DAYSPERYEAR+24)
> +#define DAYSPER400YEARS (400*DAYSPERYEAR+97)
>
>
>  void
> @@ -71,7 +73,7 @@ grub_unixtime2datetime (grub_int64_t nix, struct 
> grub_datetime *datetime)
>    /* Convenience: let's have 3 consecutive non-bissextile years
>       at the beginning of the counting date. So count from 1901. */
>    int days_epoch;
> -  /* Number of days since 1st Januar, 1901.  */
> +  /* Number of days since 1st January, 1 (proleptic).  */
>    unsigned days;
>    /* Seconds into current day.  */
>    unsigned secs_in_day;
> @@ -87,9 +89,25 @@ grub_unixtime2datetime (grub_int64_t nix, struct 
> grub_datetime *datetime)
>      days_epoch = grub_divmod64 (nix, SECPERDAY, NULL);
>
>    secs_in_day = nix - days_epoch * SECPERDAY;
> -  days = days_epoch + 69 * DAYSPERYEAR + 17;
> +  days = days_epoch + 369 * DAYSPERYEAR + 17 + 24 * 3 + 4 * DAYSPER400YEARS;

I can imagine how this math was build but I would prefer if you add
a comment where these numbers come from...

> -  datetime->year = 1901 + 4 * (days / DAYSPER4YEARS);
> +  datetime->year = 1 + 400 * (days / DAYSPER400YEARS);
> +  days %= DAYSPER400YEARS;
> +
> +  /* On 31st December of bissextile years 365 days from the beginning
> +     of the year elapsed but year isn't finished yet */

I think the comment is not precise. It happens once per 400 years.
Right? I think it should be clarified.

> +  if (days / DAYSPER100YEARS == 4)
> +    {
> +      datetime->year += 396;
> +      days -= 396*DAYSPERYEAR + 96;

Where do these numbers come from? The math begs for comment.

> +    }
> +  else
> +    {
> +      datetime->year += 100 * (days / DAYSPER100YEARS);
> +      days %= DAYSPER100YEARS;

Ditto.

> +    }
> +
> +  datetime->year += 4 * (days / DAYSPER4YEARS);
>    days %= DAYSPER4YEARS;
>    /* On 31st December of bissextile years 365 days from the beginning
>       of the year elapsed but year isn't finished yet */
> @@ -103,11 +121,10 @@ grub_unixtime2datetime (grub_int64_t nix, struct 
> grub_datetime *datetime)
>        datetime->year += days / DAYSPERYEAR;
>        days %= DAYSPERYEAR;
>      }
> +  int isbisextile = datetime->year % 4 == 0 && (datetime->year % 100 != 0 || 
> datetime->year % 400 == 0);

Could you use bool instead? And define variable at the beginning of function?

>    for (i = 0; i < 12
> -      && days >= (i==1 && datetime->year % 4 == 0
> -                   ? 29 : months[i]); i++)
> -    days -= (i==1 && datetime->year % 4 == 0
> -                         ? 29 : months[i]);
> +      && days >= (i==1 && isbisextile ? 29 : months[i]); i++)
> +    days -= (i==1 && isbisextile ? 29 : months[i]);
>    datetime->month = i + 1;
>    datetime->day = 1 + days;
>    datetime->hour = (secs_in_day / SECPERHOUR);
> diff --git a/include/grub/datetime.h b/include/grub/datetime.h
> index bcec636f0..9289b0d00 100644
> --- a/include/grub/datetime.h
> +++ b/include/grub/datetime.h
> @@ -54,7 +54,7 @@ void grub_unixtime2datetime (grub_int64_t nix,
>  static inline int
>  grub_datetime2unixtime (const struct grub_datetime *datetime, grub_int64_t 
> *nix)
>  {
> -  grub_int32_t ret;
> +  grub_int64_t ret;
>    int y4, ay;
>    const grub_uint16_t monthssum[12]
>      = { 0,
> @@ -75,15 +75,11 @@ grub_datetime2unixtime (const struct grub_datetime 
> *datetime, grub_int64_t *nix)
>    const int SECPERHOUR = 60 * SECPERMIN;
>    const int SECPERDAY = 24 * SECPERHOUR;
>    const int SECPERYEAR = 365 * SECPERDAY;
> -  const int SECPER4YEARS = 4 * SECPERYEAR + SECPERDAY;
> +  const grub_int64_t SECPER4YEARS = 4 * SECPERYEAR + SECPERDAY;
>
> -  if (datetime->year > 2038 || datetime->year < 1901)
> -    return 0;
>    if (datetime->month > 12 || datetime->month < 1)
>      return 0;
>
> -  /* In the period of validity of unixtime all years divisible by 4
> -     are bissextile*/
>    /* Convenience: let's have 3 consecutive non-bissextile years
>       at the beginning of the epoch. So count from 1973 instead of 1970 */
>    ret = 3 * SECPERYEAR + SECPERDAY;
> @@ -94,13 +90,16 @@ grub_datetime2unixtime (const struct grub_datetime 
> *datetime, grub_int64_t *nix)
>    ret += y4 * SECPER4YEARS;
>    ret += ay * SECPERYEAR;
>
> +  ret -= ((datetime->year - 1) / 100 - (datetime->year - 1) / 400 - 15) * 
> SECPERDAY;

Again, please comment this math...

>    ret += monthssum[datetime->month - 1] * SECPERDAY;
> -  if (ay == 3 && datetime->month >= 3)
> +  int isbisextile = ay == 3 && (datetime->year % 100 != 0 || datetime->year 
> % 400 == 0);

Ditto.

And could you use bool instead of int?

> +  if (isbisextile && datetime->month >= 3)
>      ret += SECPERDAY;
>
>    ret += (datetime->day - 1) * SECPERDAY;
>    if ((datetime->day > months[datetime->month - 1]
> -       && (!ay || datetime->month != 2 || datetime->day != 29))
> +       && !(isbisextile && datetime->month == 2 && datetime->day == 29))

It would be nice to add a comment here too.

Daniel