Re: [PATCH v2 2/2] fat: Support file modification times

[Daniel Kiper <dkiper@net-space.pl>]

On Tue, Mar 03, 2020 at 02:41:08PM -0500, David Michael wrote:
> This allows comparing file ages on EFI system partitions.
>
> Signed-off-by: David Michael <fedora.dm0@gmail.com>
> ---
>
> Changes since v1:
>   - Added the previous patch to help support exfat
>   - Added exfat timestamp conversion + setting
>   - Switched to datetime variable name for consistency with the header
>   - Switched to tabs-for-alignment for consistency in the file
>
>  grub-core/fs/fat.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/grub-core/fs/fat.c b/grub-core/fs/fat.c
> index dc493add2..bacf9e60f 100644
> --- a/grub-core/fs/fat.c
> +++ b/grub-core/fs/fat.c
> @@ -26,6 +26,7 @@
>  #include <grub/err.h>
>  #include <grub/dl.h>
>  #include <grub/charset.h>
> +#include <grub/datetime.h>
>  #ifndef MODE_EXFAT
>  #include <grub/fat.h>
>  #else
> @@ -730,6 +731,28 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
>    return grub_errno ? : GRUB_ERR_EOF;
>  }
>
> +static int
> +grub_exfat_timestamp (grub_uint32_t field, grub_uint8_t msec, grub_int32_t *nix) {
> +  struct grub_datetime datetime = {
> +    .year   = (field >> 25) + 1980,
> +    .month  = (field & 0x01E00000) >> 21,
> +    .day    = (field & 0x001F0000) >> 16,
> +    .hour   = (field & 0x0000F800) >> 11,
> +    .minute = (field & 0x000007E0) >>  5,
> +    .second = (field & 0x0000001F) * 2 + (msec >= 100 ? 1 : 0),
> +  };
> +
> +  /* The conversion below allows seconds=60, so don't trust its validation.  */

60 seconds is a valid value in case of leap second. Hence, the question
is: Can 60 seconds be represented properly in exFAT somehow? OK, this
does not happen often. So, if we want ignore that case then at least
I would like to have an explanation that we ignore it due to...

> +  if ((field & 0x1F) > 29)
> +    return 0;

You silently ignore this error. Should not you spit something to the
console in this case? Or maybe at least set grub_errno? This way
user will know that result of comparison should not be trusted...

> +  /* Validate the 10-msec field even though it is rounded down to seconds.  */
> +  if (msec > 199)
> +    return 0;

Ditto...

> +  return grub_datetime2unixtime (&datetime, nix);
> +}
> +
>  #else
>
>  static grub_err_t
> @@ -857,6 +880,24 @@ grub_fat_iterate_dir_next (grub_fshelp_node_t node,
>    return grub_errno ? : GRUB_ERR_EOF;
>  }
>
> +static int
> +grub_fat_timestamp (grub_uint16_t time, grub_uint16_t date, grub_int32_t *nix) {
> +  struct grub_datetime datetime = {
> +    .year   = (date >> 9) + 1980,
> +    .month  = (date & 0x01E0) >> 5,
> +    .day    = (date & 0x001F),
> +    .hour   = (time >> 11),
> +    .minute = (time & 0x07E0) >> 5,
> +    .second = (time & 0x001F) * 2,
> +  };
> +
> +  /* The conversion below allows seconds=60, so don't trust its validation.  */
> +  if ((time & 0x1F) > 29)
> +    return 0;

Ditto...

Daniel