[Q] warning BUG() related fixing and janitors question

[Q] warning BUG() related fixing and janitors question

[Ezequiel García]

Hi Dan,

I would like to know better about the janitors list, and what kind of
patches are handled here.
I've found a number of warnings in my building and I'm not sure where to
send patches and
what git tree should I take, since the warnings appear in a wide number of
subsystems.

1.
For instance, in kernel/sched/core.c

static inline struct task_struct *
pick_next_task(struct rq *rq)
{
    /* snip */
    BUG(); /* the idle class will always have a runnable task */
}

You can see that after BUG there is no return statement, which will produce
a compilation
warning you disable BUG() and it gets defined to nothing.

There are several of these warnings here and there, I guess as a consequence
of BUG() macro.

---
2.
Another one is flat_set_persistent() which is only IMO properly defined in
arch/sh/. The current
definition produces a warning in other architectures if binary flat format
is used.

So, I guess I should send a patch to each arch-specific list, right? using
arch-specific git trees?
I would like to have just one git tree, and not ten :)

Thanks for your time,
Ezequiel.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Q] warning BUG() related fixing and janitors question

[Dan Carpenter]

On Mon, Apr 16, 2012 at 04:03:28PM -0300, Ezequiel García wrote:
> Hi Dan,
> 
> I would like to know better about the janitors list, and what kind of
> patches are handled here.

We don't actually handle any patches any more.  It is a place to
help newbies, or if we are working on similar code then we CC the
list so not everyone sends the same patch 7 times, and also it's
for review.  Send patches to the people from
./scripts/get_maintainer.pl and CC kernel-janitors if you want.

> I've found a number of warnings in my building and I'm not sure where to
> send patches and
> what git tree should I take, since the warnings appear in a wide number of
> subsystems.
> 
> 1.
> For instance, in kernel/sched/core.c
> 
> static inline struct task_struct *
> pick_next_task(struct rq *rq)
> {
>     /* snip */
>     BUG(); /* the idle class will always have a runnable task */
> }
> 
> You can see that after BUG there is no return statement, which will produce
> a compilation
> warning you disable BUG() and it gets defined to nothing.
> 
> There are several of these warnings here and there, I guess as a consequence
> of BUG() macro.
> 

Don't start messing in scheduler code if you have any doubts about
what you are doing.  My advise is don't send a patch for this
warning.  Most times people leave BUG() enabled so we don't really
care about the warning.

> ---
> 2.
> Another one is flat_set_persistent() which is only IMO properly defined in
> arch/sh/. The current
> definition produces a warning in other architectures if binary flat format
> is used.
> 
> So, I guess I should send a patch to each arch-specific list, right? using
> arch-specific git trees?
> I would like to have just one git tree, and not ten :)

I'm not sure what you mean here.  What is the warning?  It sounds
like something is wrong in a Kconfig file.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Q] warning BUG() related fixing and janitors question

[Ezequiel García]

Hi Dan,

2012/4/16 Dan Carpenter <dan.carpenter@oracle.com>:
>
> Don't start messing in scheduler code if you have any doubts about
> what you are doing.  My advise is don't send a patch for this
> warning.  Most times people leave BUG() enabled so we don't really
> care about the warning.

Okey. Perhaps I picked the wrong example, there are a whole bunch of warnings
related to the same BUG() stuff, here's a template example:

void *p;

switch (something) {
  case a:
    p = foo();
  case b:
    p = bar();
  default:
    BUG();
}

dosomething(p);

--

This will trigger warning "p could be used uninitialized" or something.
I know most of us just leave BUG on, so you say these warnings aren't important?

>> ---
>> 2.
>> Another one is flat_set_persistent() which is only IMO properly defined in
>> arch/sh/. The current
>> definition produces a warning in other architectures if binary flat format
>> is used.
>>
>> So, I guess I should send a patch to each arch-specific list, right? using
>> arch-specific git trees?
>> I would like to have just one git tree, and not ten :)
>
> I'm not sure what you mean here.  What is the warning?  It sounds
> like something is wrong in a Kconfig file.
>

I'll explain better. flat_set_persistent() is used by no-one, except
from blackfin arch
where it has something in it. So it gets defined to nothing in every
architecture, except
from blackfin.

Now, arch/sh/include/asm/flat.h defines it like:

