Rule 13.1: Initializer lists shall not contain persistent side effects
This patch moves expressions with side-effects into new variables before
the initializer lists.
No functional changes.
Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
---
xen/arch/x86/io_apic.c | 9 ++++++---
xen/arch/x86/mpparse.c | 3 ++-
xen/arch/x86/setup.c | 3 ++-
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index b48a642465..4a6ab85689 100644
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs);
static __init bool bad_ioapic_register(unsigned int idx)
{
- union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) };
- union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) };
- union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) };
+ uint32_t reg_00_raw = io_apic_read(idx, 0);
+ uint32_t reg_01_raw = io_apic_read(idx, 1);
+ uint32_t reg_02_raw = io_apic_read(idx, 2);
+ union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw };
+ union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw };
+ union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw };
if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 )
{
diff --git a/xen/arch/x86/mpparse.c b/xen/arch/x86/mpparse.c
index d8ccab2449..81a819403b 100644
--- a/xen/arch/x86/mpparse.c
+++ b/xen/arch/x86/mpparse.c
@@ -798,11 +798,12 @@ void __init mp_register_lapic_address (
int mp_register_lapic(u32 id, bool enabled, bool hotplug)
{
+ u32 apic = apic_read(APIC_LVR);
struct mpc_config_processor processor = {
.mpc_type = MP_PROCESSOR,
/* Note: We don't fill in fields not consumed anywhere. */
.mpc_apicid = id,
- .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)),
+ .mpc_apicver = GET_APIC_VERSION(apic),
.mpc_cpuflag = (enabled ? CPU_ENABLED : 0) |
(id == boot_cpu_physical_apicid ?
CPU_BOOTPROCESSOR : 0),
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ee682dd136..886031d86a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image,
{
static char __initdata cmdline[MAX_GUEST_CMDLINE];
+ unsigned int max_vcpus = dom0_max_vcpus();
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
.max_evtchn_port = -1,
.max_grant_frames = -1,
.max_maptrack_frames = -1,
.grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
- .max_vcpus = dom0_max_vcpus(),
+ .max_vcpus = max_vcpus,
.arch = {
.misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
},
--
2.34.1On 02.02.2024 16:16, Simone Ballarin wrote:
> Rule 13.1: Initializer lists shall not contain persistent side effects
>
> This patch moves expressions with side-effects into new variables before
> the initializer lists.
>
> No functional changes.
>
> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
To be honest, I don't like this. It's more code for no gain. Really its
hampering clarity imo. I'm willing to be convinced otherwise, but for
now this gets a nack from me.
As an aside, ...
> --- a/xen/arch/x86/io_apic.c
> +++ b/xen/arch/x86/io_apic.c
> @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs);
>
> static __init bool bad_ioapic_register(unsigned int idx)
> {
> - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) };
> - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) };
> - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) };
> + uint32_t reg_00_raw = io_apic_read(idx, 0);
> + uint32_t reg_01_raw = io_apic_read(idx, 1);
> + uint32_t reg_02_raw = io_apic_read(idx, 2);
... while you properly use uint32_t here, ...
> + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw };
> + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw };
> + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw };
>
> if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 )
> {
> --- a/xen/arch/x86/mpparse.c
> +++ b/xen/arch/x86/mpparse.c
> @@ -798,11 +798,12 @@ void __init mp_register_lapic_address (
>
> int mp_register_lapic(u32 id, bool enabled, bool hotplug)
> {
> + u32 apic = apic_read(APIC_LVR);
... why the being-phased-out u32 here?
Jan
> struct mpc_config_processor processor = {
> .mpc_type = MP_PROCESSOR,
> /* Note: We don't fill in fields not consumed anywhere. */
> .mpc_apicid = id,
> - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)),
> + .mpc_apicver = GET_APIC_VERSION(apic),
> .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) |
> (id == boot_cpu_physical_apicid ?
> CPU_BOOTPROCESSOR : 0),
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image,
> {
> static char __initdata cmdline[MAX_GUEST_CMDLINE];
>
> + unsigned int max_vcpus = dom0_max_vcpus();
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> .max_evtchn_port = -1,
> .max_grant_frames = -1,
> .max_maptrack_frames = -1,
> .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
> - .max_vcpus = dom0_max_vcpus(),
> + .max_vcpus = max_vcpus,
> .arch = {
> .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
> },
On 06/02/24 14:13, Jan Beulich wrote:
> On 02.02.2024 16:16, Simone Ballarin wrote:
>> Rule 13.1: Initializer lists shall not contain persistent side effects
>>
>> This patch moves expressions with side-effects into new variables before
>> the initializer lists.
>>
>> No functional changes.
>>
>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>
> To be honest, I don't like this. It's more code for no gain. Really its
> hampering clarity imo. I'm willing to be convinced otherwise, but for
> now this gets a nack from me.
>
> As an aside, ...
>
>> --- a/xen/arch/x86/io_apic.c
>> +++ b/xen/arch/x86/io_apic.c
>> @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs);
>>
>> static __init bool bad_ioapic_register(unsigned int idx)
>> {
>> - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) };
>> - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) };
>> - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) };
>> + uint32_t reg_00_raw = io_apic_read(idx, 0);
>> + uint32_t reg_01_raw = io_apic_read(idx, 1);
>> + uint32_t reg_02_raw = io_apic_read(idx, 2);
I'll propose a deviation for these single item initializers.
>
> ... while you properly use uint32_t here, ...
>
>> + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw };
>> + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw };
>> + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw };
>>
>> if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 )
>> {
>> --- a/xen/arch/x86/mpparse.c
>> +++ b/xen/arch/x86/mpparse.c
>> @@ -798,11 +798,12 @@ void __init mp_register_lapic_address (
>>
>> int mp_register_lapic(u32 id, bool enabled, bool hotplug)
>> {
>> + u32 apic = apic_read(APIC_LVR);>
> ... why the being-phased-out u32 here?
>
It was just to be coherent with the type of id.
I will use uint32_t in the next submission.
> Jan
>
>> struct mpc_config_processor processor = {
>> .mpc_type = MP_PROCESSOR,
>> /* Note: We don't fill in fields not consumed anywhere. */
>> .mpc_apicid = id,
>> - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)),
>> + .mpc_apicver = GET_APIC_VERSION(apic),
>> .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) |
>> (id == boot_cpu_physical_apicid ?
>> CPU_BOOTPROCESSOR : 0),
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image,
>> {
>> static char __initdata cmdline[MAX_GUEST_CMDLINE];
>>
>> + unsigned int max_vcpus = dom0_max_vcpus();
>> struct xen_domctl_createdomain dom0_cfg = {
>> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
>> .max_evtchn_port = -1,
>> .max_grant_frames = -1,
>> .max_maptrack_frames = -1,
>> .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
>> - .max_vcpus = dom0_max_vcpus(),
>> + .max_vcpus = max_vcpus,
>> .arch = {
>> .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
>> },
>
--
Simone Ballarin, M.Sc.
Field Application Engineer, BUGSENG (https://bugseng.com)
On Tue, 6 Feb 2024, Jan Beulich wrote:
> On 02.02.2024 16:16, Simone Ballarin wrote:
> > Rule 13.1: Initializer lists shall not contain persistent side effects
> >
> > This patch moves expressions with side-effects into new variables before
> > the initializer lists.
> >
> > No functional changes.
> >
> > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com>
>
> To be honest, I don't like this. It's more code for no gain. Really its
> hampering clarity imo. I'm willing to be convinced otherwise, but for
> now this gets a nack from me.
Which part of the patch you don't like? The changes to
bad_ioapic_register?
> As an aside, ...
>
> > --- a/xen/arch/x86/io_apic.c
> > +++ b/xen/arch/x86/io_apic.c
> > @@ -2559,9 +2559,12 @@ integer_param("max_gsi_irqs", max_gsi_irqs);
> >
> > static __init bool bad_ioapic_register(unsigned int idx)
> > {
> > - union IO_APIC_reg_00 reg_00 = { .raw = io_apic_read(idx, 0) };
> > - union IO_APIC_reg_01 reg_01 = { .raw = io_apic_read(idx, 1) };
> > - union IO_APIC_reg_02 reg_02 = { .raw = io_apic_read(idx, 2) };
> > + uint32_t reg_00_raw = io_apic_read(idx, 0);
> > + uint32_t reg_01_raw = io_apic_read(idx, 1);
> > + uint32_t reg_02_raw = io_apic_read(idx, 2);
>
> ... while you properly use uint32_t here, ...
>
> > + union IO_APIC_reg_00 reg_00 = { .raw = reg_00_raw };
> > + union IO_APIC_reg_01 reg_01 = { .raw = reg_01_raw };
> > + union IO_APIC_reg_02 reg_02 = { .raw = reg_02_raw };
> >
> > if ( reg_00.raw == -1 && reg_01.raw == -1 && reg_02.raw == -1 )
> > {
> > --- a/xen/arch/x86/mpparse.c
> > +++ b/xen/arch/x86/mpparse.c
> > @@ -798,11 +798,12 @@ void __init mp_register_lapic_address (
> >
> > int mp_register_lapic(u32 id, bool enabled, bool hotplug)
> > {
> > + u32 apic = apic_read(APIC_LVR);
>
> ... why the being-phased-out u32 here?
>
> Jan
>
> > struct mpc_config_processor processor = {
> > .mpc_type = MP_PROCESSOR,
> > /* Note: We don't fill in fields not consumed anywhere. */
> > .mpc_apicid = id,
> > - .mpc_apicver = GET_APIC_VERSION(apic_read(APIC_LVR)),
> > + .mpc_apicver = GET_APIC_VERSION(apic),
> > .mpc_cpuflag = (enabled ? CPU_ENABLED : 0) |
> > (id == boot_cpu_physical_apicid ?
> > CPU_BOOTPROCESSOR : 0),
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -885,13 +885,14 @@ static struct domain *__init create_dom0(const module_t *image,
> > {
> > static char __initdata cmdline[MAX_GUEST_CMDLINE];
> >
> > + unsigned int max_vcpus = dom0_max_vcpus();
> > struct xen_domctl_createdomain dom0_cfg = {
> > .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> > .max_evtchn_port = -1,
> > .max_grant_frames = -1,
> > .max_maptrack_frames = -1,
> > .grant_opts = XEN_DOMCTL_GRANT_version(opt_gnttab_max_version),
> > - .max_vcpus = dom0_max_vcpus(),
> > + .max_vcpus = max_vcpus,
> > .arch = {
> > .misc_flags = opt_dom0_msr_relaxed ? XEN_X86_MSR_RELAXED : 0,
> > },
>
On 07.02.2024 01:56, Stefano Stabellini wrote: > On Tue, 6 Feb 2024, Jan Beulich wrote: >> On 02.02.2024 16:16, Simone Ballarin wrote: >>> Rule 13.1: Initializer lists shall not contain persistent side effects >>> >>> This patch moves expressions with side-effects into new variables before >>> the initializer lists. >>> >>> No functional changes. >>> >>> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> >> To be honest, I don't like this. It's more code for no gain. Really its >> hampering clarity imo. I'm willing to be convinced otherwise, but for >> now this gets a nack from me. > > Which part of the patch you don't like? The changes to > bad_ioapic_register? Really all of them. bad_ioapic_register() is merely worst. Jan
© 2016 - 2026 Red Hat, Inc.