Re: [RFC] ASLA design, depth of code review and lack thereof

[Takashi Iwai]

At Mon, 7 Jun 2004 15:18:06 +0100,
Russell King wrote:
> 
> On Mon, Jun 07, 2004 at 03:57:51PM +0200, Takashi Iwai wrote:
> > They're nice but they don't provide "cast checking", no?
> > The main purpose of the magic_* stuffs in ALSA is to check the cast of
> > the void pointer back to the original data type, which the compiler
> > can't check.
> > 
> > Maybe we can implement only this "magic" check separetly and get rid
> > of allocation checks, but I'm not sure whether it's worthy to do
> > that.
> 
> I must ask the question: why is ALSA such a special case that it
> needs this level of "magic" checking, when the rest of the kernel
> has happily used void pointers to carry driver specific data around
> for the last 10 years without serious incident or any major debugging
> problems?

Sure, it isn't always needed for the matured drivers.

The magic check is a (kind of) debugging stuff, as you can see even
in linux/spinlock.h.  The ugliness in ALSA's implementation is that it
isn't transparent but forces to use some unusual macros.  From the
viewpoint of code readability, I'm willing to get rid of them, too.
But this indeed can help sometimes to catch silly bugs, especially in
the initial development phase.  That's the reason I hesitate to remove
it completely.


> As long as objects are cleanly defined, and it is obvious what these
> private driver specific pointers are for, then this "magic" become
> unnecessary.  Take a look at the driver model for instance.
> 
> For instance, dev_set_drvdata() and dev_get_drvdata() provide access
> to a clearly defined void pointer for drivers to use.  It is clear
> that a device driver uses it to place its private data structure
> there, and it is the only code which should be accessing that.

That's a good point.  Having the interface for accessing such a
private data is better.

However, the obtained data is still void pointer.  Suppose that you
have a driver spreaded over several source files and you change the
data type of this private data.  A careless programmer like me might
forget to change a part in another file.  The driver still compiles
and runs fine unless it hits casually the unchanged part and results
in memory corruption.

That's a silly scenario, but I believe the dynamic cast check is
usefull for catching such a bug easily.  That's why we see similar
magic checks here and there.  The question is how clean it's
represented.

I'm thinking of a simpler way, i.e. introducing macros something like:

#ifdef DEBUG
#define MAGIC_NUMBER	unsigned int __magic_number;
#define SET_MAGIC(rec,magic)	((rec)->__magic_number = (magic))
#define CHECK_MAGIC(rec,magic,action)	do { \
	if ((rec) == NULL ||  (rec)->__magic_number != (magic)) {\
		SOME_ERROR(); action; \
	} } while (0)
#else
#define MAGIC_NUMBER
#define SET_MAGIC(rec,magic)
#define CHEC_MAGIC(rec,magic,action)
#endif

struct foo {
	...
	MAGIC_NUMBER;
};

#define MY_MAGIC	0xabcd1234

static void allocate()
{
	struct foo *data = kmalloc(sizeof(*data));
	SET_MAGIC(data, MY_MAGIC);
	...
}

static int handler(void *private_data)
{
	struct foo *data = private_data;
	CHECK_MAGIC(data, MY_MAGIC, return -EINVAL);
	...
}

In this case, we can simply remove *_MAGIC from the final version (if
wanted), while the current ALSA magic implementation can't be removed
simply.


> I guess though that the problem area for ALSA is the way the snd_pcm_t
> private_data member magically appears in the snd_pcm_substream_t
> private_data member behind the drivers back, so it's unclear who
> actually owns the data in the private_data members.

Well, this is not the case.  Usually a driver uses only one of them.


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