Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/08/18 07:33, Jan Beulich wrote:
>>>> On 30.08.18 at 19:08, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 30/08/18 15:07, Jan Beulich wrote:
>>>>>> On 30.08.18 at 14:31, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> The observant amongst you might realise that this reverts parts of c/s
>>>> 51ad90aea21c - What can I say?  Several years of hindsight is very useful, 
>>>> and
>>>> at the time I did ask the maintainers which option they thought would be
>>>> better...
>>> ... I think both the earlier and this change are heading in the
>>> wrong direction: I would much rather see the newline omitted
>>> everywhere, _including_ in panic() itself: This causes one line
>>> less to scroll off the screen in case you don't have a serial
>>> console.
>> I don't expect that suggestion would get anywhere if you put it to a
>> vote with The REST.
> Well, I can certainly live with being the only one here, should
> that turn out to be the case.
>
>> For one, it breaks any ability to construct a single line of text from
>> multiple printk() calls (which we have plenty of examples of in the
>> codebase), and it further deviates from everyone’s expectation of how
>> printk() works (which is the very reason we've picked up all these
>> inconsistencies since I last made them consistent).
> Let me understand this: Are you suggesting two (by their names
> and purposes) completely different functions

I'm going to stop you mid sentence and object to printk() and panic()
being classified as "completely different".

The are both, first and foremost, used for getting information out onto
the console.  panic() just has an extra side effect of crashing the machine.

> need to adhere to some common principle? If so, I don't think I can agree 
> with you
> here.

Functions with common usage should adhere to common principles. 
Therefore, panic() and printk() should be consistent on whether they
take a newline or not in their format string.

Furthermore, APIs of ours which are modelled after an existing API, in
this case printf(), should have the same expectations, because that is
how people learn C.  Therefore, printk() and panic() should have a
trailing newline in the format string if a newline wants emitting on the
console.

>> IMO, such a change would be detrimental, because either the code will
>> get out of sync again (most likely), or there will extra review
>> aggravation as people submitting code to normal expectations have their
>> code rejected.
> Quite frequently people follow existing practice when adding new
> code. If all panic() invocations omitted the newline, chances are
> pretty good that new instances would do so as well.

This patch is a 3 year case study demonstrating the exact opposite.

I made panic() consistently (not) take a \n, and 3 years later, we have
still got back to a 50% ~mix.

> Chances are even so good that people tend to copy and paste buggy code
> without paying attention.

When everyone is submitting code which seems to slip \n in (including
you - c/s 5784de3 which was the XPTI patch), and none of the
reviewers/committers notice and fix it up, it is obvious that the
current expectations are wrong.

[Merging part of the later conversion]

Reducing the number of lines printed from a panic/backtrace may be a
good thing, but please can we not conflate it with this which is fix an
API inconsistency.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel