On Thu, 4 Jan 2024 01:59:10 +0000
Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Jan 03, 2024 at 08:32:46PM -0500, Steven Rostedt wrote:
>
> > +static struct inode *instance_inode(struct dentry *parent, struct inode *inode)
> > +{
> > + struct tracefs_inode *ti;
> > + struct inode *root_inode;
> > +
> > + root_inode = d_inode(inode->i_sb->s_root);
> > +
> > + /* If parent is NULL then use root inode */
> > + if (!parent)
> > + return root_inode;
> > +
> > + /* Find the inode that is flagged as an instance or the root inode */
> > + do {
> > + inode = d_inode(parent);
> > + if (inode == root_inode)
> > + return root_inode;
> > +
> > + ti = get_tracefs(inode);
> > +
> > + if (ti->flags & TRACEFS_INSTANCE_INODE)
> > + return inode;
> > + } while ((parent = parent->d_parent));
>
> *blink*
>
> This is equivalent to
> ...
> parent = parent->d_parent;
> } while (true);
Yeah, that loop went through a few iterations as I first thought that root
was a tracefs_inode and the get_tracefs() would work on it. No, it was not,
and it caused a cash. But I didn't rewrite the loop well after fixing that.
I was also not sure if parent could be NULL, and wanted to protect against it.
>
> ->d_parent is *never* NULL. And what the hell is that loop supposed to do,
> anyway? Find the nearest ancestor tagged with TRACEFS_INSTANCE_INODE?
>
> If root is not marked that way, I would suggest
> if (!parent)
> parent = inode->i_sb->s_root;
> while (!IS_ROOT(parent)) {
> struct tracefs_inode *ti = get_tracefs(parent->d_inode);
> if (ti->flags & TRACEFS_INSTANCE_INODE)
> break;
> parent = parent->d_parent;
> }
> return parent->d_inode;
Sure, I could rewrite it that way too.
Thanks,
-- Steve
