| Message ID | 1e0f12095bcbc82ae3585c9fcf57bec7e324049c.1697638210.git.simone.ballarin@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: address violations of Rule 13.1 | expand |
On Wed, 18 Oct 2023, Simone Ballarin wrote: > Rule 13.1: Initializer lists shall not contain persistent side effects > > This patch moves expressions with side-effects outside the initializer lists. > > No functional changes. > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > --- > xen/common/sched/core.c | 16 ++++++++++++---- > xen/drivers/char/ns16550.c | 4 +++- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c > index 12deefa745..519884f989 100644 > --- a/xen/common/sched/core.c > +++ b/xen/common/sched/core.c > @@ -1504,6 +1504,8 @@ long vcpu_yield(void) > { > struct vcpu * v=current; > spinlock_t *lock; > + domid_t domain_id; > + int vcpu_id; > > rcu_read_lock(&sched_res_rculock); > > @@ -1515,7 +1517,9 @@ long vcpu_yield(void) > > SCHED_STAT_CRANK(vcpu_yield); > > - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); > + domain_id = current->domain->domain_id; > + vcpu_id = current->vcpu_id; > + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); Also here it looks like accessing the current pointer is considered a side effect. Why? This is a just a global variable that is only accessed for reading. > raise_softirq(SCHEDULE_SOFTIRQ); > return 0; > } > @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > case SCHEDOP_shutdown: > { > struct sched_shutdown sched_shutdown; > + domid_t domain_id; > + int vcpu_id; > > ret = -EFAULT; > if ( copy_from_guest(&sched_shutdown, arg, 1) ) > break; > > + domain_id = current->domain->domain_id; > + vcpu_id = current->vcpu_id; > TRACE_3D(TRC_SCHED_SHUTDOWN, > - current->domain->domain_id, current->vcpu_id, > - sched_shutdown.reason); > + domain_id, vcpu_id, sched_shutdown.reason); > ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason); > > break; > @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > { > struct sched_shutdown sched_shutdown; > struct domain *d = current->domain; > + int vcpu_id = current->vcpu_id; > > ret = -EFAULT; > if ( copy_from_guest(&sched_shutdown, arg, 1) ) > break; > > TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, > - d->domain_id, current->vcpu_id, sched_shutdown.reason); > + d->domain_id, vcpu_id, sched_shutdown.reason); > > spin_lock(&d->shutdown_lock); > if ( d->shutdown_code == SHUTDOWN_CODE_INVALID ) > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > index 28ddedd50d..0b3d8b2a30 100644 > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -445,10 +445,12 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) > struct msi_info msi = { > .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], > uart->ps_bdf[2]), > - .irq = rc = uart->irq, > + .irq = uart->irq, > .entry_nr = 1 > }; > > + rc = uart->irq; What's the side effect here? If the side effect is rc = uart->irq, why is it considered a side effect?
On 19/10/23 03:06, Stefano Stabellini wrote: > On Wed, 18 Oct 2023, Simone Ballarin wrote: >> Rule 13.1: Initializer lists shall not contain persistent side effects >> >> This patch moves expressions with side-effects outside the initializer lists. >> >> No functional changes. >> >> Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> >> --- >> xen/common/sched/core.c | 16 ++++++++++++---- >> xen/drivers/char/ns16550.c | 4 +++- >> 2 files changed, 15 insertions(+), 5 deletions(-) >> >> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c >> index 12deefa745..519884f989 100644 >> --- a/xen/common/sched/core.c >> +++ b/xen/common/sched/core.c >> @@ -1504,6 +1504,8 @@ long vcpu_yield(void) >> { >> struct vcpu * v=current; >> spinlock_t *lock; >> + domid_t domain_id; >> + int vcpu_id; >> >> rcu_read_lock(&sched_res_rculock); >> >> @@ -1515,7 +1517,9 @@ long vcpu_yield(void) >> >> SCHED_STAT_CRANK(vcpu_yield); >> >> - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); >> + domain_id = current->domain->domain_id; >> + vcpu_id = current->vcpu_id; >> + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); > > Also here it looks like accessing the current pointer is considered a > side effect. Why? This is a just a global variable that is only accessed > for reading. > > >> raise_softirq(SCHEDULE_SOFTIRQ); >> return 0; >> } >> @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> case SCHEDOP_shutdown: >> { >> struct sched_shutdown sched_shutdown; >> + domid_t domain_id; >> + int vcpu_id; >> >> ret = -EFAULT; >> if ( copy_from_guest(&sched_shutdown, arg, 1) ) >> break; >> >> + domain_id = current->domain->domain_id; >> + vcpu_id = current->vcpu_id; >> TRACE_3D(TRC_SCHED_SHUTDOWN, >> - current->domain->domain_id, current->vcpu_id, >> - sched_shutdown.reason); >> + domain_id, vcpu_id, sched_shutdown.reason); >> ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason); >> >> break; >> @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) >> { >> struct sched_shutdown sched_shutdown; >> struct domain *d = current->domain; >> + int vcpu_id = current->vcpu_id; >> >> ret = -EFAULT; >> if ( copy_from_guest(&sched_shutdown, arg, 1) ) >> break; >> >> TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, >> - d->domain_id, current->vcpu_id, sched_shutdown.reason); >> + d->domain_id, vcpu_id, sched_shutdown.reason); >> >> spin_lock(&d->shutdown_lock); >> if ( d->shutdown_code == SHUTDOWN_CODE_INVALID ) >> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c >> index 28ddedd50d..0b3d8b2a30 100644 >> --- a/xen/drivers/char/ns16550.c >> +++ b/xen/drivers/char/ns16550.c >> @@ -445,10 +445,12 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) >> struct msi_info msi = { >> .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], >> uart->ps_bdf[2]), >> - .irq = rc = uart->irq, >> + .irq = uart->irq, >> .entry_nr = 1 >> }; >> >> + rc = uart->irq; > > What's the side effect here? If the side effect is rc = uart->irq, why > is it considered a side effect? > Yes, rc = uart->irq is the side-effect: it writes a variable declared outside the context of the expression. Consider the following example: int rc; struct S s = { .x = rc = 2, .y = rc = 3 }; What's the value of rc?
On 19.10.2023 03:06, Stefano Stabellini wrote: > On Wed, 18 Oct 2023, Simone Ballarin wrote: >> --- a/xen/common/sched/core.c >> +++ b/xen/common/sched/core.c >> @@ -1504,6 +1504,8 @@ long vcpu_yield(void) >> { >> struct vcpu * v=current; >> spinlock_t *lock; >> + domid_t domain_id; >> + int vcpu_id; >> >> rcu_read_lock(&sched_res_rculock); >> >> @@ -1515,7 +1517,9 @@ long vcpu_yield(void) >> >> SCHED_STAT_CRANK(vcpu_yield); >> >> - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); >> + domain_id = current->domain->domain_id; >> + vcpu_id = current->vcpu_id; >> + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); > > Also here it looks like accessing the current pointer is considered a > side effect. Why? This is a just a global variable that is only accessed > for reading. Not exactly. It's a per-CPU variable access on Arm, but involves a function call on x86. Still that function call has no other side effects, but I guess Misra wouldn't honor this. I'm afraid though that the suggested change violates rule 2.2, as the two new assignments are dead code when !CONFIG_TRACEBUFFER. Jan
On 19/10/23 11:35, Jan Beulich wrote: > On 19.10.2023 03:06, Stefano Stabellini wrote: >> On Wed, 18 Oct 2023, Simone Ballarin wrote: >>> --- a/xen/common/sched/core.c >>> +++ b/xen/common/sched/core.c >>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void) >>> { >>> struct vcpu * v=current; >>> spinlock_t *lock; >>> + domid_t domain_id; >>> + int vcpu_id; >>> >>> rcu_read_lock(&sched_res_rculock); >>> >>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void) >>> >>> SCHED_STAT_CRANK(vcpu_yield); >>> >>> - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); >>> + domain_id = current->domain->domain_id; >>> + vcpu_id = current->vcpu_id; >>> + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); >> >> Also here it looks like accessing the current pointer is considered a >> side effect. Why? This is a just a global variable that is only accessed >> for reading. > > Not exactly. It's a per-CPU variable access on Arm, but involves a > function call on x86. Still that function call has no other side > effects, but I guess Misra wouldn't honor this. > > I'm afraid though that the suggested change violates rule 2.2, as > the two new assignments are dead code when !CONFIG_TRACEBUFFER. > > Jan I confirm that there is no problem in X86: I will simply propose a patch adding __attribute_pure__ to get_cpu_info.
On 19.10.2023 13:12, Simone Ballarin wrote: > On 19/10/23 11:35, Jan Beulich wrote: >> On 19.10.2023 03:06, Stefano Stabellini wrote: >>> On Wed, 18 Oct 2023, Simone Ballarin wrote: >>>> --- a/xen/common/sched/core.c >>>> +++ b/xen/common/sched/core.c >>>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void) >>>> { >>>> struct vcpu * v=current; >>>> spinlock_t *lock; >>>> + domid_t domain_id; >>>> + int vcpu_id; >>>> >>>> rcu_read_lock(&sched_res_rculock); >>>> >>>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void) >>>> >>>> SCHED_STAT_CRANK(vcpu_yield); >>>> >>>> - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); >>>> + domain_id = current->domain->domain_id; >>>> + vcpu_id = current->vcpu_id; >>>> + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); >>> >>> Also here it looks like accessing the current pointer is considered a >>> side effect. Why? This is a just a global variable that is only accessed >>> for reading. >> >> Not exactly. It's a per-CPU variable access on Arm, but involves a >> function call on x86. Still that function call has no other side >> effects, but I guess Misra wouldn't honor this. >> >> I'm afraid though that the suggested change violates rule 2.2, as >> the two new assignments are dead code when !CONFIG_TRACEBUFFER. > > I confirm that there is no problem in X86: I will simply propose a patch > adding __attribute_pure__ to get_cpu_info. I specifically did not suggest that, because I'm afraid the "pure" attribute may not be used there. Besides this attribute typically being discarded for inline functions in favor of the compiler's own judgement, it would allow the compiler to squash multiple invocations. This might even be desirable in most cases, but would break across a stack pointer change. (In this context you also need to keep in mind late optimizations done by LTO.) Jan
On 19/10/23 13:19, Jan Beulich wrote: > On 19.10.2023 13:12, Simone Ballarin wrote: >> On 19/10/23 11:35, Jan Beulich wrote: >>> On 19.10.2023 03:06, Stefano Stabellini wrote: >>>> On Wed, 18 Oct 2023, Simone Ballarin wrote: >>>>> --- a/xen/common/sched/core.c >>>>> +++ b/xen/common/sched/core.c >>>>> @@ -1504,6 +1504,8 @@ long vcpu_yield(void) >>>>> { >>>>> struct vcpu * v=current; >>>>> spinlock_t *lock; >>>>> + domid_t domain_id; >>>>> + int vcpu_id; >>>>> >>>>> rcu_read_lock(&sched_res_rculock); >>>>> >>>>> @@ -1515,7 +1517,9 @@ long vcpu_yield(void) >>>>> >>>>> SCHED_STAT_CRANK(vcpu_yield); >>>>> >>>>> - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); >>>>> + domain_id = current->domain->domain_id; >>>>> + vcpu_id = current->vcpu_id; >>>>> + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); >>>> >>>> Also here it looks like accessing the current pointer is considered a >>>> side effect. Why? This is a just a global variable that is only accessed >>>> for reading. >>> >>> Not exactly. It's a per-CPU variable access on Arm, but involves a >>> function call on x86. Still that function call has no other side >>> effects, but I guess Misra wouldn't honor this. >>> >>> I'm afraid though that the suggested change violates rule 2.2, as >>> the two new assignments are dead code when !CONFIG_TRACEBUFFER. >> >> I confirm that there is no problem in X86: I will simply propose a patch >> adding __attribute_pure__ to get_cpu_info. > > I specifically did not suggest that, because I'm afraid the "pure" attribute > may not be used there. Besides this attribute typically being discarded for > inline functions in favor of the compiler's own judgement, it would allow > the compiler to squash multiple invocations. This might even be desirable in > most cases, but would break across a stack pointer change. (In this context > you also need to keep in mind late optimizations done by LTO.) > > Jan > Ok, in this case I will just configure ECLAIR to consider it without effects.
On Thu, 19 Oct 2023, Simone Ballarin wrote: > On 19/10/23 13:19, Jan Beulich wrote: > > On 19.10.2023 13:12, Simone Ballarin wrote: > > > On 19/10/23 11:35, Jan Beulich wrote: > > > > On 19.10.2023 03:06, Stefano Stabellini wrote: > > > > > On Wed, 18 Oct 2023, Simone Ballarin wrote: > > > > > > --- a/xen/common/sched/core.c > > > > > > +++ b/xen/common/sched/core.c > > > > > > @@ -1504,6 +1504,8 @@ long vcpu_yield(void) > > > > > > { > > > > > > struct vcpu * v=current; > > > > > > spinlock_t *lock; > > > > > > + domid_t domain_id; > > > > > > + int vcpu_id; > > > > > > rcu_read_lock(&sched_res_rculock); > > > > > > @@ -1515,7 +1517,9 @@ long vcpu_yield(void) > > > > > > SCHED_STAT_CRANK(vcpu_yield); > > > > > > - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, > > > > > > current->vcpu_id); > > > > > > + domain_id = current->domain->domain_id; > > > > > > + vcpu_id = current->vcpu_id; > > > > > > + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); > > > > > > > > > > Also here it looks like accessing the current pointer is considered a > > > > > side effect. Why? This is a just a global variable that is only > > > > > accessed > > > > > for reading. > > > > > > > > Not exactly. It's a per-CPU variable access on Arm, but involves a > > > > function call on x86. Still that function call has no other side > > > > effects, but I guess Misra wouldn't honor this. > > > > > > > > I'm afraid though that the suggested change violates rule 2.2, as > > > > the two new assignments are dead code when !CONFIG_TRACEBUFFER. > > > > > > I confirm that there is no problem in X86: I will simply propose a patch > > > adding __attribute_pure__ to get_cpu_info. > > > > I specifically did not suggest that, because I'm afraid the "pure" attribute > > may not be used there. Besides this attribute typically being discarded for > > inline functions in favor of the compiler's own judgement, it would allow > > the compiler to squash multiple invocations. This might even be desirable in > > most cases, but would break across a stack pointer change. (In this context > > you also need to keep in mind late optimizations done by LTO.) > > > > Jan > > > > Ok, in this case I will just configure ECLAIR to consider it without effects. I think that's fine, just remember to either use SAF or document under docs/misra/deviations.rst
On Thu, 19 Oct 2023, Simone Ballarin wrote: > On 19/10/23 03:06, Stefano Stabellini wrote: > > On Wed, 18 Oct 2023, Simone Ballarin wrote: > > > Rule 13.1: Initializer lists shall not contain persistent side effects > > > > > > This patch moves expressions with side-effects outside the initializer > > > lists. > > > > > > No functional changes. > > > > > > Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> > > > --- > > > xen/common/sched/core.c | 16 ++++++++++++---- > > > xen/drivers/char/ns16550.c | 4 +++- > > > 2 files changed, 15 insertions(+), 5 deletions(-) > > > > > > diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c > > > index 12deefa745..519884f989 100644 > > > --- a/xen/common/sched/core.c > > > +++ b/xen/common/sched/core.c > > > @@ -1504,6 +1504,8 @@ long vcpu_yield(void) > > > { > > > struct vcpu * v=current; > > > spinlock_t *lock; > > > + domid_t domain_id; > > > + int vcpu_id; > > > rcu_read_lock(&sched_res_rculock); > > > @@ -1515,7 +1517,9 @@ long vcpu_yield(void) > > > SCHED_STAT_CRANK(vcpu_yield); > > > - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, > > > current->vcpu_id); > > > + domain_id = current->domain->domain_id; > > > + vcpu_id = current->vcpu_id; > > > + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); > > > > Also here it looks like accessing the current pointer is considered a > > side effect. Why? This is a just a global variable that is only accessed > > for reading. > > > > > > > raise_softirq(SCHEDULE_SOFTIRQ); > > > return 0; > > > } > > > @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, > > > XEN_GUEST_HANDLE_PARAM(void) arg) > > > case SCHEDOP_shutdown: > > > { > > > struct sched_shutdown sched_shutdown; > > > + domid_t domain_id; > > > + int vcpu_id; > > > ret = -EFAULT; > > > if ( copy_from_guest(&sched_shutdown, arg, 1) ) > > > break; > > > + domain_id = current->domain->domain_id; > > > + vcpu_id = current->vcpu_id; > > > TRACE_3D(TRC_SCHED_SHUTDOWN, > > > - current->domain->domain_id, current->vcpu_id, > > > - sched_shutdown.reason); > > > + domain_id, vcpu_id, sched_shutdown.reason); > > > ret = domain_shutdown(current->domain, > > > (u8)sched_shutdown.reason); > > > break; > > > @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, > > > XEN_GUEST_HANDLE_PARAM(void) arg) > > > { > > > struct sched_shutdown sched_shutdown; > > > struct domain *d = current->domain; > > > + int vcpu_id = current->vcpu_id; > > > ret = -EFAULT; > > > if ( copy_from_guest(&sched_shutdown, arg, 1) ) > > > break; > > > TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, > > > - d->domain_id, current->vcpu_id, sched_shutdown.reason); > > > + d->domain_id, vcpu_id, sched_shutdown.reason); > > > spin_lock(&d->shutdown_lock); > > > if ( d->shutdown_code == SHUTDOWN_CODE_INVALID ) > > > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c > > > index 28ddedd50d..0b3d8b2a30 100644 > > > --- a/xen/drivers/char/ns16550.c > > > +++ b/xen/drivers/char/ns16550.c > > > @@ -445,10 +445,12 @@ static void __init cf_check > > > ns16550_init_postirq(struct serial_port *port) > > > struct msi_info msi = { > > > .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], > > > uart->ps_bdf[2]), > > > - .irq = rc = uart->irq, > > > + .irq = uart->irq, > > > .entry_nr = 1 > > > }; > > > + rc = uart->irq; > > > > What's the side effect here? If the side effect is rc = uart->irq, why > > is it considered a side effect? > > > > Yes, rc = uart->irq is the side-effect: it writes a variable > declared outside the context of the expression. > > Consider the following example: > > int rc; > > struct S s = { > .x = rc = 2, > .y = rc = 3 > }; > > What's the value of rc? OK thanks for the explanation.
On 18.10.2023 16:18, Simone Ballarin wrote: > --- a/xen/common/sched/core.c > +++ b/xen/common/sched/core.c > @@ -1504,6 +1504,8 @@ long vcpu_yield(void) > { > struct vcpu * v=current; > spinlock_t *lock; > + domid_t domain_id; > + int vcpu_id; While I realize that the field this variable is initialized from is plain "int", I still think that being wrong means the new variables here and below want to be "unsigned int". > @@ -1515,7 +1517,9 @@ long vcpu_yield(void) > > SCHED_STAT_CRANK(vcpu_yield); > > - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); > + domain_id = current->domain->domain_id; > + vcpu_id = current->vcpu_id; > + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); If you touch this, I think you ought to also switch to using "v" in place of "current" (making the supposed side effect go away aiui). Yet then (for the further changes to this file) - what persistent side effect does reading "current" have? Clarification of that is part of the justification for this change, and hence ought to be spelled out in the description. > --- a/xen/drivers/char/ns16550.c > +++ b/xen/drivers/char/ns16550.c > @@ -445,10 +445,12 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) > struct msi_info msi = { > .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], > uart->ps_bdf[2]), > - .irq = rc = uart->irq, > + .irq = uart->irq, > .entry_nr = 1 > }; > > + rc = uart->irq; > + > if ( rc > 0 ) If this needs transforming, I think we'd better switch it to rc = 0; if ( uart->irq > 0 ) thus matching what we have elsewhere in the function. Jan
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c index 12deefa745..519884f989 100644 --- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -1504,6 +1504,8 @@ long vcpu_yield(void) { struct vcpu * v=current; spinlock_t *lock; + domid_t domain_id; + int vcpu_id; rcu_read_lock(&sched_res_rculock); @@ -1515,7 +1517,9 @@ long vcpu_yield(void) SCHED_STAT_CRANK(vcpu_yield); - TRACE_2D(TRC_SCHED_YIELD, current->domain->domain_id, current->vcpu_id); + domain_id = current->domain->domain_id; + vcpu_id = current->vcpu_id; + TRACE_2D(TRC_SCHED_YIELD, domain_id, vcpu_id); raise_softirq(SCHEDULE_SOFTIRQ); return 0; } @@ -1888,14 +1892,17 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) case SCHEDOP_shutdown: { struct sched_shutdown sched_shutdown; + domid_t domain_id; + int vcpu_id; ret = -EFAULT; if ( copy_from_guest(&sched_shutdown, arg, 1) ) break; + domain_id = current->domain->domain_id; + vcpu_id = current->vcpu_id; TRACE_3D(TRC_SCHED_SHUTDOWN, - current->domain->domain_id, current->vcpu_id, - sched_shutdown.reason); + domain_id, vcpu_id, sched_shutdown.reason); ret = domain_shutdown(current->domain, (u8)sched_shutdown.reason); break; @@ -1905,13 +1912,14 @@ ret_t do_sched_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { struct sched_shutdown sched_shutdown; struct domain *d = current->domain; + int vcpu_id = current->vcpu_id; ret = -EFAULT; if ( copy_from_guest(&sched_shutdown, arg, 1) ) break; TRACE_3D(TRC_SCHED_SHUTDOWN_CODE, - d->domain_id, current->vcpu_id, sched_shutdown.reason); + d->domain_id, vcpu_id, sched_shutdown.reason); spin_lock(&d->shutdown_lock); if ( d->shutdown_code == SHUTDOWN_CODE_INVALID ) diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index 28ddedd50d..0b3d8b2a30 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -445,10 +445,12 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port) struct msi_info msi = { .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]), - .irq = rc = uart->irq, + .irq = uart->irq, .entry_nr = 1 }; + rc = uart->irq; + if ( rc > 0 ) { struct msi_desc *msi_desc = NULL;
Rule 13.1: Initializer lists shall not contain persistent side effects This patch moves expressions with side-effects outside the initializer lists. No functional changes. Signed-off-by: Simone Ballarin <simone.ballarin@bugseng.com> --- xen/common/sched/core.c | 16 ++++++++++++---- xen/drivers/char/ns16550.c | 4 +++- 2 files changed, 15 insertions(+), 5 deletions(-)