Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
Hi Andrew, On 30/08/18 13:31, Andrew Cooper wrote: Callers are inconsistent with whether they pass a newline to panic(), including adjacent calls in the same function using different styles. painc() not expecting a newline is inconsistent with most other printing functions, which is most likely why we've gained so many inconsistencies. Switch panic() to expect a newline, and update all callers which currently lack a newline to include one. This actually reduces the size of .rodata (0x07e3e8 down to 0x07e3a8) because a number of strings are passed to both panic() and printk(). As they previously differed by \n alone, they couldn't be merged. Signed-off-by: Andrew Cooper Acked-by: Julien Grall Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
>>> On 07.09.18 at 18:42, wrote: > [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. Well, later on in the conversion I've also agreed that due to the extra stuff getting logged, your change is perhaps the best that can be done at this point in time. Feel free to read this as Acked-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
On 31/08/18 07:33, Jan Beulich wrote: On 30.08.18 at 19:08, wrote: >> On 30/08/18 15:07, Jan Beulich wrote: >> On 30.08.18 at 14:31, 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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
>>> On 31.08.18 at 09:30, wrote:
> On 31/08/18 09:21, Jan Beulich wrote:
> On 31.08.18 at 09:12, wrote:
>>> On 31/08/18 09:04, Jan Beulich wrote:
>>> On 31.08.18 at 08:43, wrote:
> On 31/08/18 08:33, Jan Beulich wrote:
> On 30.08.18 at 19:08, wrote:
>>> On 30/08/18 15:07, Jan Beulich wrote:
>>> On 30.08.18 at 14:31, 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.
>
> Can't we just drop printing the extra \n in panic()?
>
> -printk("%s\n", buf);
> +printk("%s", buf);
That's what I'm suggesting, yes, plus (if there are any) dropping
trailing newlines in panic() invocations.
>>>
>>> Uuh, both?
>>>
>>> This would look like:
>>>
>>> ***
>>> PANIC on cpu 2:
>>> blalblabla**
>>
>> Hmm, the trailing would perhaps better also be dropped. As per
>> my original reply to Andrew, the goal should imo be to have as little
>> useless output as possible (but as much as necessary/helpful, i.e. I'm
>> not suggesting to drop the first line of stars), to avoid meaningful
>> lines scrolling off.
>
> I don't think:
>
> ***
> PANIC on cpu 2:
> blalblablaReboot in five seconds...
>
> is what we want.
>
> So one '\n' should stay: either when printing the panic message, or in
> the panic message itself.
Oh, all these pretty useless extra lines. In that case I think Andrew's
patch is quite fine, unless we can reach agreement to drop all the
extra cruft such that the actual panic message is last on the screen.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
On 31/08/18 09:21, Jan Beulich wrote:
On 31.08.18 at 09:12, wrote:
>> On 31/08/18 09:04, Jan Beulich wrote:
>> On 31.08.18 at 08:43, wrote:
On 31/08/18 08:33, Jan Beulich wrote:
On 30.08.18 at 19:08, wrote:
>> On 30/08/18 15:07, Jan Beulich wrote:
>> On 30.08.18 at 14:31, 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.
Can't we just drop printing the extra \n in panic()?
-printk("%s\n", buf);
+printk("%s", buf);
>>>
>>> That's what I'm suggesting, yes, plus (if there are any) dropping
>>> trailing newlines in panic() invocations.
>>
>> Uuh, both?
>>
>> This would look like:
>>
>> ***
>> PANIC on cpu 2:
>> blalblabla**
>
> Hmm, the trailing would perhaps better also be dropped. As per
> my original reply to Andrew, the goal should imo be to have as little
> useless output as possible (but as much as necessary/helpful, i.e. I'm
> not suggesting to drop the first line of stars), to avoid meaningful
> lines scrolling off.
I don't think:
***
PANIC on cpu 2:
blalblablaReboot in five seconds...
is what we want.
So one '\n' should stay: either when printing the panic message, or in
the panic message itself.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
>>> On 31.08.18 at 09:12, wrote:
> On 31/08/18 09:04, Jan Beulich wrote:
> On 31.08.18 at 08:43, wrote:
>>> On 31/08/18 08:33, Jan Beulich wrote:
>>> On 30.08.18 at 19:08, wrote:
> On 30/08/18 15:07, Jan Beulich wrote:
> On 30.08.18 at 14:31, 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.
>>>
>>> Can't we just drop printing the extra \n in panic()?
>>>
>>> -printk("%s\n", buf);
>>> +printk("%s", buf);
>>
>> That's what I'm suggesting, yes, plus (if there are any) dropping
>> trailing newlines in panic() invocations.
>
> Uuh, both?
>
> This would look like:
>
> ***
> PANIC on cpu 2:
> blalblabla**
Hmm, the trailing would perhaps better also be dropped. As per
my original reply to Andrew, the goal should imo be to have as little
useless output as possible (but as much as necessary/helpful, i.e. I'm
not suggesting to drop the first line of stars), to avoid meaningful
lines scrolling off.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
On 31/08/18 09:04, Jan Beulich wrote:
On 31.08.18 at 08:43, wrote:
>> On 31/08/18 08:33, Jan Beulich wrote:
>> On 30.08.18 at 19:08, wrote:
On 30/08/18 15:07, Jan Beulich wrote:
On 30.08.18 at 14:31, 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.
>>
>> Can't we just drop printing the extra \n in panic()?
>>
>> -printk("%s\n", buf);
>> +printk("%s", buf);
>
> That's what I'm suggesting, yes, plus (if there are any) dropping
> trailing newlines in panic() invocations.
Uuh, both?
This would look like:
***
PANIC on cpu 2:
blalblabla**
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
>>> On 31.08.18 at 08:43, wrote:
> On 31/08/18 08:33, Jan Beulich wrote:
> On 30.08.18 at 19:08, wrote:
>>> On 30/08/18 15:07, Jan Beulich wrote:
>>> On 30.08.18 at 14:31, 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.
>
> Can't we just drop printing the extra \n in panic()?
>
> -printk("%s\n", buf);
> +printk("%s", buf);
That's what I'm suggesting, yes, plus (if there are any) dropping
trailing newlines in panic() invocations.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
On 31/08/18 08:33, Jan Beulich wrote:
On 30.08.18 at 19:08, wrote:
>> On 30/08/18 15:07, Jan Beulich wrote:
>> On 30.08.18 at 14:31, 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.
Can't we just drop printing the extra \n in panic()?
-printk("%s\n", buf);
+printk("%s", buf);
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
>>> On 30.08.18 at 19:08, wrote: > On 30/08/18 15:07, Jan Beulich wrote: > On 30.08.18 at 14:31, 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 need to adhere to some common principle? If so, I don't think I can agree with you here. > 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. Chances are even so good that people tend to copy and paste buggy code without paying attention. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
On 30/08/18 15:07, Jan Beulich wrote: On 30.08.18 at 14:31, wrote: >> Callers are inconsistent with whether they pass a newline to panic(), >> including adjacent calls in the same function using different styles. >> >> painc() not expecting a newline is inconsistent with most other printing >> functions, which is most likely why we've gained so many inconsistencies. >> >> Switch panic() to expect a newline, and update all callers which currently >> lack a newline to include one. >> >> This actually reduces the size of .rodata (0x07e3e8 down to 0x07e3a8) because >> a number of strings are passed to both panic() and printk(). As they >> previously differed by \n alone, they couldn't be merged. > I agree this is a nice side effect, but ... > >> Signed-off-by: Andrew Cooper >> --- >> CC: Jan Beulich >> CC: Wei Liu >> CC: Stefano Stabellini >> CC: Julien Grall >> >> (Restricted to the core arch maintainers as this is a tree-wide piece of >> cleanup with no functional impact to other areas.) >> >> 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. 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). 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. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen: Fix inconsistent callers of panic()
>>> On 30.08.18 at 14:31, wrote: > Callers are inconsistent with whether they pass a newline to panic(), > including adjacent calls in the same function using different styles. > > painc() not expecting a newline is inconsistent with most other printing > functions, which is most likely why we've gained so many inconsistencies. > > Switch panic() to expect a newline, and update all callers which currently > lack a newline to include one. > > This actually reduces the size of .rodata (0x07e3e8 down to 0x07e3a8) because > a number of strings are passed to both panic() and printk(). As they > previously differed by \n alone, they couldn't be merged. I agree this is a nice side effect, but ... > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Wei Liu > CC: Stefano Stabellini > CC: Julien Grall > > (Restricted to the core arch maintainers as this is a tree-wide piece of > cleanup with no functional impact to other areas.) > > 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. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
