Re: [PATCH] include/linux/slab.h: new KFREE() macro.

[Vadim Lobanov]

On Sun, 2007-01-07 at 16:02 -0800, Amit Choudhary wrote:
> That's where KFREE(ptr) comes in so that the code doesn't look ugly and still the purpose is
> achieved.

Shoving it into a macro makes it no better.

> > Reading code like that makes me say "wtf?", simply because 'ptr' is not
> > used thereafter,
> 
> Really? Then why do we have all the debugging options to catch re-use of the memory that has been
> freed. So many debugging options has been implemented, so much effort has gone into them, partly
> because programmers sometimes miss correct programming.

Which is exactly why we should continue to let programmers focus on
trying to get their code correct and letting the existing safety tools
catch thinkos, instead of distracting them with confusing and completely
pointless variable assignments.

> I do not know what you are talking about here. You are saying that a function does not need three
> different arrays with different names. How can you say that? How do you know what is the
> requirement?

What I said was that your example proves something entirely different
than what you think: rather than proving the need for your KFREE()
macro, it instead proves the need to design the code correctly from the
start, so as to avoid even thinking about this crud.

If you insist, here's your example function, trivially rewritten without
any NULL assignments. (I used two arrays, not three, to save space. The
basic idea works by-design for any random number of arrays, each of any
arbitrary size.)

struct type1 {
	/* something */
};

struct type2 {
	/* something */
};

#define COUNT 10

void function1(struct type1 **a1, struct type2 **a2, unsigned int sz);

void function2(void)
{
	struct type1 *arr1[COUNT];
	struct type2 *arr2[COUNT];
	int i;

	/* init */
	for (i = 0; i < COUNT; i++) {
		arr1[i] = kmalloc(sizeof(struct type1), ...);
		if (!arr1[i])
			goto free_1;
	}
	for (i = 0; i < COUNT; i++) {
		arr2[i] = kmalloc(sizeof(struct type2), ...);
		if (!arr2[i])
			goto free_2;
	}

	/* work */
	function1(arr1, arr2, COUNT);

	/* fini */
	i = COUNT;
free_2:
	for (i--; i >= 0; i--) {
		kfree(arr2[i]);
	}
	i = COUNT;
free_1:
	for (i--; i >= 0; i--) {
		kfree(arr1[i]);
	}
}

In most cases, though, the above code design would be brain-damaged from
the start: unless the sizes involved are prohibitively large, the
function should be allocating all the memory in a single pass.

So, where's the demonstrated need for KFREE()?

-- Vadim Lobanov


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/