Messages in this thread | | | Date | Thu, 7 Jul 2022 21:21:52 +0000 | From | Carlos Llamas <> | Subject | Re: [PATCH] binder: fix redefinition of seq_file attributes |
| |
On Mon, Jul 04, 2022 at 06:13:40PM +0200, Greg Kroah-Hartman wrote: > On Fri, Jul 01, 2022 at 06:20:41PM +0000, Carlos Llamas wrote: > > + binder_for_each_debugfs_entry(db_entry) { > > + dentry = binderfs_create_file(binder_logs_root_dir, > > + db_entry->name, > > + db_entry->fops, > > + db_entry->data); > > + if (IS_ERR(dentry)) { > > + ret = PTR_ERR(dentry); > > + goto out; > > + } > > I know this is a copy of what is there already, but there is never a > need to check the result of a debugfs_create_* call. Just call it and > move on, never "abort" based on the result of a debugfs call, that's not > a good idea.
This is true, none of these debugfs files seem critical for mounting a binderfs instance. I'm thinking init_binder_logs() should just return void. I'm only a bit hesitant to completely ignore the return code as users specifically ask for these files to be created via mount option "stats". So probably a pr_warn is what is actually needed here.
> > So can you change this here, or want to send a follow-on patch that > removes these checks?
Sure, I'll send a follow-on patch. I'm currently AFK so setting ETA for next week until I can actually test this change.
> > > } > > > > proc_log_dir = binderfs_create_dir(binder_logs_root_dir, "proc"); > > Also there's never a need to save a directory, you can always look it up > when you want to remove it.
It seems this is a convenient way to share this path with binder which otherwise doesn't know where binderfs was mounted. From having a quick look it doesn't seem that we need to share all the details in struct binderfs_info though. Maybe there is a better way to handle all this.
Christian, since this is binderfs area WDYT?
-- Carlos Llamas
| |