Message ID | 1483988759-14606-1-git-send-email-peter.maydell@linaro.org |
---|---|
State | Superseded |
Headers | show |
Hi, Your series seems to have some coding style problems. See output below for more information: Subject: [Qemu-devel] [PATCH] disas/cris.c: Fix Coverity warning about unchecked NULL Type: series Message-id: 1483988759-14606-1-git-send-email-peter.maydell@linaro.org === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1483988759-14606-1-git-send-email-peter.maydell@linaro.org -> patchew/1483988759-14606-1-git-send-email-peter.maydell@linaro.org Switched to a new branch 'test' 63cfa26 disas/cris.c: Fix Coverity warning about unchecked NULL === OUTPUT BEGIN === Checking PATCH 1/1: disas/cris.c: Fix Coverity warning about unchecked NULL... ERROR: code indent should never use tabs #24: FILE: disas/cris.c:2493: +^Iif (sregp == NULL || sregp->name == NULL)$ ERROR: suspect code indent for conditional statements (8, 10) #24: FILE: disas/cris.c:2493: + if (sregp == NULL || sregp->name == NULL) /* Should have been caught as a non-match earlier. */ ERROR: braces {} are necessary for all arms of this statement #24: FILE: disas/cris.c:2493: + if (sregp == NULL || sregp->name == NULL) [...] total: 3 errors, 0 warnings, 8 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On Mon, Jan 09, 2017 at 07:05:59PM +0000, Peter Maydell wrote: > Coverity (CID 1005689) warns that we don't check that > spec_reg_info() returned non-NULL before dereferencing. > Add the check, though as the comment notes this is > a can't-really-happen case because the earlier constraint > matching should have ruled out the "unknown reg" case. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com> > --- > disas/cris.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/disas/cris.c b/disas/cris.c > index 08161d1..8a1daf9 100644 > --- a/disas/cris.c > +++ b/disas/cris.c > @@ -2490,7 +2490,7 @@ print_with_operands (const struct cris_opcode *opcodep, > const struct cris_spec_reg *sregp > = spec_reg_info ((insn >> 12) & 15, disdata->distype); > > - if (sregp->name == NULL) > + if (sregp == NULL || sregp->name == NULL) > /* Should have been caught as a non-match earlier. */ > *tp++ = '?'; > else > -- > 2.7.4 >
On 9 January 2017 at 19:10, <no-reply@patchew.org> wrote: > Checking PATCH 1/1: disas/cris.c: Fix Coverity warning about unchecked NULL... > ERROR: code indent should never use tabs > #24: FILE: disas/cris.c:2493: > +^Iif (sregp == NULL || sregp->name == NULL)$ > > ERROR: suspect code indent for conditional statements (8, 10) > #24: FILE: disas/cris.c:2493: > + if (sregp == NULL || sregp->name == NULL) > /* Should have been caught as a non-match earlier. */ > > ERROR: braces {} are necessary for all arms of this statement > #24: FILE: disas/cris.c:2493: > + if (sregp == NULL || sregp->name == NULL) > [...] > > total: 3 errors, 0 warnings, 8 lines checked This is because the whole file is GNU coding standards style, being a binutils import. Better to stick with it rather than rework, I think. thanks -- PMM
On Mon, Jan 09, 2017 at 09:35:16PM +0000, Peter Maydell wrote: > On 9 January 2017 at 19:10, <no-reply@patchew.org> wrote: > > Checking PATCH 1/1: disas/cris.c: Fix Coverity warning about unchecked NULL... > > ERROR: code indent should never use tabs > > #24: FILE: disas/cris.c:2493: > > +^Iif (sregp == NULL || sregp->name == NULL)$ > > > > ERROR: suspect code indent for conditional statements (8, 10) > > #24: FILE: disas/cris.c:2493: > > + if (sregp == NULL || sregp->name == NULL) > > /* Should have been caught as a non-match earlier. */ > > > > ERROR: braces {} are necessary for all arms of this statement > > #24: FILE: disas/cris.c:2493: > > + if (sregp == NULL || sregp->name == NULL) > > [...] > > > > total: 3 errors, 0 warnings, 8 lines checked > > This is because the whole file is GNU coding standards > style, being a binutils import. Better to stick with it rather > than rework, I think. Yes, I agree. Cheers, Edgar
09.01.2017 22:05, Peter Maydell wrote: > Coverity (CID 1005689) warns that we don't check that > spec_reg_info() returned non-NULL before dereferencing. > Add the check, though as the comment notes this is > a can't-really-happen case because the earlier constraint > matching should have ruled out the "unknown reg" case. Applied to -trivial, thanks! /mjt
diff --git a/disas/cris.c b/disas/cris.c index 08161d1..8a1daf9 100644 --- a/disas/cris.c +++ b/disas/cris.c @@ -2490,7 +2490,7 @@ print_with_operands (const struct cris_opcode *opcodep, const struct cris_spec_reg *sregp = spec_reg_info ((insn >> 12) & 15, disdata->distype); - if (sregp->name == NULL) + if (sregp == NULL || sregp->name == NULL) /* Should have been caught as a non-match earlier. */ *tp++ = '?'; else
Coverity (CID 1005689) warns that we don't check that spec_reg_info() returned non-NULL before dereferencing. Add the check, though as the comment notes this is a can't-really-happen case because the earlier constraint matching should have ruled out the "unknown reg" case. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- disas/cris.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.7.4