Re: [PATCH v2 16/17] UBI: hide EBA internals

[Boris Brezillon]

On Fri, 16 Sep 2016 12:43:48 +0200
Richard Weinberger <richard@xxxxxx> wrote:

> Boris,
> 
> On 05.09.2016 17:05, Boris Brezillon wrote:
> > Create a private ubi_eba_table struct to hide EBA internals and provide
> > helpers to allocate, destroy, copy and assing an EBA table to a volume.
> > 
> > Now that external EBA users are using helpers to query/modify the EBA
> > state we can safely change the internal representation, which will be
> > needed to support the LEB consolidation concept.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/mtd/ubi/build.c |   2 +-
> >  drivers/mtd/ubi/eba.c   | 166 ++++++++++++++++++++++++++++++++++++++++--------
> >  drivers/mtd/ubi/ubi.h   |   8 ++-
> >  drivers/mtd/ubi/vmt.c   |  40 +++++-------
> >  4 files changed, 165 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> > index 0680516bb472..45ea1ddebc5c 100644
> > --- a/drivers/mtd/ubi/build.c
> > +++ b/drivers/mtd/ubi/build.c
> > @@ -574,7 +574,7 @@ void ubi_free_internal_volumes(struct ubi_device *ubi)
> >  
> >  	for (i = ubi->vtbl_slots;
> >  	     i < ubi->vtbl_slots + UBI_INT_VOL_COUNT; i++) {
> > -		kfree(ubi->volumes[i]->eba_tbl);
> > +		ubi_eba_set_table(ubi->volumes[i], NULL);  
> 
> Why not ubi_eba_destroy_table()? We don't have to reset the pointer to NULL
> here.

ubi_eba_destroy_table() would work, but as said in the commit message,
I'm trying to hide EBA's internals from other part of the UBI
implementation, and thus, I'd like to avoid having external code
(everything that is outside of eba.c) reference/manipulate the
ubi->eba_tbl field directly.

ubi_eba_destroy_table() was only exported (made non-static) to let vmt
code free an EBA table if the resize operation fails in the middle
(between ubi_eba_create_table() and ubi_eba_set_table() calls).

> I'm also not really happy with the name ubi_eba_set_table() because it does
> more the setting the table. It destroys also the old one.

I can definitely rename the function. How about ubi_eba_replace_table().

> 
> What I'm trying to say is, when we bite the bullet and introduce lots of new wrapper
> functions to hide internals I want very clear and describing names for them.

I understand and I agree.
I thought ubi_eba_set_table() was accurately describing the function
purpose: assigning an EBA table to a volume. The fact that the old
table (if any) is freed when the new one is assigned is just an
internal detail, and that should not impact the user behavior.
But I'm perfectly fine renaming this function.

> I agree that this is a matter of taste but I had a few "Oh this looks wrong" moments
> while reviewing this patch just because the naming confused me. After looking
> up the code behind the wrapper it was clear.
> 
> Thanks,
> //richard