Re: [PATCH for -tip 4/4] irq: for_each_irq_desc() makes simplify

[KOSAKI Motohiro]

Hi

>>  # define for_each_irq_desc(irq, desc)                                        \
>>       for (irq = 0, desc = irq_to_desc(irq); irq < nr_irqs;           \
>> -          irq++, desc = irq_to_desc(irq))
>> +          irq++, desc = irq_to_desc(irq))                            \
>> +             if (desc)
>> +
>> +
>>  # define for_each_irq_desc_reverse(irq, desc)                                \
>>       for (irq = nr_irqs - 1, desc = irq_to_desc(irq); irq >= 0;      \
>> -          irq--, desc = irq_to_desc(irq))
>> +          irq--, desc = irq_to_desc(irq))                            \
>> +             if (desc)
>
> I know this has gone in, but isn't this naked 'if' unsafe.  Consider the
> following hypothetical code:
>
>  if (safe)
>    for_each_irq_desc(irq, desc) {
>       ...
>    }
>  else
>    panic();
>
> With the macro definition above, the loop would panic() each time !desc,
> and _not_ panic() when !safe.  I'd consider this behaviour to be
> unexpected, to say the least :-)

Correct.

> The fix is to change the
>
>  if (desc)
>
> in the macro to
>
>  if (!desc) ; else

Ok. I'll do that.
Very thanks for good reviewing.

btw, actually current kernel aready have similar macros.
e.g.

#define for_each_node_with_cpus(node)                   \
        for_each_online_node(node)                      \
                if (nr_cpus_node(node))


Shoud we fixed it too? ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/