From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Richard Weinberger <richard@nod.at>
Cc: Artem Bityutskiy <dedekind1@gmail.com>,
David Woodhouse <dwmw2@infradead.org>,
Brian Norris <computersforpeace@gmail.com>,
linux-mtd@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 16/17] UBI: hide EBA internals
Date: Fri, 16 Sep 2016 13:38:08 +0200 [thread overview]
Message-ID: <20160916133808.1dd5db40@bbrezillon> (raw)
In-Reply-To: <646e09d9-d148-b21b-d411-75ebb05dd121@nod.at>
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
next prev parent reply other threads:[~2016-09-16 11:38 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-05 15:04 [PATCH v2 00/21] UBI: various rework/cleanup/fixes Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 01/17] UBI: fastmap: use ubi_find_volume() instead of open coding it Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 02/17] UBI: fix add_fastmap() to use the vid_hdr passed in argument Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 03/17] UBI: fastmap: avoid multiple be32_to_cpu() when unneccesary Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 04/17] UBI: fastmap: scrub PEB when bitflips are detected in a free PEB EC header Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 05/17] UBI: factorize code used to manipulate volumes at attach time Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 06/17] UBI: factorize destroy_av() and ubi_remove_av() code Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 07/17] UBI: fastmap: use ubi_rb_for_each_entry() in unmap_peb() Boris Brezillon
2016-09-05 15:04 ` [PATCH v2 08/17] UBI: fastmap: use ubi_io_{read, write}_data() instead of ubi_io_{read, write}() Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 09/17] UBI: provide helpers to allocate and free aeb elements Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 10/17] UBI: move the global ech and vidh variables into struct ubi_attach_info Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 11/17] UBI: simplify recover_peb() code Boris Brezillon
2016-09-16 10:14 ` Richard Weinberger
2016-09-16 11:23 ` Boris Brezillon
2016-09-16 13:23 ` Richard Weinberger
2016-09-16 13:27 ` Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 12/17] UBI: simplify LEB write and atomic LEB change code Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 13/17] UBI: add an helper to check lnum validity Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 14/17] UBI: provide an helper to check whether a LEB is mapped or not Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 15/17] UBI: provide an helper to query LEB information Boris Brezillon
2016-09-05 15:05 ` [PATCH v2 16/17] UBI: hide EBA internals Boris Brezillon
2016-09-16 10:43 ` Richard Weinberger
2016-09-16 11:38 ` Boris Brezillon [this message]
2016-09-16 13:24 ` Richard Weinberger
2016-09-05 15:05 ` [PATCH v2 17/17] UBI: introduce the VID buffer concept Boris Brezillon
2016-09-16 11:38 ` Richard Weinberger
2016-09-16 11:41 ` Boris Brezillon
2016-09-16 13:26 ` [PATCH v2 00/21] UBI: various rework/cleanup/fixes Richard Weinberger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160916133808.1dd5db40@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=computersforpeace@gmail.com \
--cc=dedekind1@gmail.com \
--cc=dwmw2@infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=richard@nod.at \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).