Re: [PATCH 14/24] [NETFILTER]: Use bool in nf_conntrack_l4proto

[Patrick McHardy <kaber@trash.net>]

Jan Engelhardt wrote:
> On Thursday 2008-04-03 17:00, Patrick McHardy wrote:
>>
>> Some hints for the future to make this easier for both of us:
>>
>> - submit small batches, split into logical units
> 
> This sets me up a bit.. sometimes it's "should have folded these"
> (like the const annotation patches that were at the start of
> the series), then it's "smaller batches" :-/

Folding patches makes the batches smaller (note: batch, not
patch) :) What I meant by this was to not flood me with 30
patches three times at once, if issues come up in one of the
first patches it often results in the later ones not applying.
Logical batches, like

- batch 1: constification
- batch 2: boolean conversions
- batch x: things like rename ipt_recent, add IPv6 support
- batch y: arp_tables userspace interface changes in order
            to achieve X.

>> - avoid style changes like this, especially when the entire file
>>   uses a consistent style:
>>
>>>  -static int icmpv6_pkt_to_tuple(const struct sk_buff *skb,
>>>  -                   unsigned int dataoff,
>>>  -                   struct nf_conntrack_tuple *tuple)
> 
> allow me the remark of "consistently odd", as you can see what a
> change in indent does when spaces are not used in the right
> place. But whatever, yeah.
> 
>>>  +static bool
>>>  +icmpv6_pkt_to_tuple(const struct sk_buff *skb, unsigned int dataoff,
>>>  +                    struct nf_conntrack_tuple *tuple)
>>
>> - run checkpatch
> 
> Not before I rip out that incredibly stupid "use tabs" warning.
> The warning may be right for users who apparently have not dealt
> with patch submitting process a lot, but for longtime contributers
> that get their style right the 1st time it's just wrong.

No, we've fixed up net/ more than once using scripts before
checkpatch even existed. Simple: keep existing style.

>     That program has no sense for when spaces are needed.
> The style I used and use (tabs=indent, spaces=align - it only makes
> sense) was always fine by you, but now it's not because
> checkpatch says so?

See above. I used to use a different indenting style as well,
but I prefer consistency over having it exactly as I like it.

>> - don't mix cleanups with real changes
>> - run sparse with endian checks
>> - compile test all the code your changing, including CONFIG_COMPAT
> 
> Will do.
>