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

[Viacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2026-03-19 at 22:49 +0800, Zilin Guan wrote:
> 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?
> 

I think we can return -ENOENT if hfs_brec_read() fails and continue as
hidden directory is missing. Let's follow to the current logic for now.

Thanks,
Slava.