| Message ID | 149206979424.1420.4973403493863924866.stgit@Solace.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Dario, On 13/04/17 08:49, Dario Faggioli wrote: > Since the counter is unsigned, it's pointless/bogous to check > for if to be above zero. > > Check that it is at least one before it's decremented, instead. > > Spotted by Coverity. Do you have the Coverity-ID? :) > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > --- > Julien, > > This is very low risk, and I'd call it a bugfix in the sense that it quiesces > coverity. Release-Acked-by: Julien Grall <julien.grall@arm.com> > > Dario > --- > xen/common/sched_credit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index 93658dc..efdf6bf 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -275,8 +275,8 @@ static inline void > dec_nr_runnable(unsigned int cpu) > { > ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock)); > + ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 1); > CSCHED_PCPU(cpu)->nr_runnable--; > - ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 0); > } > > static inline void > Cheers,
On Thu, Apr 13, 2017 at 10:42:56AM +0100, Julien Grall wrote: > Hi Dario, > > On 13/04/17 08:49, Dario Faggioli wrote: > > Since the counter is unsigned, it's pointless/bogous to check > > for if to be above zero. > > > > Check that it is at least one before it's decremented, instead. > > > > Spotted by Coverity. > > Do you have the Coverity-ID? :) > Not really. It is XenServer's internal instance.
On Thu, 2017-04-13 at 10:42 +0100, Julien Grall wrote: > Hi Dario, > > On 13/04/17 08:49, Dario Faggioli wrote: > > Since the counter is unsigned, it's pointless/bogous to check > > for if to be above zero. > > > > Check that it is at least one before it's decremented, instead. > > > > Spotted by Coverity. > > Do you have the Coverity-ID? :) > This comes from the Citrix internal instance, so the ID wouldn't make any sense. I don't know if the XenProject instance has also caught it. I guess it's likely, but I don't have access, so I can't check. Adding the above line is what Andrew suggested doing (saying he does it himself) when things like this happens, to better reflect the reality. Let me know if I should do anything different (of feel free to add or change anything related to this upon commit). Regards, Dario
On 13/04/17 11:00, Dario Faggioli wrote: > On Thu, 2017-04-13 at 10:42 +0100, Julien Grall wrote: >> Hi Dario, >> >> On 13/04/17 08:49, Dario Faggioli wrote: >>> Since the counter is unsigned, it's pointless/bogous to check >>> for if to be above zero. >>> >>> Check that it is at least one before it's decremented, instead. >>> >>> Spotted by Coverity. >> Do you have the Coverity-ID? :) >> > This comes from the Citrix internal instance, so the ID wouldn't make > any sense. > > I don't know if the XenProject instance has also caught it. I guess > it's likely, but I don't have access, so I can't check. OSSTest is currently failing at the Coverity jobs, probably because of firewall issues uploading the analysis results. I expect it would have noticed, had an upload succeeded recently. On the subject of access, we really should open it up now. The fact that anyone can clone Xen and run Coverity themselves means there is no point in keeping the main one private. > > Adding the above line is what Andrew suggested doing (saying he does it > himself) when things like this happens, to better reflect the reality. It is the least bad way I've found of expressing the difference. > > Let me know if I should do anything different (of feel free to add or > change anything related to this upon commit). > > Regards, > Dario
On 13/04/17 08:49, Dario Faggioli wrote: > Since the counter is unsigned, it's pointless/bogous to check > for if to be above zero. > > Check that it is at least one before it's decremented, instead. > > Spotted by Coverity. > > Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> Reviewed-by: George Dunlap <george.dunlap@citrix.com> And queued > --- > Cc: George Dunlap <george.dunlap@citrix.com> > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > --- > Julien, > > This is very low risk, and I'd call it a bugfix in the sense that it quiesces > coverity. > > Dario > --- > xen/common/sched_credit.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c > index 93658dc..efdf6bf 100644 > --- a/xen/common/sched_credit.c > +++ b/xen/common/sched_credit.c > @@ -275,8 +275,8 @@ static inline void > dec_nr_runnable(unsigned int cpu) > { > ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock)); > + ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 1); > CSCHED_PCPU(cpu)->nr_runnable--; > - ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 0); > } > > static inline void >
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c index 93658dc..efdf6bf 100644 --- a/xen/common/sched_credit.c +++ b/xen/common/sched_credit.c @@ -275,8 +275,8 @@ static inline void dec_nr_runnable(unsigned int cpu) { ASSERT(spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock)); + ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 1); CSCHED_PCPU(cpu)->nr_runnable--; - ASSERT(CSCHED_PCPU(cpu)->nr_runnable >= 0); } static inline void
Since the counter is unsigned, it's pointless/bogous to check for if to be above zero. Check that it is at least one before it's decremented, instead. Spotted by Coverity. Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Julien Grall <julien.grall@arm.com> --- Julien, This is very low risk, and I'd call it a bugfix in the sense that it quiesces coverity. Dario --- xen/common/sched_credit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)