Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1484866 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 16/17] UBI: hide EBA internals Richard Weinberger <richard@nod.at> - 2016-09-16 12:50 +0200
    Re: [PATCH v2 16/17] UBI: hide EBA internals Boris Brezillon <boris.brezillon@free-electrons.com> - 2016-09-16 13:40 +0200
      Re: [PATCH v2 16/17] UBI: hide EBA internals Richard Weinberger <richard@nod.at> - 2016-09-16 15:30 +0200

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

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@free-electrons.com>
> ---
>  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.
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.

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 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

[toc] | [next] | [standalone]

#1484910

On Fri, 16 Sep 2016 12:43:48 +0200
Richard Weinberger <richard@nod.at> 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@free-electrons.com>
> > ---
> >  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

[toc] | [prev] | [next] | [standalone]

#1484988

Boris,

On 16.09.2016 13:38, Boris Brezillon wrote:
> 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().

Reads much better (to me). :-)

>>
>> 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.

Thanks,
//richard

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web