#define    flag_set_persisten(relval,p)    { (void)p; 0; })

while the rest of them just defines it to zero:

#define    flag_set_persisten(relval,p)    0

This will cause a warning for unused variable p, in fs/binfmt_flat.c
and IMO the arch/sh is doing it right (just to avoid the warning).

I tested both definitions with bloat-o-metering and it showed no
extra code.

Do you see my point now?

Thanks for your help,
Ezequiel.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Q] warning BUG() related fixing and janitors question

[Marcin Ślusarz]

2012/4/16 Ezequiel García <elezegarcia@gmail.com>:
> I'll explain better. flat_set_persistent() is used by no-one, except
> from blackfin arch
> where it has something in it. So it gets defined to nothing in every
> architecture, except
> from blackfin.
>
> Now, arch/sh/include/asm/flat.h defines it like:
>
> #define    flag_set_persisten(relval,p)    { (void)p; 0; })
>
> while the rest of them just defines it to zero:
>
> #define    flag_set_persisten(relval,p)    0
>
> This will cause a warning for unused variable p, in fs/binfmt_flat.c
> and IMO the arch/sh is doing it right (just to avoid the warning).
>
> I tested both definitions with bloat-o-metering and it showed no
> extra code.

It's better to replace it with:
static inline int flat_set_persistent(unsigned long relval,
							unsigned long *persistent)
{
	return 0;
}

No warnings, same generated code, type safety.
Look around the code. It's a very common pattern.

Marcin

Re: [Q] warning BUG() related fixing and janitors question

[Dan Carpenter]

On Mon, Apr 16, 2012 at 06:12:07PM -0300, Ezequiel García wrote:
> Hi Dan,
> 
> 2012/4/16 Dan Carpenter <dan.carpenter@oracle.com>:
> >
> > Don't start messing in scheduler code if you have any doubts about
> > what you are doing.  My advise is don't send a patch for this
> > warning.  Most times people leave BUG() enabled so we don't really
> > care about the warning.
> 
> Okey. Perhaps I picked the wrong example, there are a whole bunch of warnings
> related to the same BUG() stuff, here's a template example:
> 
> void *p;
> 
> switch (something) {
>   case a:
>     p = foo();
>   case b:
>     p = bar();
>   default:
>     BUG();
> }
> 
> dosomething(p);
> 
> --
> 
> This will trigger warning "p could be used uninitialized" or something.
> I know most of us just leave BUG on, so you say these warnings aren't important?

Yeah.  Probably best to ignore them.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Q] warning BUG() related fixing and janitors question

[Ezequiel García]

On Mon, Apr 16, 2012 at 7:16 PM, Marcin Ślusarz
<marcin.slusarz@gmail.com> wrote:
>
> It's better to replace it with:
> static inline int flat_set_persistent(unsigned long relval,
>                                                        unsigned long *persistent)
> {
>        return 0;
> }
>
> No warnings, same generated code, type safety.
> Look around the code. It's a very common pattern.
>

Yes, I guess you're right.
Plus it's even easier to understand than that #define in arch/sh.

Are these changes suitable, or am I being too picky?

Thanks both.
Ezequiel.

Re: [Q] warning BUG() related fixing and janitors question

[Marcin Ślusarz]

2012/4/17 Ezequiel García <elezegarcia@gmail.com>:
> On Mon, Apr 16, 2012 at 7:16 PM, Marcin Ślusarz
> <marcin.slusarz@gmail.com> wrote:
>>
>> It's better to replace it with:
>> static inline int flat_set_persistent(unsigned long relval,
>>                                                        unsigned long *persistent)
>> {
>>        return 0;
>> }
>>
>> No warnings, same generated code, type safety.
>> Look around the code. It's a very common pattern.
>>
>
> Yes, I guess you're right.
> Plus it's even easier to understand than that #define in arch/sh.
>
> Are these changes suitable, or am I being too picky?

Well, it depends. If you see warnings from the code, go for it.
If there's none and code is trivial or in little used module, don't bother.
In other cases, it depends on the maintainer of changed code - some
maintainers apply cleanups eagerly, others... ignore them. You will soon
learn which subsystems to avoid :(

Little advice: You will learn more if you concentrate on improving
one subsystem. Find something that interest you and dive into the code.
Small cleanups are easy, but they should only be a warm up.

Marcin