On Mon, 2010-04-19 at 19:22 +0200, Jan Kiszka wrote:
> Philippe Gerum wrote:
> > On Mon, 2010-04-19 at 18:25 +0200, Philippe Gerum wrote:
> >> On Mon, 2010-04-19 at 18:14 +0200, Jan Kiszka wrote:
> >>> 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.
> >> Flexibility and robustness have to be combined. Explicit declaration in
> >> Kconfig is against flexibility, because this is one external thing more
> >> to take care of, for adding something as simple as a debug switch. So if
> 
> You already have to take care of Kconfig if you want a debug switch.
> 

Sure, but I don't find useful to clutter it even more to get it working.

> Jan
> 


-- 
Philippe.



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

Reply via email to