Re: [PATCH v2 2/2] hfsplus: extract hidden directory search into a helper function

[Viacheslav Dubeyko <Slava.Dubeyko@ibm.com>]

On Wed, 2026-03-18 at 23:00 +0800, Zilin Guan wrote:
> In hfsplus_fill_super(), the process of looking up the hidden directory
> involves initializing a catalog search, building a search key, reading
> the b-tree record, and releasing the search data.
> 
> Currently, this logic is open-coded directly within the main superblock
> initialization routine. This makes hfsplus_fill_super() quite lengthy
> and its error handling paths less straightforward.
> 
> Extract the hidden directory search sequence into a new helper function,
> hfsplus_get_hidden_dir_entry(). This improves overall code readability,
> cleanly encapsulates the hfs_find_data lifecycle, and simplifies the
> error exits in hfsplus_fill_super().
> 
> Moreover, the error handling logic for hfs_brec_read is updated to mirror
> hfsplus_lookup(). This gracefully handles the -ENOENT case, keeping the
> directory search behavior consistent with the rest of the filesystem.
> 
> Signed-off-by: Zilin Guan <zilin@seu.edu.cn>
> ---
>  fs/hfsplus/super.c | 41 ++++++++++++++++++++++++++++-------------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index f396fee19ab8..df3f104298cf 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -424,12 +424,33 @@ void hfsplus_prepare_volume_header_for_commit(struct hfsplus_vh *vhdr)
>  	vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
>  }
>  
> +static inline int hfsplus_get_hidden_dir_entry(struct super_block *sb,
> +					       const struct qstr *str,
> +					       hfsplus_cat_entry *entry)
> +{
> +	struct hfs_find_data fd;
> +	int err;
> +
> +	err = hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
> +	if (err)

Why not unlikely(err) here too?

> +		return err;
> +
> +	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, str);
> +	if (unlikely(err < 0))

The hfsplus_cat_build_key() return error code or 0. So, we can use unlikely(err)
here.

> +		goto free_fd;
> +
> +	err = hfs_brec_read(&fd, entry, sizeof(*entry));
> +
> +free_fd:
> +	hfs_find_exit(&fd);
> +	return err;
> +}
> +
>  static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  {
>  	struct hfsplus_vh *vhdr;
>  	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
>  	hfsplus_cat_entry entry;
> -	struct hfs_find_data fd;
>  	struct inode *root, *inode;
>  	struct qstr str;
>  	struct nls_table *nls;
> @@ -565,16 +586,11 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  
>  	str.len = sizeof(HFSP_HIDDENDIR_NAME) - 1;
>  	str.name = HFSP_HIDDENDIR_NAME;
> -	err = hfs_find_init(sbi->cat_tree, &fd);
> -	if (err)
> -		goto out_put_root;
> -	err = hfsplus_cat_build_key(sb, fd.search_key, HFSPLUS_ROOT_CNID, &str);
> -	if (unlikely(err < 0)) {
> -		hfs_find_exit(&fd);
> -		goto out_put_root;
> -	}
> -	if (!hfs_brec_read(&fd, &entry, sizeof(entry))) {
> -		hfs_find_exit(&fd);
> +	err = hfsplus_get_hidden_dir_entry(sb, &str, &entry);
> +	if (err) {
> +		if (err != -ENOENT)
> +			goto out_put_root;

The hfs_brec_read() can return multiple errors (for example, -EINVAL). Are you
sure that this check is correct?

Thanks,
Slava.

> +	} else {
>  		if (entry.type != cpu_to_be16(HFSPLUS_FOLDER)) {
>  			err = -EIO;
>  			goto out_put_root;
> @@ -585,8 +601,7 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc)
>  			goto out_put_root;
>  		}
>  		sbi->hidden_dir = inode;
> -	} else
> -		hfs_find_exit(&fd);
> +	}
>  
>  	if (!sb_rdonly(sb)) {
>  		/*