seperate mallocs

Philip Pokorny ppokorny at
Sat Apr 3 23:56:38 CEST 2004

Jean Delvare wrote:

>>>After taking a closer look at the code it seemed obvious that this
>>>case just couldn't happen. The negative "kind" case is handled by the
>>>"if(kind <= 0)" block right above, and all the other values are
>>>handled in the swicth/case block. So why would we try to catch an
>>>error that, by construction, cannot happen?
>>Ahh, but that's just it.  "all" the other values are *not* handled in 
>>the switch/case block.  The variable "kind" is passed in as an
>>argument to the function.  As such, we have *no* control over the
>>values it takes.  It is set by the i2c core code that parses the
>>force_foo= arguments and sets "kind" as appropriate.  But if we got a
>>version mismatch between part of the core and the driver, then perhaps
>>kind could be set to a value we don't expect.
>I don't get you. The various possible force parameters are defined by
>the driver itself, through the line SENSORS_INSMOD_<N> call. There is no
>chip-specific code in the core, so there can't be any version mismatch
>as you suggest. The SENSORS_INSMOD_<N> call expands to express the
>static struct i2c_force_data forces[], which in turns is pointed to by
>static struct i2c_address_data addr_data. This is where i2c-core looks
>at for how to convert force parameters to the driver's detect function.
>Still the definition is part of the driver, so there is no version
>mismatch possible.
The code that implements the force_ parsing and detection isn't in the 
driver.  It's not even in lm_sensors.  It's in i2c (i2c_detect.c and 
i2c-proc.h)  It parses data structures that are part of the module.  So 
I would suggest that there *is* some potential for version mis-match 
between the i2c code and the driver.

Further the way that the force_xxx functionality is implemented is 
almost completely opaque to the driver developer.  I only just now dug 
through the twisty little maze of passages that implement this.  Perhaps 
since you've been working with the entire code base, you're more 
familiar with it's workings.  I stand by my assertion that I should 
defend against bad values because I don't have easy visibility to where 
those values come from.

>What would possibly cause trouble is if you later add a new kind of chip
>supported by an older driver, and incidentally forget to populate the
>switch/case statement for this kind. In this case, I agree that the
>default case, which I removed, would have triggered. But I estimate that
>the correct solution is not to forget. If we start assuming that we
>might have done something wrong at any point in the driver, we will end
>up handling possible errors everywhere and the code will be plain
>unreadable and unmaintainable.
Let's not go overboard.  I'm not suggesting we test every variable all 
the time.  The xxx_detect() function is basically the main entrypoint 
for the entire driver.  It only gets called during driver initialization 
and there is very little overhead for a default clause in a switch 
statement.  This is an appropriate place to verify that all arguments 
are consistent and within bounds.

>Note that in most cases, the only thing that will happened in the faulty
>case is that the client name would be an empty string, and, in some
>cases, that the extra features of this specific chip would be disabled.
>Nothing serious. I admit that it's a bit more dangerous for your drivers
>because of the complex feature table merging mechanism you use. Not that
>I criticize it, it's actually nice and saves code. But it's unusual.
>>So better to detect that and bail out, rather than blindly carry on 
>>without a "valid" chip type.  The rest of the code is going to make 
>>decisions based on the 'data->type' field to which kind is eventually 
>>copied, so the behavior of the driver would become "undefined" with
>>an erroneous value in that variable.
>This mostly applies to your driver only. BTW, I have been doing similar
>changes (dropping default case because it couldn't happend) several
>times these last weeks, mostly while porting new drivers.
>>Defensive programming is never a bad idea.  'default' clauses in a 
>>switch are almost always a good idea.
>In most cases I would agree. But when it comes to handling errors that
>cannot happen (if that's what it is) in kernel code, I don't. Kernel
>code is supposed to be kept as simple as possible. Increasing the
>driver's size with no benefit and possibly confuse the reader is no
>If you can really think of a possible scenario which would trigger the
>default case, I'm listening.
To take this to the extreme, you seem to be saying that because this is 
the kernel, it's better to be fast and small than safe.


I'm not convinced by your argument, and I don't think I'm going to 
change your mind, so I propose a comprimise.

Why not enclose the default: block with #ifdef DEBUG/#endif.  That way 
during driver development, we have the extra check and during "normal" 
runtime you don't have the overhead?


More information about the lm-sensors mailing list