Re: [PATCH 2/9] fs: print change date in directory listing for FAT

Hi Heinrich,

On 2026-05-18T05:57:19, Heinrich Schuchardt
<[email protected]> wrote:
> fs: print change date in directory listing for FAT
>
> fs_ls_generic() displays file sizes but no timestamps. The FAT
> filesystem stores a change date in every directory entry and already
> populates fs_dirent::change_time in fat_readdir(). Print the date
> alongside the file size for filesystems that provide it.
>
> Add a u32 capability bitmap (caps) to struct fstype_info. Each bit
> documents a property that the filesystem's readdir() implementation
> guarantees:
>
>   FS_CAP_DATE  BIT(0)  change_time in fs_dirent is valid
>
> fs_ls_generic() tests FS_CAP_DATE once before the loop to select a
> consistent output format for the entire listing:
>
>   12345678  2024-03-15 09:30  filename.txt    (FAT)
>   12345678   filename.txt                     (ext4, squashfs, ...)
>
> Set FS_CAP_DATE for FAT. fat2rtc() loses the __maybe_unused annotation
> [...]
>
> fs/fat/fat.c |  2 +-
>  fs/fs.c      | 38 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 36 insertions(+), 4 deletions(-)

> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> @@ -1539,7 +1539,7 @@ int fat_readdir(struct fs_dir_stream *dirs, struct fs_dirent **dentp)
>
>       memset(dent, 0, sizeof(*dent));
>       strcpy(dent->name, dir->itr.name);
> -     if (CONFIG_IS_ENABLED(EFI_LOADER)) {
> +     if (!IS_ENABLED(CONFIG_XPL_BUILD)) {
>               dent->attr = dir->itr.dent->attr;

The commit message says fat2rtc() loses __maybe_unused, but the diff
does not touch fat2rtc()

> diff --git a/fs/fs.c b/fs/fs.c
> @@ -38,6 +38,11 @@ static int fs_dev_part;
>  static struct disk_partition fs_partition;
>  static int fs_type = FS_TYPE_ANY;
>
> +/*
> + * define FS_CAP_DATE - readdir() populates fs_dirent::change_time
> + */
> +#define FS_CAP_DATE  BIT(0)

The leading 'define' reads like a half-finished kernel-doc. Please
make this a plain comment:

   /* FS_CAP_DATE: readdir() populates fs_dirent::change_time */

> diff --git a/fs/fs.c b/fs/fs.c
> @@ -98,10 +107,19 @@ static inline int fs_ls_unsupported(const char *dirname)
>       return -1;
>  }
>
> +/* Forward declaration - defined after fstypes[] */
> +static struct fstype_info *fs_get_info(int fstype);
> +

Patch 1 moved struct fstype_info to the top precisely to avoid this.
Please move fs_get_info() up next to the struct (or just before
fs_ls_generic()) and drop the forward decl.

> diff --git a/fs/fs.c b/fs/fs.c
> @@ -112,15 +130,26 @@ static int fs_ls_generic(const char *dirname)
>       while ((dent = fs_readdir(dirs))) {
>               if (dent->type == FS_DT_DIR) {
> -                     printf("            %s/\n", dent->name);
> +                     printf("          ");
>                       ndirs++;
>               } else if (dent->type == FS_DT_LNK) {
> -                     printf("    <SYM>   %s\n", dent->name);
> +                     printf("    <SYM> ");
>                       nfiles++;
>               } else {
> -                     printf(" %8lld   %s\n", dent->size, dent->name);
> +                     printf(" %8lld ", dent->size);
>                       nfiles++;
>               }

This silently narrows the gap between size and name from three spaces
to one for every filesystem, not only the ones opting into FS_CAP_DATE

That is why patch 9 has to fix up the erofs test, but ubifs, sandbox
etc. get the same churn for no functional gain. How about keeping the
original layout when has_date is false?

> diff --git a/fs/fs.c b/fs/fs.c
> @@ -112,15 +130,26 @@ static int fs_ls_generic(const char *dirname)
> +             if (has_date)
> +                     printf("%04d-%02d-%02d %02d:%02d ",
> +                            dent->change_time.tm_year,
> +                            dent->change_time.tm_mon,
> +                            dent->change_time.tm_mday,
> +                            dent->change_time.tm_hour,
> +                            dent->change_time.tm_min);

If the date was never set, this presumably prints '0000-00-00 00:00
filename', which is more misleading than no date at all. It is is
unset it would be better to show nothing.

Regards,
Simon