Re: [2.06 RELEASE PATCH 1/3] fs: Use 64bit type for filesystem timestamp

[Daniel Kiper]

On Wed, May 05, 2021 at 10:32:31AM +0200, Javier Martinez Canillas wrote:
> From: Carlos Maiolino <cmaiolino@redhat.com>
>
> Some filesystems nowadays uses 64bit types for timestamps, so, update

s/64bit/64-bit/

> grub_dirhook_info struct to use an int64 type to store mtime. This also

s/int64/grub_int64_t/

> updates grub_unixtime2datetime() to receive a 64-bit timestamp
> argument and do 64bit-safe divisions.

s/64bit-safe/64-bit-safe/

> All the remaining conversion from 32 to 64 should be safe, as 32 to 64

s/32/32-bits/
s/64/64-bits/

...and below please...

> attributions will be implicitly casted. The most crictical part in the
> 32 to 64bits conversion is on grub_unixtime2datetime() where it needs
> to deal with the 64bit type, specially with division in x86_architectures,

I think it is not "x86_architectures" specific. In general it applies to
all architectures which does not provide native 64-bit division. So,
I think this sentence should be rephrased a bit.

> so, for that, the grub_divmod64() helper has been used.
>
> These changes enables grub to support dates beyond y2038.
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

This line should be dropped.

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---

[...]

> diff --git a/grub-core/lib/datetime.c b/grub-core/lib/datetime.c
> index 95b8c9ff5e3..3e84fa1dbc4 100644
> --- a/grub-core/lib/datetime.c
> +++ b/grub-core/lib/datetime.c
> @@ -19,6 +19,7 @@
>
>  #include <grub/datetime.h>
>  #include <grub/i18n.h>
> +#include <grub/misc.h>
>
>  static const char *const grub_weekday_names[] =
>  {
> @@ -60,9 +61,10 @@ grub_get_weekday_name (struct grub_datetime *datetime)
>
>
>  void
> -grub_unixtime2datetime (grub_int32_t nix, struct grub_datetime *datetime)
> +grub_unixtime2datetime (grub_int64_t nix, struct grub_datetime *datetime)
>  {
>    int i;
> +  grub_uint64_t rem;

If you do not care about reminder please drop it and use NULL instead.

>    grub_uint8_t months[12] = {31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};
>    /* In the period of validity of unixtime all years divisible by 4
>       are bissextile*/
> @@ -75,9 +77,16 @@ grub_unixtime2datetime (grub_int32_t nix, struct 
> grub_datetime *datetime)
>    unsigned secs_in_day;
>    /* Transform C divisions and modulos to mathematical ones */
>    if (nix < 0)
> -    days_epoch = -(((unsigned) (SECPERDAY-nix-1)) / SECPERDAY);
> +    /*
> +     * the division here shouldn't be larger than INT_MAX,
> +     * so, it's safe to store the result back in an int
> +     */
> +    days_epoch = -(grub_divmod64(((grub_int64_t)(SECPERDAY) - nix - 1),
> +                             SECPERDAY,
> +                             &rem));

       days_epoch = -(grub_divmod64 (((grub_int64_t) (SECPERDAY) - nix - 1),
                                     SECPERDAY, NULL));

Missing spaces...

>    else
> -    days_epoch = ((unsigned) nix) / SECPERDAY;
> +    days_epoch = grub_divmod64(nix, SECPERDAY, &rem);

       days_epoch = grub_divmod64 (nix, SECPERDAY, NULL);

Ditto...

>    secs_in_day = nix - days_epoch * SECPERDAY;
>    days = days_epoch + 69 * DAYSPERYEAR + 17;
>
> diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
> index e33be51f83b..6fb5627025d 100644
> --- a/grub-core/net/bootp.c
> +++ b/grub-core/net/bootp.c
> @@ -468,7 +468,7 @@ send_dhcp_packet (struct grub_net_network_level_interface 
> *iface)
>    grub_err_t err;
>    struct grub_net_bootp_packet *pack;
>    struct grub_datetime date;
> -  grub_int32_t t = 0;
> +  grub_int64_t t = 0;
>    struct grub_net_buff *nb;
>    struct udphdr *udph;
>    grub_net_network_level_address_t target;
> diff --git a/grub-core/normal/misc.c b/grub-core/normal/misc.c
> index 8bb6da31fb3..f7e9e3ac4a1 100644
> --- a/grub-core/normal/misc.c
> +++ b/grub-core/normal/misc.c
> @@ -136,7 +136,7 @@ grub_normal_print_device_info (const char *name)
>           }
>         if (fs->fs_mtime)
>           {
> -           grub_int32_t tm;
> +           grub_int64_t tm;
>             struct grub_datetime datetime;
>             (fs->fs_mtime) (dev, &tm);
>             if (grub_errno == GRUB_ERR_NONE)
> diff --git a/grub-core/tests/sleep_test.c b/grub-core/tests/sleep_test.c
> index 3d11c717cb7..63f6713165f 100644
> --- a/grub-core/tests/sleep_test.c
> +++ b/grub-core/tests/sleep_test.c
> @@ -32,7 +32,7 @@ static void
>  sleep_test (void)
>  {
>    struct grub_datetime st, en;
> -  grub_int32_t stu = 0, enu = 0;
> +  grub_int64_t stu = 0, enu = 0;
>    int is_delayok;
>    grub_test_assert (!grub_get_datetime (&st), "Couldn't retrieve start 
> time");
>    grub_millisleep (10000);
> @@ -45,7 +45,7 @@ sleep_test (void)
>    if (enu - stu >= 15 && enu - stu <= 17)
>      is_delayok = 1;
>  #endif
> -  grub_test_assert (is_delayok, "Interval out of range: %d", enu-stu);
> +  grub_test_assert (is_delayok, "Interval out of range: %lld", enu-stu);

"%lld" is not correct here. I would define PRIdGRUB_INT64_T in
include/grub/types.h and use it here.

...and s/enu-stu/enu - stu/

Daniel