On Tue, 11 Aug 2026 10:28:18 +0900 Masami Hiramatsu (Google) <[email protected]> wrote:
> Hi Steve, > > I have some comments on this. > > On Mon, 10 Aug 2026 16:07:08 -0400 > Steven Rostedt <[email protected]> wrote: > > > @@ -551,20 +643,40 @@ static struct dentry *eventfs_root_lookup(struct > > inode *dir, > > if (strcmp(name, entry->name) != 0) > > continue; > > > > + if (ro && !entry->read_only) > > + return NULL; > > + > > data = ei->data; > > if (entry->callback(name, &mode, &data, &fops) <= 0) > > return NULL; > > > > + if (ro) > > + mode |= 0444; > > Don't we need to clear writable bits? e.g. > > mode = (mode & ~0222) | 0444; Sure. > > [...] > > @@ -812,6 +937,52 @@ struct eventfs_inode *eventfs_create_events_dir(const > > char *name, struct dentry > > return ERR_PTR(-ENOMEM); > > } > > > > +/** > > + * eventfs_create_events_dir_ro - create a read-only events directory > > + * @name: The name of the top level directory to create. > > + * @entries: A list of entries that represent the files under this > > directory > > + * @size: The number of @entries > > + * @data: The default data to pass to the files (an entry may override > > it). > > This document need to be updated too. It should be > "eventfs_create_events_ro_copy" > and only takes @name and @ei. Bah, I thought I fixed that. I may have but lost the changes in a rebase. > > > + * > > + * This function configures the eventfs filesystem root as a read-only > > + * trace event directory using the existing eventfs_inode lazy-lookup > > + * infrastructure. > > + * > > + * See eventfs_create_dir() for use of @entries. > > + */ > > +int eventfs_create_events_ro_copy(const char *name, struct eventfs_inode > > *ei) > > +{ > > + static struct dentry *dentry; > > + struct tracefs_inode *ti; > > + struct inode *inode; > > + > > + /* Can only be called once. */ > > + if (dentry) > > + return -EBUSY; > > + > > + /* Reference acquired but never freed */ > > + dentry = eventfs_ro_get_root(); > > + if (IS_ERR(dentry)) > > + return PTR_ERR(dentry); > > + > > + inode = d_inode(dentry); > > + > > + INIT_LIST_HEAD(&ei->children); > > + INIT_LIST_HEAD(&ei->list); > > Nit: This seems redundant because those lists are initialized in > event_create_events_dir() already, and here we initialize again. > Currently, there is no chance to add anything on these lists. > (But if we add something on these lists, re-initializing will > make the items orphaned silently.) Nice catch. I'll fix it. I also found some other issues with the superblock setup. This isn't going to go into the next merge window as it's too late. Thanks for looking at it, -- Steve > > > + > > + ti = get_tracefs(inode); > > + ti->flags |= TRACEFS_EVENT_INODE; > > + ti->private = ei; > > + > > + inode->i_op = &eventfs_ro_dir_inode_operations; > > + inode->i_fop = &eventfs_ro_file_operations; > > + > > + /* This is never freed */ > > + dentry->d_fsdata = get_ei(ei); > > + > > + return 0; > > +} > > Thanks, >
