seperate mallocs

[Philip Pokorny]

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
>good.
>
>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?

:v)