* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 7:30 [PATCH] xen: Append a newline character to panic() where missing Michal Orzel
@ 2023-06-14 7:45 ` Luca Fancellu
2023-06-14 8:00 ` Jan Beulich
2023-06-14 8:04 ` Andrew Cooper
2023-06-14 9:51 ` Bertrand Marquis
2 siblings, 1 reply; 10+ messages in thread
From: Luca Fancellu @ 2023-06-14 7:45 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Andrew Cooper,
Roger Pau Monné, Wei Liu
> On 14 Jun 2023, at 08:30, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Missing newline is inconsistent with the rest of the callers, since
> panic() expects it.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
> ---
> xen/arch/arm/bootfdt.c | 2 +-
> xen/arch/arm/domain_build.c | 6 +++---
> xen/arch/x86/cpu/microcode/core.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index b6f92a174f5f..2673ad17a1e1 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
> size_cells, data);
>
> if ( rc == -ENOSPC )
> - panic("Max number of supported reserved-memory regions reached.");
> + panic("Max number of supported reserved-memory regions reached.\n");
> else if ( rc != -ENOENT )
> return rc;
> return 0;
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 579bc8194fed..d0d6be922db1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -74,7 +74,7 @@ int __init parse_arch_dom0_param(const char *s, const char *e)
>
> return 0;
> #else
> - panic("'sve' property found, but CONFIG_ARM64_SVE not selected");
> + panic("'sve' property found, but CONFIG_ARM64_SVE not selected\n");
Sorry about that! I’ve missed it
> #endif
> }
>
> @@ -697,7 +697,7 @@ static void __init allocate_static_memory(struct domain *d,
> return;
>
> fail:
> - panic("Failed to allocate requested static memory for domain %pd.", d);
> + panic("Failed to allocate requested static memory for domain %pd.\n", d);
> }
>
> /*
> @@ -769,7 +769,7 @@ static void __init assign_static_memory_11(struct domain *d,
> return;
>
> fail:
> - panic("Failed to assign requested static memory for direct-map domain %pd.",
> + panic("Failed to assign requested static memory for direct-map domain %pd.\n",
> d);
> }
>
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index e65af4b82ea3..c3fee629063e 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -524,7 +524,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
> */
> if ( wait_for_condition(wait_cpu_callout, (done + 1),
> MICROCODE_UPDATE_TIMEOUT_US) )
> - panic("Timeout when finished updating microcode (finished %u/%u)",
> + panic("Timeout when finished updating microcode (finished %u/%u)\n",
> done, nr_cores);
>
> /* Print warning message once if long time is spent here */
>
> base-commit: 2f69ef96801f0d2b9646abf6396e60f99c56e3a0
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 7:45 ` Luca Fancellu
@ 2023-06-14 8:00 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2023-06-14 8:00 UTC (permalink / raw)
To: Luca Fancellu, Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Andrew Cooper, Roger Pau Monné, Wei Liu
On 14.06.2023 09:45, Luca Fancellu wrote:
>> On 14 Jun 2023, at 08:30, Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> Missing newline is inconsistent with the rest of the callers, since
>> panic() expects it.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>
> Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 7:30 [PATCH] xen: Append a newline character to panic() where missing Michal Orzel
2023-06-14 7:45 ` Luca Fancellu
@ 2023-06-14 8:04 ` Andrew Cooper
2023-06-14 8:09 ` Michal Orzel
2023-06-14 9:51 ` Bertrand Marquis
2 siblings, 1 reply; 10+ messages in thread
From: Andrew Cooper @ 2023-06-14 8:04 UTC (permalink / raw)
To: Michal Orzel, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Roger Pau Monné, Wei Liu
On 14/06/2023 8:30 am, Michal Orzel wrote:
> Missing newline is inconsistent with the rest of the callers, since
> panic() expects it.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
although...
> ---
> xen/arch/arm/bootfdt.c | 2 +-
> xen/arch/arm/domain_build.c | 6 +++---
> xen/arch/x86/cpu/microcode/core.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index b6f92a174f5f..2673ad17a1e1 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
> size_cells, data);
>
> if ( rc == -ENOSPC )
> - panic("Max number of supported reserved-memory regions reached.");
> + panic("Max number of supported reserved-memory regions reached.\n");
Trailing punctuation like . or ! is useless. Most messages don't have
them, and it just takes up space in .rodata, the console ring, and time
on the UART.
I'd recommend dropping the ones you modify, and/or cleaning it up more
widely.
~Andrew
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 8:04 ` Andrew Cooper
@ 2023-06-14 8:09 ` Michal Orzel
2023-06-14 9:02 ` Julien Grall
0 siblings, 1 reply; 10+ messages in thread
From: Michal Orzel @ 2023-06-14 8:09 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Julien Grall, Bertrand Marquis,
Volodymyr Babchuk, Jan Beulich, Roger Pau Monné, Wei Liu
On 14/06/2023 10:04, Andrew Cooper wrote:
>
>
> On 14/06/2023 8:30 am, Michal Orzel wrote:
>> Missing newline is inconsistent with the rest of the callers, since
>> panic() expects it.
>>
>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> although...
>
>> ---
>> xen/arch/arm/bootfdt.c | 2 +-
>> xen/arch/arm/domain_build.c | 6 +++---
>> xen/arch/x86/cpu/microcode/core.c | 2 +-
>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>> index b6f92a174f5f..2673ad17a1e1 100644
>> --- a/xen/arch/arm/bootfdt.c
>> +++ b/xen/arch/arm/bootfdt.c
>> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
>> size_cells, data);
>>
>> if ( rc == -ENOSPC )
>> - panic("Max number of supported reserved-memory regions reached.");
>> + panic("Max number of supported reserved-memory regions reached.\n");
>
> Trailing punctuation like . or ! is useless. Most messages don't have
> them, and it just takes up space in .rodata, the console ring, and time
> on the UART.
>
> I'd recommend dropping the ones you modify, and/or cleaning it up more
> widely.
I will keep in mind to do that in global scope in the next patch.
We also have quite a lot of printk() with trailing punctuation.
~Michal
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 8:09 ` Michal Orzel
@ 2023-06-14 9:02 ` Julien Grall
2023-06-14 9:06 ` Michal Orzel
0 siblings, 1 reply; 10+ messages in thread
From: Julien Grall @ 2023-06-14 9:02 UTC (permalink / raw)
To: Michal Orzel, Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Jan Beulich, Roger Pau Monné, Wei Liu
On 14/06/2023 09:09, Michal Orzel wrote:
>
>
> On 14/06/2023 10:04, Andrew Cooper wrote:
>>
>>
>> On 14/06/2023 8:30 am, Michal Orzel wrote:
>>> Missing newline is inconsistent with the rest of the callers, since
>>> panic() expects it.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>
>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> although...
>>
>>> ---
>>> xen/arch/arm/bootfdt.c | 2 +-
>>> xen/arch/arm/domain_build.c | 6 +++---
>>> xen/arch/x86/cpu/microcode/core.c | 2 +-
>>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>> index b6f92a174f5f..2673ad17a1e1 100644
>>> --- a/xen/arch/arm/bootfdt.c
>>> +++ b/xen/arch/arm/bootfdt.c
>>> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
>>> size_cells, data);
>>>
>>> if ( rc == -ENOSPC )
>>> - panic("Max number of supported reserved-memory regions reached.");
>>> + panic("Max number of supported reserved-memory regions reached.\n");
>>
>> Trailing punctuation like . or ! is useless. Most messages don't have
>> them, and it just takes up space in .rodata, the console ring, and time
>> on the UART.
>>
>> I'd recommend dropping the ones you modify, and/or cleaning it up more
>> widely.
> I will keep in mind to do that in global scope in the next patch.
> We also have quite a lot of printk() with trailing punctuation.
This is quite a bit of churn and I am unconvinced this is necessary.
Also, if the others want such style, then this should be agreed on in
the CODING_STYLE first. Otherwise, I am afraid this is not something I
will enforce because I see limited value.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 9:02 ` Julien Grall
@ 2023-06-14 9:06 ` Michal Orzel
2023-06-14 9:09 ` Julien Grall
0 siblings, 1 reply; 10+ messages in thread
From: Michal Orzel @ 2023-06-14 9:06 UTC (permalink / raw)
To: Julien Grall, Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Jan Beulich, Roger Pau Monné, Wei Liu
On 14/06/2023 11:02, Julien Grall wrote:
>
>
> On 14/06/2023 09:09, Michal Orzel wrote:
>>
>>
>> On 14/06/2023 10:04, Andrew Cooper wrote:
>>>
>>>
>>> On 14/06/2023 8:30 am, Michal Orzel wrote:
>>>> Missing newline is inconsistent with the rest of the callers, since
>>>> panic() expects it.
>>>>
>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>
>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> although...
>>>
>>>> ---
>>>> xen/arch/arm/bootfdt.c | 2 +-
>>>> xen/arch/arm/domain_build.c | 6 +++---
>>>> xen/arch/x86/cpu/microcode/core.c | 2 +-
>>>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>>> index b6f92a174f5f..2673ad17a1e1 100644
>>>> --- a/xen/arch/arm/bootfdt.c
>>>> +++ b/xen/arch/arm/bootfdt.c
>>>> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
>>>> size_cells, data);
>>>>
>>>> if ( rc == -ENOSPC )
>>>> - panic("Max number of supported reserved-memory regions reached.");
>>>> + panic("Max number of supported reserved-memory regions reached.\n");
>>>
>>> Trailing punctuation like . or ! is useless. Most messages don't have
>>> them, and it just takes up space in .rodata, the console ring, and time
>>> on the UART.
>>>
>>> I'd recommend dropping the ones you modify, and/or cleaning it up more
>>> widely.
>> I will keep in mind to do that in global scope in the next patch.
>> We also have quite a lot of printk() with trailing punctuation.
>
> This is quite a bit of churn and I am unconvinced this is necessary.
> Also, if the others want such style, then this should be agreed on in
> the CODING_STYLE first. Otherwise, I am afraid this is not something I
> will enforce because I see limited value.
>
I then suggest to take this patch as is if you are also happy with it like others.
~Michal
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 9:06 ` Michal Orzel
@ 2023-06-14 9:09 ` Julien Grall
2023-06-15 20:53 ` Julien Grall
0 siblings, 1 reply; 10+ messages in thread
From: Julien Grall @ 2023-06-14 9:09 UTC (permalink / raw)
To: Michal Orzel, Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Jan Beulich, Roger Pau Monné, Wei Liu
On 14/06/2023 10:06, Michal Orzel wrote:
>
>
> On 14/06/2023 11:02, Julien Grall wrote:
>>
>>
>> On 14/06/2023 09:09, Michal Orzel wrote:
>>>
>>>
>>> On 14/06/2023 10:04, Andrew Cooper wrote:
>>>>
>>>>
>>>> On 14/06/2023 8:30 am, Michal Orzel wrote:
>>>>> Missing newline is inconsistent with the rest of the callers, since
>>>>> panic() expects it.
>>>>>
>>>>> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
>>>>
>>>> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>
>>>> although...
>>>>
>>>>> ---
>>>>> xen/arch/arm/bootfdt.c | 2 +-
>>>>> xen/arch/arm/domain_build.c | 6 +++---
>>>>> xen/arch/x86/cpu/microcode/core.c | 2 +-
>>>>> 3 files changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
>>>>> index b6f92a174f5f..2673ad17a1e1 100644
>>>>> --- a/xen/arch/arm/bootfdt.c
>>>>> +++ b/xen/arch/arm/bootfdt.c
>>>>> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
>>>>> size_cells, data);
>>>>>
>>>>> if ( rc == -ENOSPC )
>>>>> - panic("Max number of supported reserved-memory regions reached.");
>>>>> + panic("Max number of supported reserved-memory regions reached.\n");
>>>>
>>>> Trailing punctuation like . or ! is useless. Most messages don't have
>>>> them, and it just takes up space in .rodata, the console ring, and time
>>>> on the UART.
>>>>
>>>> I'd recommend dropping the ones you modify, and/or cleaning it up more
>>>> widely.
>>> I will keep in mind to do that in global scope in the next patch.
>>> We also have quite a lot of printk() with trailing punctuation.
>>
>> This is quite a bit of churn and I am unconvinced this is necessary.
>> Also, if the others want such style, then this should be agreed on in
>> the CODING_STYLE first. Otherwise, I am afraid this is not something I
>> will enforce because I see limited value.
>>
> I then suggest to take this patch as is if you are also happy with it like others.
The patch looks fine. I will commit it a bit later just to give a chance
to Bertrand/Stefano to object.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 9:09 ` Julien Grall
@ 2023-06-15 20:53 ` Julien Grall
0 siblings, 0 replies; 10+ messages in thread
From: Julien Grall @ 2023-06-15 20:53 UTC (permalink / raw)
To: Michal Orzel, Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Bertrand Marquis, Volodymyr Babchuk,
Jan Beulich, Roger Pau Monné, Wei Liu
Hi,
On 14/06/2023 10:09, Julien Grall wrote:
> The patch looks fine. I will commit it a bit later just to give a chance
> to Bertrand/Stefano to object.
And committed.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen: Append a newline character to panic() where missing
2023-06-14 7:30 [PATCH] xen: Append a newline character to panic() where missing Michal Orzel
2023-06-14 7:45 ` Luca Fancellu
2023-06-14 8:04 ` Andrew Cooper
@ 2023-06-14 9:51 ` Bertrand Marquis
2 siblings, 0 replies; 10+ messages in thread
From: Bertrand Marquis @ 2023-06-14 9:51 UTC (permalink / raw)
To: Michal Orzel
Cc: Xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
Jan Beulich, Andrew Cooper, Roger Pau Monné, Wei Liu
Hi Michal,
> On 14 Jun 2023, at 09:30, Michal Orzel <michal.orzel@amd.com> wrote:
>
> Missing newline is inconsistent with the rest of the callers, since
> panic() expects it.
>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
And I agree with Julien: trailing punctuation is not an issue and
I would definitely not require you to fix it.
Cheers
Bertrand
> ---
> xen/arch/arm/bootfdt.c | 2 +-
> xen/arch/arm/domain_build.c | 6 +++---
> xen/arch/x86/cpu/microcode/core.c | 2 +-
> 3 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index b6f92a174f5f..2673ad17a1e1 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -225,7 +225,7 @@ static int __init process_reserved_memory_node(const void *fdt, int node,
> size_cells, data);
>
> if ( rc == -ENOSPC )
> - panic("Max number of supported reserved-memory regions reached.");
> + panic("Max number of supported reserved-memory regions reached.\n");
> else if ( rc != -ENOENT )
> return rc;
> return 0;
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 579bc8194fed..d0d6be922db1 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -74,7 +74,7 @@ int __init parse_arch_dom0_param(const char *s, const char *e)
>
> return 0;
> #else
> - panic("'sve' property found, but CONFIG_ARM64_SVE not selected");
> + panic("'sve' property found, but CONFIG_ARM64_SVE not selected\n");
> #endif
> }
>
> @@ -697,7 +697,7 @@ static void __init allocate_static_memory(struct domain *d,
> return;
>
> fail:
> - panic("Failed to allocate requested static memory for domain %pd.", d);
> + panic("Failed to allocate requested static memory for domain %pd.\n", d);
> }
>
> /*
> @@ -769,7 +769,7 @@ static void __init assign_static_memory_11(struct domain *d,
> return;
>
> fail:
> - panic("Failed to assign requested static memory for direct-map domain %pd.",
> + panic("Failed to assign requested static memory for direct-map domain %pd.\n",
> d);
> }
>
> diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
> index e65af4b82ea3..c3fee629063e 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -524,7 +524,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
> */
> if ( wait_for_condition(wait_cpu_callout, (done + 1),
> MICROCODE_UPDATE_TIMEOUT_US) )
> - panic("Timeout when finished updating microcode (finished %u/%u)",
> + panic("Timeout when finished updating microcode (finished %u/%u)\n",
> done, nr_cores);
>
> /* Print warning message once if long time is spent here */
>
> base-commit: 2f69ef96801f0d2b9646abf6396e60f99c56e3a0
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread