Re: [PATCH 0/1] Constify struct address_space_operations for 2.6.32-git-053fe57acv2

[Emese Revfy]

Paul Mundt wrote:
> I don't see anything relating to sparse in that mail. You've effectively
> lumped sparse and constification together in the same camp, but it's
> unclear why this makes constification a better option other than that
> it's simply the option you opted for. All of your arguments "against"
> sparse in that context are equally applicable to constification, so I'll
> reiterate that you haven't sufficiently addressed the sparse angle.
> 
> At present you seem to be the only one convinced that constification is
> the way to go, despite it being highly intrusive and ignoring the
> potential for more favourable and less intrusive options. You've also
> failed to adequately address the issues and suggestsions pointed out by
> others, and until this happens there is little point in posting any
> follow-up patches.
> 
>>> Until such a consensus is reached one way or the other, please refrain
>>> from sending hundreds of patches -- one or two are sufficient for showing
>>> what you want to do until folks are on board with it, as is the typical
>>> nature of mechanical changes.
>> I think there is consensus to constify ops variables as much as
>> possible (e.g., Alexey's similar patches).
>>
>> The discussions in these threads were about constifying the ops structure
>> fields themselves and I already explained why they are useful, see the
>> above link and this one: http://lkml.org/lkml/2009/12/8/492
> 
> And in here as well in the reply to that mail the same criticism exists
> as does the suggestion to look at doing it cleanly in sparse, which
> brings us back to what was already mentioned earlier.

Let me summarise the discussion so far:

As per Al Viro, Arjan and other developers the goal is to force
static allocations and prevent runtime modification of ops structures
(where it is possible, there are always exceptions like ata_port_operations).

The current strategy of constifying variables achieves the second goal only,
it still requires human review to catch violations of the first goal.

This is where consitfying the structure field becomes important: it prevents
direct modifications of runtime allocated ops structures therefore it
gives a strong signal to the programmer that he's trying to do something
undesired (this approach is in fact already used in the kernel, see: iwl_ops).
There is another benefit in that static but non-const ops structures cannot be
directly modified either, therefore it will be easier to make them const later.

Of course both constification efforts can be bypassed, a "clever" programmer can
write code in many ways that will write to otherwise "const" structures.
Nor is it possible to detect all such attempts by tools in fact, it would be
equivalent to solving the halting problem.

Therefore I think that it's a lot easier to have the compiler detect unwanted
direct modifications by constifying the structure fields than use sparse (which,
unlike a compiler, isn't used by everyone and would require more complex changes
than field constification for no real gain). In any case, constifying structure
fields is not exclusive of teaching sparse or other tools like checkpatch about
some bad code constructs, I will try my best on checkpatch.

To wrap it all up: human review will always be required to catch bad code and
we can help the process if we force would-be violators to go to lengths to
bypass the policy and make it easy for the reviewer to notice that something
is up.

> Thinking you have consensus because you don't see a difference and don't
> bother replying to the feedback you've gotten doesn't bode well for the
> future of your patch series or killfile avoidance strategy.

Please let me know whose feedback I didn't address.
--
Emese
--
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/