Philippe Gerum wrote:
> On Mon, 2010-04-19 at 17:58 +0200, Jan Kiszka wrote:
>> Gilles Chanteperdrix wrote:
>>> Jan Kiszka wrote:
>>>> Gilles Chanteperdrix wrote:
>>>>> Jan Kiszka wrote:
>>>>>> Gilles Chanteperdrix wrote:
>>>>>>> Jan Kiszka wrote:
>>>>>>>> Gilles Chanteperdrix wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> I found some code which was referencing directly some
>>>>>>>>> CONFIG_XENO_OPT_DEBUG_ variables with things like:
>>>>>>>>>
>>>>>>>>> #ifdef CONFIG_XENO_OPT_DEBUG_FOO
>>>>>>>>>
>>>>>>>>> This usage is incompatible with the pre-requisites of the assert.h
>>>>>>>>> header that CONFIG_XENO_OPT_DEBUG_FOO should be defined at all times.
>>>>>>>>> While grepping for CONFIG_XENO_OPT_DEBUG_, I found that we also have
>>>>>>>>> many duplicates of construction like:
>>>>>>>>> #ifndef CONFIG_XENO_OPT_DEBUG_FOO
>>>>>>>>> #define CONFIG_XENO_OPT_DEBUG_FOO 0
>>>>>>>>> #endif /* CONFIG_XENO_OPT_DEBUG_FOO */
>>>>>>>>>
>>>>>>>>> So, a patch follows which:
>>>>>>>>> - replace the #ifdef with some #if XENO_DEBUG(FOO)
>>>>>>>> Should probably come as a separate patch.
>>>>>>> Come on, the patch is simple, one patch for this is enough.
>>>>>> One part is an obvious fix, the other a refactoring under discussion.
>>>>>>
>>>>>>>>> - move all the initializations to assert.h
>>>>>>>>>
>>>>>>>>> This will make any reference to CONFIG_XENO_OPT_DEBUG_FOO outside of
>>>>>>>>> assert.h suspicious, and easy to detect.
>>>>>>>> How many duplicates did you find?
>>>>>>> A lot, especially for CONFIG_XENO_OPT_DEBUG_NUCLEUS
>>>>>> That's because the CONFIG_XENO_OPT_DEBUG_NUCLEUS defines were misplaced.
>>>>>> Besides that we had one CONFIG_XENO_OPT_DEBUG_QUEUE duplicate, but tons
>>>>>> of proper users. I don't see such an immediate need.
>>>>>>
>>>>>> When adding this kind of switching, it's till more handy to have things
>>>>>> locally in your subsystem. That also makes reviewing easier when you
>>>>>> only find changes in files that belong to a subsystem.
>>>>> That is not the main point, the main point is that putting all these
>>>>> defines in one files allow to detect easily direct references to the
>>>>> symbols.
>>>>>
>>>>>>>> Generally, I'm more a fan of decentralized management here (e.g. this
>>>>>>>> avoids needless patch conflicts in central files).
>>>>>>> If we maintain the list in alphabetical order (which I have done), we 
>>>>>>> reduce the likeliness for conflicts. The aim of doing this is also that 
>>>>>>> I can check that the sources are clean with:
>>>>>>>
>>>>>>> find xenomai-2.5 ! -name 'assert.h' -name '*.[ch]' | xargs grep 
>>>>>>> CONFIG_XENO_OPT_DEBUG_
>>>>>>>
>>>>>>> And that I can add this test to the automated build test.
>>>>>>>
>>>>>>> Note that forgetting to add a #define to the list yields an immediate
>>>>>>> compilation error. So, the patch makes things completely safe.
>>>>>> What did you change to make this happen (for new users of XENO_DEBUG)?
>>>>> Nothing. If you forget to add the define, and do not enable the debug
>>>>> option (which everybody does most of the time), you get a compilation
>>>>> error.
>>>> The alternative (and decentralized) approach for fixing this consists of
>>>> Kconfig magic for generating the value:
>>>>
>>>> config XENO_OPT_DEBUG_FOO
>>>>    bool "..."
>>>>
>>>> config XENO_OPT_DEBUG_FOO_P
>>>>    int
>>>>    default "1" if XENO_OPT_DEBUG_FOO
>>>>    default "0"
>>>>
>>>> and XENO_DEBUG() could be extended to test for
>>>> CONFIG_XENO_OPT_DEBUG_FOO_P when given "FOO". I'm just not sure if this
>>>> can be expressed for legacy 2.4 kernels, so it might have to wait for
>>>> Xenomai 3.
> 
> Well, actually, I would not merge this in Xenomai 3. I find this rather
> overkill; mainline first I mean, and mainline, i.e. the Xenomai code
> base only requires a simple and straightforward way to get debug
> switches right. Having to make Kconfig a kitchen sink for some unknown
> out of tree modules to be happy is not really my preferred approach in
> this particular case.
> 
> Don't get me wrong, I'm not opposed to a more decentralized approach on
> the paper, it's just that I only care about the mainline tree here.

The point is not out-of-tree but robustness. Neither the current
decentralized #ifdef-#define nor its centralized brother meet this
criteria. An approach like the above which forces you to provide all
required bits before any of the cases (disabled/enabled) starts to work
does so.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

_______________________________________________
Xenomai-core mailing list
Xenomai-core@gna.org
https://mail.gna.org/listinfo/xenomai-core

Reply via email to