On Tue, 19 Oct 2021 16:44:20 +0300
Nikita Yushchenko <nikita.yushche...@virtuozzo.com> wrote:
> >> - bm_data = kzalloc(sizeof(struct binfmt_misc), GFP_KERNEL);
> >> - if (!bm_data)
> >> - return -ENOMEM;
> >> + INIT_LIST_HEAD(&bm_data->entries);
> >> + rwlock_init(&bm_data->entries_lock);
> >>
> >> - INIT_LIST_HEAD(&bm_data->entries);
> >> - rwlock_init(&bm_data->entries_lock);
> >> + ve->binfmt_misc = bm_data;
> >
> > Isn't it better to move ve->binfmt_misc assignment to the
> > end of function where we know that all operations was successful?
>
> Since ve->binfmt_misc is global, not per-mount, the logic is simpler if
> creation of bm_data is made
> independent from mount success/failure. I.e. regardless of success of mount
> operation, bm_data is
> created at the first mount and saved in ve->binfmt_misc. Then, it will be
> cleaned at ve destruction time.
I agree.
>
> >
> >> + /* this will be cleared by ve_binfmt_fini() */
> >> + }
> >>
> >> err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> >> - if (err) {
> >> - kfree(bm_data);
> >
> > If we have ve->binfmt_misc assignment in the upper part of code, then
> > we need to do ve->binfmt_misc = NULL here.
>
> This will mishandle case when ve->binfmt_misc was initialized at previous
> mount.
>
> >
> >> + if (err)
> >> return err;
> >> - }
> >>
> >> sb->s_op = &s_ops;
> >> -
> >> - ve->binfmt_misc = bm_data;
> > see above
> >
> >> bm_data->enabled = 1;
> >>
> >> return 0;
> >> @@ -971,6 +958,8 @@ static void ve_binfmt_fini(void *data)
> >> while (!list_empty(&bm_data->entries))
> >> kill_node(bm_data, list_first_entry(
> >> &bm_data->entries, Node, list));
> >> +
> >> + kfree(bm_data);
> >
> > We have kfree in ve_destroy (kernel/ve/ve.c) already.
>
> Indeed.
>
> But, why splitting destruction into two parts?
> Why not doing both at the same location.
no problem, but then we have to remove kfree from kernel/ve/ve.c or
we get double kfree() here.
>
> Nikita
Alex
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel
