Messages in this thread | | | From | Carlos Llamas <> | Date | Fri, 9 Jul 2021 10:32:14 -0600 | Subject | Re: [PATCH] ANDROID: binderfs: add capabilities support |
| |
On Fri, Jul 9, 2021 at 2:56 AM Christian Brauner <christian.brauner@ubuntu.com> wrote: > > On Wed, Jul 07, 2021 at 04:24:19PM +0000, Carlos Llamas wrote: > > Provide userspace with a mechanism to discover binder driver > > capabilities to refrain from using these unsupported features > > Hey Carlos, > > The model will be one file per feature?
Yes. I dropped a previous single bitmask file idea per Greg's suggestion. The file per feature improves on a number of areas such as feature count limit, readability and it's easier to manage (add/remove features).
> > Instead of calling the directory "caps" should this maybe be called > "features"? I'm not fuzzed about it and if you want to keep "caps" > that's fine. The term is just a bit overused and makes me think of other > things than this.
I have no problems switching over to "features".
> > > in the first place. Note that older capabilities are assumed > > to be supported and only new ones will be added. > > What if you ever want to deprecate one? :)
If the file for a feature doesn't exist then such feature is not supported. So we can avoid creating such file if a feature were to be deprecated.
> > > > > Signed-off-by: Carlos Llamas <cmllamas@google.com> > > --- > > drivers/android/binderfs.c | 45 ++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 45 insertions(+) > > > > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c > > index e80ba93c62a9..f793887f6dc8 100644 > > --- a/drivers/android/binderfs.c > > +++ b/drivers/android/binderfs.c > > @@ -58,6 +58,10 @@ enum binderfs_stats_mode { > > binderfs_stats_mode_global, > > }; > > > > +struct binder_capabilities { > > + bool oneway_spam; > > +}; > > + > > static const struct constant_table binderfs_param_stats[] = { > > { "global", binderfs_stats_mode_global }, > > {} > > @@ -69,6 +73,10 @@ static const struct fs_parameter_spec binderfs_fs_parameters[] = { > > {} > > }; > > > > +static struct binder_capabilities binder_caps = { > > + .oneway_spam = true, > > I know this is the oneway spam _detection_ feature but this file makes > it sound like the binder driver has the capability to generate one-way > spam. :) Maybe name at least name the file "oneway_spam_detection".
That's true. I'll rename it as suggested.
> > > +}; > > + > > static inline struct binderfs_info *BINDERFS_SB(const struct super_block *sb) > > { > > return sb->s_fs_info; > > @@ -583,6 +591,39 @@ static struct dentry *binderfs_create_dir(struct dentry *parent, > > return dentry; > > } > > > > +static int binder_caps_show(struct seq_file *m, void *unused) > > +{ > > + bool *cap = m->private; > > + > > + seq_printf(m, "%d\n", *cap); > > + > > + return 0; > > +} > > +DEFINE_SHOW_ATTRIBUTE(binder_caps); > > + > > +static int init_binder_caps(struct super_block *sb) > > You can drop the goto here and just always return directly.
I also noticed this and I decided to keep it consistent with init_binder_logs() structure. But I don't have a strong preference so I'll switch to early returns.
> > > +{ > > + struct dentry *dentry, *root; > > Please name this "dir" instead of "root". "root" is conventionally used > for sb->s_root and especially here in this file I only ever used it to > indicate s_root.
ok, sounds good.
> > > + int ret = 0; > > + > > + root = binderfs_create_dir(sb->s_root, "caps"); > > + if (IS_ERR(root)) { > > + ret = PTR_ERR(root); > > return PTR_ERR(root); > > > + goto out; > > + } > > + > > + dentry = binderfs_create_file(root, "oneway_spam", > > + &binder_caps_fops, > > + &binder_caps.oneway_spam); > > + if (IS_ERR(dentry)) { > > + ret = PTR_ERR(dentry); > > return PTR_ERR(root); > > > + goto out; > > + } > > + > > +out: > > + return ret; > > +} > > + > > static int init_binder_logs(struct super_block *sb) > > { > > struct dentry *binder_logs_root_dir, *dentry, *proc_log_dir; > > @@ -723,6 +764,10 @@ static int binderfs_fill_super(struct super_block *sb, struct fs_context *fc) > > name++; > > } > > > > + ret = init_binder_caps(sb); > > + if (ret) > > + return ret; > > + > > if (info->mount_opts.stats_mode == binderfs_stats_mode_global) > > return init_binder_logs(sb); > > > > -- > > 2.32.0.93.g670b81a890-goog > >
thanks, carlos llamas
| |