Re: [PATCH] UBI: dereference after kfree in create_vtbl

["Satyam Sharma" <satyam.sharma@gmail.com>]

On 5/5/07, Artem Bityutskiy <dedekind@infradead.org> wrote:
> On Sat, 2007-05-05 at 19:18 +0530, Satyam Sharma wrote:
> > Well, you're developing / maintaining this right now, so it's your
> > call. Though I bet most people would find keeping that list_add_tail
> > local to scan.c more tasteful.
>
> I do not think so. If you are interested, try to find "UBI take 2"
> patches in lkml. Look how it looked liked. It consisted of many
> independent units and units could access other units _only_ via
> interfaces. I would do what you say there.

But what you're doing now is *worse*, don't you see it? You _think_
that you are not exporting any add_to_list function from scan.c
because you removed the wrapper from scan.h, but then you open-code it
anyway in create_vtbl (yes, that list_add _is_ the *only* meaningful
code in add_to_list; it just so _happens_ that most of its callers
till now also wanted to allocate a ubi_scan_leb too at that time so
you pushed that too into add_to_list rather than do it in the callers
-- via a separate alloc_leb, for example). Now that means there is an
*undocumented* use of the "add to list" _functionality_ outside of
scan.c anyway and which wouldn't even come up if someone tries to
analyse / overhaul the code some day in the future. I know it's just
_one_ exception, but still, I'm a heckler for style.

> Read Teo's comments - I actually now agree with them. And after I had
> changed UBI i got rid of several thousands lines of code, and the code
> became simpler.
>
> So, my argument is:
> 1. It makes no sense to introduce one more non-static function to _just_
> encapsulate list_add_tail and _just_ for one caller.

Well, introducing one more non-static function is *not* what I
originally had in mind (it was proposed only as a temporary measure
till all users of ubi_scan_add_to_list could be updated to use the
other version, explained below).

I actually planned to _replace_ ubi_scan_add_to_list with another
version that does *not* allocate memory (the __ubi_scan_add_to_list I
proposed above), and leave allocation up to its *callers* who
therefore pass a valid ubi_scan_leb to it. But that is where I ran
into ubi_scan_add_used().

> 2. It is _C_, it is _kernel_, and it is OK sometimes _not_ to follow
> computer since rules.

Ah, well, forget it. It's all a matter of taste and maintainability. I
guess in the end it needs to be fine with you.