| Message ID | 1495458775-20946-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: > _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, it has > the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is logically > dead. > > Add an extra condition into the logic to skip the flag check if _PAGE_GNTTAB > is 0. And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look logically the same (i.e. I'd expect the same warnings to be triggered [or not]). > No functional change. > > Coverity-ID: 1362036 > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> So provided the change really silences Coverity: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
Hi, On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, it has >> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is logically >> dead. >> >> Add an extra condition into the logic to skip the flag check if _PAGE_GNTTAB >> is 0. > > And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look > logically the same (i.e. I'd expect the same warnings to be triggered > [or not]). I haven't seen any answer on this question. Andrew, does this patch still hold for Xen 4.9? > >> No functional change. >> >> Coverity-ID: 1362036 >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > So provided the change really silences Coverity: > Acked-by: Jan Beulich <jbeulich@suse.com> Cheers,
On 31/05/17 09:52, Julien Grall wrote: > Hi, > > On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, >>> it has >>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>> logically >>> dead. >>> >>> Add an extra condition into the logic to skip the flag check if >>> _PAGE_GNTTAB >>> is 0. >> >> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look >> logically the same (i.e. I'd expect the same warnings to be triggered >> [or not]). > > I haven't seen any answer on this question. Andrew, does this patch > still hold for Xen 4.9? Sorry - it fell through the cracks, but yes, it does stand for 4.9. As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very clearly a "short circuit every thing else if this value is zero", while the other looks like a programming mistake, which is also why I expect this to resolve Coverity's complaint. Unfortunately, I can't be certain that this will resolve the issue until it gets committed, as I don't have a useful way to run Coverity on arbitrary non-debug builds. ~Andrew > >> >>> No functional change. >>> >>> Coverity-ID: 1362036 >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> So provided the change really silences Coverity: >> Acked-by: Jan Beulich <jbeulich@suse.com> > > Cheers, >
Hi Andrew, On 31/05/17 14:23, Andrew Cooper wrote: > On 31/05/17 09:52, Julien Grall wrote: >> Hi, >> >> On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, >>>> it has >>>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>>> logically >>>> dead. >>>> >>>> Add an extra condition into the logic to skip the flag check if >>>> _PAGE_GNTTAB >>>> is 0. >>> >>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look >>> logically the same (i.e. I'd expect the same warnings to be triggered >>> [or not]). >> >> I haven't seen any answer on this question. Andrew, does this patch >> still hold for Xen 4.9? > > Sorry - it fell through the cracks, but yes, it does stand for 4.9. > > As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very > clearly a "short circuit every thing else if this value is zero", while > the other looks like a programming mistake, which is also why I expect > this to resolve Coverity's complaint. > > Unfortunately, I can't be certain that this will resolve the issue until > it gets committed, as I don't have a useful way to run Coverity on > arbitrary non-debug builds. Are we running staging-4.* branch on Coverity? Looking at the git, I see *coverity* branch only on unstable. Cheers,
>>> On 01.06.17 at 13:09, <julien.grall@arm.com> wrote: > Hi Andrew, > > On 31/05/17 14:23, Andrew Cooper wrote: >> On 31/05/17 09:52, Julien Grall wrote: >>> Hi, >>> >>> On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, >>>>> it has >>>>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>>>> logically >>>>> dead. >>>>> >>>>> Add an extra condition into the logic to skip the flag check if >>>>> _PAGE_GNTTAB >>>>> is 0. >>>> >>>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look >>>> logically the same (i.e. I'd expect the same warnings to be triggered >>>> [or not]). >>> >>> I haven't seen any answer on this question. Andrew, does this patch >>> still hold for Xen 4.9? >> >> Sorry - it fell through the cracks, but yes, it does stand for 4.9. >> >> As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very >> clearly a "short circuit every thing else if this value is zero", while >> the other looks like a programming mistake, which is also why I expect >> this to resolve Coverity's complaint. >> >> Unfortunately, I can't be certain that this will resolve the issue until >> it gets committed, as I don't have a useful way to run Coverity on >> arbitrary non-debug builds. > > Are we running staging-4.* branch on Coverity? Looking at the git, I see > *coverity* branch only on unstable. Yes, so my suggestion would be to commit the patch on master, see if it helps, and if so consider backporting for 4.9. If it doesn't help, it should be reverted or replaced by something "better". Jan
Hi Jan, On 01/06/17 12:15, Jan Beulich wrote: >>>> On 01.06.17 at 13:09, <julien.grall@arm.com> wrote: >> Hi Andrew, >> >> On 31/05/17 14:23, Andrew Cooper wrote: >>> On 31/05/17 09:52, Julien Grall wrote: >>>> Hi, >>>> >>>> On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>>>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, >>>>>> it has >>>>>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>>>>> logically >>>>>> dead. >>>>>> >>>>>> Add an extra condition into the logic to skip the flag check if >>>>>> _PAGE_GNTTAB >>>>>> is 0. >>>>> >>>>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look >>>>> logically the same (i.e. I'd expect the same warnings to be triggered >>>>> [or not]). >>>> >>>> I haven't seen any answer on this question. Andrew, does this patch >>>> still hold for Xen 4.9? >>> >>> Sorry - it fell through the cracks, but yes, it does stand for 4.9. >>> >>> As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very >>> clearly a "short circuit every thing else if this value is zero", while >>> the other looks like a programming mistake, which is also why I expect >>> this to resolve Coverity's complaint. >>> >>> Unfortunately, I can't be certain that this will resolve the issue until >>> it gets committed, as I don't have a useful way to run Coverity on >>> arbitrary non-debug builds. >> >> Are we running staging-4.* branch on Coverity? Looking at the git, I see >> *coverity* branch only on unstable. > > Yes, so my suggestion would be to commit the patch on master, see > if it helps, and if so consider backporting for 4.9. If it doesn't help, > it should be reverted or replaced by something "better". Well master has now debug enabled and AFAIU the warning can only occurred on non-debug build. So I am not sure how this would help to be in master. Cheers,
>>> On 01.06.17 at 13:18, <julien.grall@arm.com> wrote: > Hi Jan, > > On 01/06/17 12:15, Jan Beulich wrote: >>>>> On 01.06.17 at 13:09, <julien.grall@arm.com> wrote: >>> Hi Andrew, >>> >>> On 31/05/17 14:23, Andrew Cooper wrote: >>>> On 31/05/17 09:52, Julien Grall wrote: >>>>> Hi, >>>>> >>>>> On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>>>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>>>>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, >>>>>>> it has >>>>>>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>>>>>> logically >>>>>>> dead. >>>>>>> >>>>>>> Add an extra condition into the logic to skip the flag check if >>>>>>> _PAGE_GNTTAB >>>>>>> is 0. >>>>>> >>>>>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look >>>>>> logically the same (i.e. I'd expect the same warnings to be triggered >>>>>> [or not]). >>>>> >>>>> I haven't seen any answer on this question. Andrew, does this patch >>>>> still hold for Xen 4.9? >>>> >>>> Sorry - it fell through the cracks, but yes, it does stand for 4.9. >>>> >>>> As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very >>>> clearly a "short circuit every thing else if this value is zero", while >>>> the other looks like a programming mistake, which is also why I expect >>>> this to resolve Coverity's complaint. >>>> >>>> Unfortunately, I can't be certain that this will resolve the issue until >>>> it gets committed, as I don't have a useful way to run Coverity on >>>> arbitrary non-debug builds. >>> >>> Are we running staging-4.* branch on Coverity? Looking at the git, I see >>> *coverity* branch only on unstable. >> >> Yes, so my suggestion would be to commit the patch on master, see >> if it helps, and if so consider backporting for 4.9. If it doesn't help, >> it should be reverted or replaced by something "better". > > Well master has now debug enabled and AFAIU the warning can only > occurred on non-debug build. So I am not sure how this would help to be > in master. Oh, good point. I think it would really be a good idea to have both debug and non-debug builds tested, which at once would avoid sudden bursts of issues when switching the default. Jan
CC Ian + Wei for the testing On 01/06/17 12:21, Jan Beulich wrote: >>>> On 01.06.17 at 13:18, <julien.grall@arm.com> wrote: >> Hi Jan, >> >> On 01/06/17 12:15, Jan Beulich wrote: >>>>>> On 01.06.17 at 13:09, <julien.grall@arm.com> wrote: >>>> Hi Andrew, >>>> >>>> On 31/05/17 14:23, Andrew Cooper wrote: >>>>> On 31/05/17 09:52, Julien Grall wrote: >>>>>> Hi, >>>>>> >>>>>> On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>>>>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>>>>>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release builds, >>>>>>>> it has >>>>>>>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>>>>>>> logically >>>>>>>> dead. >>>>>>>> >>>>>>>> Add an extra condition into the logic to skip the flag check if >>>>>>>> _PAGE_GNTTAB >>>>>>>> is 0. >>>>>>> >>>>>>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && ... )" look >>>>>>> logically the same (i.e. I'd expect the same warnings to be triggered >>>>>>> [or not]). >>>>>> >>>>>> I haven't seen any answer on this question. Andrew, does this patch >>>>>> still hold for Xen 4.9? >>>>> >>>>> Sorry - it fell through the cracks, but yes, it does stand for 4.9. >>>>> >>>>> As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very >>>>> clearly a "short circuit every thing else if this value is zero", while >>>>> the other looks like a programming mistake, which is also why I expect >>>>> this to resolve Coverity's complaint. >>>>> >>>>> Unfortunately, I can't be certain that this will resolve the issue until >>>>> it gets committed, as I don't have a useful way to run Coverity on >>>>> arbitrary non-debug builds. >>>> >>>> Are we running staging-4.* branch on Coverity? Looking at the git, I see >>>> *coverity* branch only on unstable. >>> >>> Yes, so my suggestion would be to commit the patch on master, see >>> if it helps, and if so consider backporting for 4.9. If it doesn't help, >>> it should be reverted or replaced by something "better". >> >> Well master has now debug enabled and AFAIU the warning can only >> occurred on non-debug build. So I am not sure how this would help to be >> in master. > > Oh, good point. I think it would really be a good idea to have > both debug and non-debug builds tested, which at once would > avoid sudden bursts of issues when switching the default. > > Jan >
On 01/06/17 18:25, Julien Grall wrote: > CC Ian + Wei for the testing > > On 01/06/17 12:21, Jan Beulich wrote: >>>>> On 01.06.17 at 13:18, <julien.grall@arm.com> wrote: >>> Hi Jan, >>> >>> On 01/06/17 12:15, Jan Beulich wrote: >>>>>>> On 01.06.17 at 13:09, <julien.grall@arm.com> wrote: >>>>> Hi Andrew, >>>>> >>>>> On 31/05/17 14:23, Andrew Cooper wrote: >>>>>> On 31/05/17 09:52, Julien Grall wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 05/22/2017 02:32 PM, Jan Beulich wrote: >>>>>>>>>>> On 22.05.17 at 15:12, <andrew.cooper3@citrix.com> wrote: >>>>>>>>> _PAGE_GNTTAB is only used in debug builds of Xen; in release >>>>>>>>> builds, >>>>>>>>> it has >>>>>>>>> the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is >>>>>>>>> logically >>>>>>>>> dead. >>>>>>>>> >>>>>>>>> Add an extra condition into the logic to skip the flag check if >>>>>>>>> _PAGE_GNTTAB >>>>>>>>> is 0. >>>>>>>> >>>>>>>> And this helps? To me "if ( 0 && ... )" and "if ( (x & 0) && >>>>>>>> ... )" look >>>>>>>> logically the same (i.e. I'd expect the same warnings to be >>>>>>>> triggered >>>>>>>> [or not]). >>>>>>> >>>>>>> I haven't seen any answer on this question. Andrew, does this patch >>>>>>> still hold for Xen 4.9? >>>>>> >>>>>> Sorry - it fell through the cracks, but yes, it does stand for 4.9. >>>>>> >>>>>> As to the "if ( 0 && ... )" and "if ( (x & 0) && ... )", one is very >>>>>> clearly a "short circuit every thing else if this value is zero", >>>>>> while >>>>>> the other looks like a programming mistake, which is also why I >>>>>> expect >>>>>> this to resolve Coverity's complaint. >>>>>> >>>>>> Unfortunately, I can't be certain that this will resolve the >>>>>> issue until >>>>>> it gets committed, as I don't have a useful way to run Coverity on >>>>>> arbitrary non-debug builds. >>>>> >>>>> Are we running staging-4.* branch on Coverity? Looking at the git, >>>>> I see >>>>> *coverity* branch only on unstable. >>>> >>>> Yes, so my suggestion would be to commit the patch on master, see >>>> if it helps, and if so consider backporting for 4.9. If it doesn't >>>> help, >>>> it should be reverted or replaced by something "better". >>> >>> Well master has now debug enabled and AFAIU the warning can only >>> occurred on non-debug build. So I am not sure how this would help to be >>> in master. >> >> Oh, good point. I think it would really be a good idea to have >> both debug and non-debug builds tested, which at once would >> avoid sudden bursts of issues when switching the default. We cannot test both at the same time. The free Coverity Scan only offers a single stream, which means we can only submit a single logical stream of successive changes. Ideally, this patch would have gotten in to staging while coverity was still running the release build, so we would have gotten a positive answer on the next scan. Without taking this patch (assuming it fixes the issue), we will have the same issue flap in our results every time we do a release. ~Andrew
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 77b0af1..b588653 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -1252,7 +1252,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner) * (Note that the undestroyable active grants are not a security hole in * Xen. All active grants can safely be cleaned up when the domain dies.) */ - if ( (l1e_get_flags(l1e) & _PAGE_GNTTAB) && + if ( _PAGE_GNTTAB && (l1e_get_flags(l1e) & _PAGE_GNTTAB) && !l1e_owner->is_shutting_down && !l1e_owner->is_dying ) { gdprintk(XENLOG_WARNING,
_PAGE_GNTTAB is only used in debug builds of Xen; in release builds, it has the value 0. Coverity complains that "l1e_get_flags(l1e) & 0" is logically dead. Add an extra condition into the logic to skip the flag check if _PAGE_GNTTAB is 0. No functional change. Coverity-ID: 1362036 Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Julien Grall <julien.grall@arm.com> --- xen/arch/x86/mm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)