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

[Zilin Guan]

On Wed, Mar 18, 2026 at 10:33:47PM +0000, Viacheslav Dubeyko wrote:
> On Wed, 2026-03-18 at 23:00 +0800, Zilin Guan wrote:
> > +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?

Right, I'll update this in v3.

> > +		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.

Agreed.

> > +		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.

I see your point.

The current logic follows hfsplus_lookup(), where only -ENOENT is treated
as missing, and other errors are propagated. The original code effectively 
ignored hfs_brec_read() errors and continued as if the directory was missing. 
For critical errors like -EIO/-EINVAL/-ENOMEM, failing the mount seems safer.

If maintaining the legacy behavior is preferred, I can map all read errors 
to -ENOENT inside the helper instead:

	err = hfs_brec_read(&fd, entry, sizeof(*entry));
	if (err)
		err = -ENOENT;

Would you prefer to keep the legacy behavior, or is propagating the exact 
error acceptable?

Thanks,
Zilin