Re: [PATCH 3/4] configfs: implement committable items

Bartosz Golaszewski
2021-01-14T14:20:13Z

On Thu, Jan 14, 2021 at 9:40 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Thu, Jan 14, 2021 at 12:46 AM Joel Becker <jlbec@evilplan.org> wrote:
> >
> > On Wed, Nov 25, 2020 at 04:22:46PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > This implements configfs committable items. We mostly follow the
> > > documentation except that we extend config_group_ops with uncommit_item()
> > > callback for reverting the changes made by commit_item().
> >
> > Woohoo! A long time coming, but thank you for working on the
> > implementation!
> >
> > > Each committable group has two sub-directories: pending and live. New
> > > items can only be created in pending/. Attributes can only be modified
> > > while the item is in pending/. Once it's ready to be committed, it must
> > > be moved over to live/ using the rename() system call. This is when the
> > > commit_item() function will be called.
> >
> > The original API intended for live items to still be modifyable. The
> > live/ path forbids mkdir()/rmdir(), but it allows store(). Otherwise,
> > items can't be adjusted at all while in use, which is severely limiting.
> > Obviously the store() handler must not allow transitions from
> > valid-value->invalid-value, but the handler would reject invalid values
> > anyway, wouldn't it?
> >
> > > diff --git a/fs/configfs/file.c b/fs/configfs/file.c
> > > index 1f0270229d7b..a20e55fd05e8 100644
> > > --- a/fs/configfs/file.c
> > > +++ b/fs/configfs/file.c
> > > @@ -243,9 +243,17 @@ fill_write_buffer(struct configfs_buffer * buffer, const char __user * buf, size
> > > static int
> > > flush_write_buffer(struct file *file, struct configfs_buffer *buffer, size_t count)
> > > {
> > > + struct config_item *parent_item = buffer->item->ci_parent;
> > > struct configfs_fragment *frag = to_frag(file);
> > > + struct configfs_dirent *sd;
> > > int res = -ENOENT;
> > >
> > > + if (parent_item && parent_item->ci_dentry) {
> > > + sd = parent_item->ci_dentry->d_fsdata;
> > > + if (sd->s_type & CONFIGFS_GROUP_LIVE)
> > > + return -EPERM;
> > > + }
> > > +
> > > down_read(&frag->frag_sem);
> > > if (!frag->frag_dead)
> > > res = buffer->attr->store(buffer->item, buffer->page, count);
> >
> > Basically, I would just leave this hunk out.
> >
>
> I would make this configurable per-attribute because for the use-case
> I need this for we definitely don't want the items to be modifiable
> once they're "live".
>
> Bartosz

After a second thought: I agree this can be left to the user's
discretion so I'll just remove this and enforce it in the sample code.

One thing I'm unsure about is whether we should allow to change the
group's name when moving it from pending to live. Any thoughts?

Bartosz