[PATCH v2] erofs-utils: mkfs: use scandir for stable output

[Jooyung Han]

Hi Gao,

I found that in the loop erofs_iget_from_srcpath() is called in
different order due to readdir and erofs_iget_from_srcpath() calls
erofs_new_inode() which fills i_ino[0] for newly created inode. I
think this i_ino[0] having different values caused the difference in
the output.


On Tue, Dec 3, 2024 at 10:38 AM Gao Xiang <hsiangkao at linux.alibaba.com> wrote:
>
> Hi Jooyung,
>
> On 2024/12/3 08:27, Jooyung Han wrote:
> > The iteration order of opendir/readdir depends on filesystem
> > implementation. Hence, even with the same contents, the filesystem of
> > the input directory affects the output.
> >
> > In this change, opendir/readdir is replaced with scandir for stable
> > order of iteration. This produces the same output regardless of the
> > filesystem of the input directory.
> >
> > Test: mkfs.erofs ... inputdir(ext3)
> > Test: mkfs.erofs ... inputdir(tmpfs)
> >    # should generate the same output
> > Signed-off-by: Jooyung Han <jooyung at google.com>
>
> Thanks for the patch.  I haven't tested the current behavior (1.8.2),
> but EROFS will sort all dirents in a directory in
> erofs_prepare_dir_file(). And then dump all subdirs in
> erofs_mkfs_dump_tree().
>
> It seems both dirents and inodes are sorted in the alphabetical
> order, could you give more hints about this?
>
> Thanks,
> Gao Xiang
>
> > ---
> >
> > v1: https://lore.kernel.org/linux-erofs/20241202232620.3604736-1-jooyung@google.com/
> > change since v1:
> >   - modify commit msg (no change-id/bug/typo)
> >   - rename the label to err_cleanup
> >
> >   lib/inode.c | 39 ++++++++++++++-------------------------
> >   1 file changed, 14 insertions(+), 25 deletions(-)
> >
> > diff --git a/lib/inode.c b/lib/inode.c
> > index f553bec..097deef 100644
> > --- a/lib/inode.c
> > +++ b/lib/inode.c
> > @@ -1422,37 +1422,25 @@ static void erofs_mkfs_flushjobs(struct erofs_sb_info *sbi)
> >   static int erofs_mkfs_handle_directory(struct erofs_inode *dir)
> >   {
> >       struct erofs_sb_info *sbi = dir->sbi;
> > -     DIR *_dir;
> > -     struct dirent *dp;
> > +     struct dirent *dp, **dent;
> > +     int i, num_entries;
> >       struct erofs_dentry *d;
> >       unsigned int nr_subdirs, i_nlink;
> >       int ret;
> >
> > -     _dir = opendir(dir->i_srcpath);
> > -     if (!_dir) {
> > -             erofs_err("failed to opendir at %s: %s",
> > +     num_entries = scandir(dir->i_srcpath, &dent, NULL, alphasort);
> > +     if (num_entries == -1) {
> > +             erofs_err("failed to scandir at %s: %s",
> >                         dir->i_srcpath, erofs_strerror(-errno));
> >               return -errno;
> >       }
> >
> >       nr_subdirs = 0;
> >       i_nlink = 0;
> > -     while (1) {
> > +     for (i = 0; i < num_entries; free(dent[i]), i++) {
> >               char buf[PATH_MAX];
> >               struct erofs_inode *inode;
> > -
> > -             /*
> > -              * set errno to 0 before calling readdir() in order to
> > -              * distinguish end of stream and from an error.
> > -              */
> > -             errno = 0;
> > -             dp = readdir(_dir);
> > -             if (!dp) {
> > -                     if (!errno)
> > -                             break;
> > -                     ret = -errno;
> > -                     goto err_closedir;
> > -             }
> > +             dp = dent[i];
> >
> >               if (is_dot_dotdot(dp->d_name)) {
> >                       ++i_nlink;
> > @@ -1466,17 +1454,17 @@ static int erofs_mkfs_handle_directory(struct erofs_inode *dir)
> >               d = erofs_d_alloc(dir, dp->d_name);
> >               if (IS_ERR(d)) {
> >                       ret = PTR_ERR(d);
> > -                     goto err_closedir;
> > +                     goto err_cleanup;
> >               }
> >
> >               ret = snprintf(buf, PATH_MAX, "%s/%s", dir->i_srcpath, d->name);
> >               if (ret < 0 || ret >= PATH_MAX)
> > -                     goto err_closedir;
> > +                     goto err_cleanup;
> >
> >               inode = erofs_iget_from_srcpath(sbi, buf);
> >               if (IS_ERR(inode)) {
> >                       ret = PTR_ERR(inode);
> > -                     goto err_closedir;
> > +                     goto err_cleanup;
> >               }
> >               d->inode = inode;
> >               d->type = erofs_mode_to_ftype(inode->i_mode);
> > @@ -1484,7 +1472,7 @@ static int erofs_mkfs_handle_directory(struct erofs_inode *dir)
> >               erofs_dbg("file %s added (type %u)", buf, d->type);
> >               nr_subdirs++;
> >       }
> > -     closedir(_dir);
> > +     free(dent);
> >
> >       ret = erofs_init_empty_dir(dir);
> >       if (ret)
> > @@ -1506,8 +1494,9 @@ static int erofs_mkfs_handle_directory(struct erofs_inode *dir)
> >
> >       return erofs_mkfs_go(sbi, EROFS_MKFS_JOB_DIR, &dir, sizeof(dir));
> >
> > -err_closedir:
> > -     closedir(_dir);
> > +err_cleanup:
> > +     for (; i < num_entries; free(dent[i]), i++);
> > +     free(dent);
> >       return ret;
> >   }
> >
>