| Message ID | 20180621123525.5365-1-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 21, 2018 at 02:35:25PM +0200, Carlos Maiolino wrote: > Initialize a few variables before pass them by reference in other > functions. > > This quiets the following Coverity reports: > > CID 100898 > CID 1437081 > CID 1437129 > CID 1437191 > CID 1437201 > CID 1437212 > CID 1437341 These all have to do with the xfs_warn() at the bottom of xfs_alloc_get_rec() referencing *bno before we set it but after we've decided that the record is bad. Why not just fix by changing it to: xfs_warn(mp, "start block 0x%x block count 0x%x", rec->alloc.ar_startblock, rec->alloc.ar_blockcount); --D > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > --- > > There is another coverity issue (CID 1437358), which actually looks more > important, which really looks to pass an uninitialized value to > xfs_getfsmap_rtdev_rtbitmap_helper(), where such looks to expect the variable to > be already initialized. I'm familiar with RT code, so looking into it yet, but > I think this one deservers a separated patch if that can really trigger a bug. > > fs/xfs/libxfs/xfs_alloc.c | 14 +++++++------- > fs/xfs/scrub/agheader.c | 4 ++-- > fs/xfs/scrub/alloc.c | 2 +- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index eef466260d43..ffdd50f5af32 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -421,8 +421,8 @@ xfs_alloc_fixup_trees( > { > int error; /* error code */ > int i; /* operation results */ > - xfs_agblock_t nfbno1; /* first new free startblock */ > - xfs_agblock_t nfbno2; /* second new free startblock */ > + xfs_agblock_t nfbno1=0; /* first new free startblock */ > + xfs_agblock_t nfbno2=0; /* second new free startblock */ > xfs_extlen_t nflen1=0; /* first new free length */ > xfs_extlen_t nflen2=0; /* second new free length */ > struct xfs_mount *mp; > @@ -783,8 +783,8 @@ xfs_alloc_ag_vextent_exact( > xfs_btree_cur_t *bno_cur;/* by block-number btree cursor */ > xfs_btree_cur_t *cnt_cur;/* by count btree cursor */ > int error; > - xfs_agblock_t fbno; /* start block of found extent */ > - xfs_extlen_t flen; /* length of found extent */ > + xfs_agblock_t fbno=0; /* start block of found extent */ > + xfs_extlen_t flen=0; /* length of found extent */ > xfs_agblock_t tbno; /* start block of busy extent */ > xfs_extlen_t tlen; /* length of busy extent */ > xfs_agblock_t tend; /* end block of busy extent */ > @@ -1597,7 +1597,7 @@ xfs_alloc_ag_vextent_small( > int error; > xfs_agblock_t fbno; > xfs_extlen_t flen; > - int i; > + int i = 0; > > if ((error = xfs_btree_decrement(ccur, 0, &i))) > goto error0; > @@ -1704,8 +1704,8 @@ xfs_free_ag_extent( > xfs_btree_cur_t *bno_cur; /* cursor for by-block btree */ > xfs_btree_cur_t *cnt_cur; /* cursor for by-size btree */ > int error; /* error return value */ > - xfs_agblock_t gtbno; /* start of right neighbor block */ > - xfs_extlen_t gtlen; /* length of right neighbor block */ > + xfs_agblock_t gtbno=0; /* start of right neighbor block */ > + xfs_extlen_t gtlen=0; /* length of right neighbor block */ > int haveleft; /* have a left neighbor block */ > int haveright; /* have a right neighbor block */ > int i; /* temp, result code */ > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c > index 9bb0745f1ad2..78a7381d6ca0 100644 > --- a/fs/xfs/scrub/agheader.c > +++ b/fs/xfs/scrub/agheader.c > @@ -388,8 +388,8 @@ xfs_scrub_agf_xref_cntbt( > struct xfs_scrub_context *sc) > { > struct xfs_agf *agf = XFS_BUF_TO_AGF(sc->sa.agf_bp); > - xfs_agblock_t agbno; > - xfs_extlen_t blocks; > + xfs_agblock_t agbno = 0; > + xfs_extlen_t blocks = 0; > int have; > int error; > > diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c > index 50e4f7fa06f0..c82347da400f 100644 > --- a/fs/xfs/scrub/alloc.c > +++ b/fs/xfs/scrub/alloc.c > @@ -47,7 +47,7 @@ xfs_scrub_allocbt_xref_other( > xfs_extlen_t len) > { > struct xfs_btree_cur **pcur; > - xfs_agblock_t fbno; > + xfs_agblock_t fbno = 0; > xfs_extlen_t flen; > int has_otherrec; > int error; > -- > 2.14.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 6/21/18 7:35 AM, Carlos Maiolino wrote: > Initialize a few variables before pass them by reference in other > functions. > > This quiets the following Coverity reports: > > CID 100898 > CID 1437081 > CID 1437129 > CID 1437191 > CID 1437201 > CID 1437212 > CID 1437341 > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> thanks for looking into those, always good to reduce the count so stuff doesn't get lost in the noise. > --- > > There is another coverity issue (CID 1437358), which actually looks more > important, which really looks to pass an uninitialized value to > xfs_getfsmap_rtdev_rtbitmap_helper(), where such looks to expect the variable to > be already initialized. I'm familiar with RT code, so looking into it yet, but > I think this one deservers a separated patch if that can really trigger a bug. > > fs/xfs/libxfs/xfs_alloc.c | 14 +++++++------- > fs/xfs/scrub/agheader.c | 4 ++-- > fs/xfs/scrub/alloc.c | 2 +- > 3 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > index eef466260d43..ffdd50f5af32 100644 > --- a/fs/xfs/libxfs/xfs_alloc.c > +++ b/fs/xfs/libxfs/xfs_alloc.c > @@ -421,8 +421,8 @@ xfs_alloc_fixup_trees( > { > int error; /* error code */ > int i; /* operation results */ > - xfs_agblock_t nfbno1; /* first new free startblock */ > - xfs_agblock_t nfbno2; /* second new free startblock */ > + xfs_agblock_t nfbno1=0; /* first new free startblock */ > + xfs_agblock_t nfbno2=0; /* second new free startblock */ So, these really do look like false positives. The function called does: error = xfs_btree_get_rec(cur, &rec, stat); if (!error && *stat == 1) { *bno = be32_to_cpu(rec->alloc.ar_startblock); *len = be32_to_cpu(rec->alloc.ar_blockcount); } return error; so *bno is unset if error || *stat == 2, but the caller returns early on error, and does not test nfbno1 on *stat == 2: if ((error = xfs_alloc_get_rec(cnt_cur, &nfbno1, &nflen1, &i))) return error; XFS_WANT_CORRUPTED_RETURN(mp, i == 1 && nfbno1 == fbno && nflen1 == flen); so it really is just confusion on coverity's part I think. As a general note, it's also OK to simply mark issues as false positives in coverity, if you're sure that they are, and leave the code alone. But sometimes if coverity can get confused the reader can too, and a patch like this can make it more understandable. OTOH sometimes unnecessary initializations can make things more confusing ;) In this case, meh, I think the initialization is ok. > xfs_extlen_t nflen1=0; /* first new free length */ > xfs_extlen_t nflen2=0; /* second new free length */ > struct xfs_mount *mp; > @@ -783,8 +783,8 @@ xfs_alloc_ag_vextent_exact( > xfs_btree_cur_t *bno_cur;/* by block-number btree cursor */ > xfs_btree_cur_t *cnt_cur;/* by count btree cursor */ > int error; > - xfs_agblock_t fbno; /* start block of found extent */ > - xfs_extlen_t flen; /* length of found extent */ > + xfs_agblock_t fbno=0; /* start block of found extent */ > + xfs_extlen_t flen=0; /* length of found extent */ same thing here. error = xfs_alloc_get_rec(bno_cur, &fbno, &flen, &i); if (error) goto error0; XFS_WANT_CORRUPTED_GOTO(args->mp, i == 1, error0); so if it returns an error or if i != 1, fbno & flen are untested. > xfs_agblock_t tbno; /* start block of busy extent */ > xfs_extlen_t tlen; /* length of busy extent */ > xfs_agblock_t tend; /* end block of busy extent */ > @@ -1597,7 +1597,7 @@ xfs_alloc_ag_vextent_small( > int error; > xfs_agblock_t fbno; > xfs_extlen_t flen; > - int i; > + int i = 0; > > if ((error = xfs_btree_decrement(ccur, 0, &i))) > goto error0; if (i) { if ((error = xfs_alloc_get_rec(ccur, &fbno, &flen, &i))) goto error0; XFS_WANT_CORRUPTED_GOTO(args->mp, i == 1, error0); } Same thing here, I think, i will be unset, but won't be tested if error is true. > @@ -1704,8 +1704,8 @@ xfs_free_ag_extent( > xfs_btree_cur_t *bno_cur; /* cursor for by-block btree */ > xfs_btree_cur_t *cnt_cur; /* cursor for by-size btree */ > int error; /* error return value */ > - xfs_agblock_t gtbno; /* start of right neighbor block */ > - xfs_extlen_t gtlen; /* length of right neighbor block */ > + xfs_agblock_t gtbno=0; /* start of right neighbor block */ > + xfs_extlen_t gtlen=0; /* length of right neighbor block */ > int haveleft; /* have a left neighbor block */ > int haveright; /* have a right neighbor block */ > int i; /* temp, result code */ ok same pattern here I think > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c > index 9bb0745f1ad2..78a7381d6ca0 100644 > --- a/fs/xfs/scrub/agheader.c > +++ b/fs/xfs/scrub/agheader.c > @@ -388,8 +388,8 @@ xfs_scrub_agf_xref_cntbt( > struct xfs_scrub_context *sc) > { > struct xfs_agf *agf = XFS_BUF_TO_AGF(sc->sa.agf_bp); > - xfs_agblock_t agbno; > - xfs_extlen_t blocks; > + xfs_agblock_t agbno = 0; > + xfs_extlen_t blocks = 0; > int have; > int error; this one seems a little more confusing but same basic idea. agbno is completely unused, blocks won't be tested if "have" (the *stat return) is zero indicating a failure so again seems like a false positive. > diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c > index 50e4f7fa06f0..c82347da400f 100644 > --- a/fs/xfs/scrub/alloc.c > +++ b/fs/xfs/scrub/alloc.c > @@ -47,7 +47,7 @@ xfs_scrub_allocbt_xref_other( > xfs_extlen_t len) > { > struct xfs_btree_cur **pcur; > - xfs_agblock_t fbno; > + xfs_agblock_t fbno = 0; > xfs_extlen_t flen; > int has_otherrec; > int error; > ok same. None of this looks incorrect, but I'm always on the fence about code changes to fix false positives in checkers. Let's see what Darrick thinks. :) -Eric -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, > > CID 100898 > > CID 1437081 > > CID 1437129 > > CID 1437191 > > CID 1437201 > > CID 1437212 > > CID 1437341 > > These all have to do with the xfs_warn() at the bottom of > xfs_alloc_get_rec() referencing *bno before we set it but after we've > decided that the record is bad. Why not just fix by changing it to: > > xfs_warn(mp, "start block 0x%x block count 0x%x", > rec->alloc.ar_startblock, rec->alloc.ar_blockcount); > I do apologize to take so long to reply to this thread, couldn't come back to it until now. Thanks for pointing it to xfs_warn() I had the same impression that Eric described (or similar), that coverity was complaining we passed it into xfs_alloc_get_rec() uninitialized, and generating false positives. After reading your reply I went to check the reports in coverity system and found out all of them pointing to xfs_warn... Well, next time I'll make sure to check the reports on their system before submitting any patch :P In the mean time I'll re-do the patch as you suggested, although, the values are in BigEndian, so I believe they should be converted before printed. Well, I'll re-send the patch and we discuss it there. Thanks again > --D > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> > > --- > > > > There is another coverity issue (CID 1437358), which actually looks more > > important, which really looks to pass an uninitialized value to > > xfs_getfsmap_rtdev_rtbitmap_helper(), where such looks to expect the variable to > > be already initialized. I'm familiar with RT code, so looking into it yet, but > > I think this one deservers a separated patch if that can really trigger a bug. > > > > fs/xfs/libxfs/xfs_alloc.c | 14 +++++++------- > > fs/xfs/scrub/agheader.c | 4 ++-- > > fs/xfs/scrub/alloc.c | 2 +- > > 3 files changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c > > index eef466260d43..ffdd50f5af32 100644 > > --- a/fs/xfs/libxfs/xfs_alloc.c > > +++ b/fs/xfs/libxfs/xfs_alloc.c > > @@ -421,8 +421,8 @@ xfs_alloc_fixup_trees( > > { > > int error; /* error code */ > > int i; /* operation results */ > > - xfs_agblock_t nfbno1; /* first new free startblock */ > > - xfs_agblock_t nfbno2; /* second new free startblock */ > > + xfs_agblock_t nfbno1=0; /* first new free startblock */ > > + xfs_agblock_t nfbno2=0; /* second new free startblock */ > > xfs_extlen_t nflen1=0; /* first new free length */ > > xfs_extlen_t nflen2=0; /* second new free length */ > > struct xfs_mount *mp; > > @@ -783,8 +783,8 @@ xfs_alloc_ag_vextent_exact( > > xfs_btree_cur_t *bno_cur;/* by block-number btree cursor */ > > xfs_btree_cur_t *cnt_cur;/* by count btree cursor */ > > int error; > > - xfs_agblock_t fbno; /* start block of found extent */ > > - xfs_extlen_t flen; /* length of found extent */ > > + xfs_agblock_t fbno=0; /* start block of found extent */ > > + xfs_extlen_t flen=0; /* length of found extent */ > > xfs_agblock_t tbno; /* start block of busy extent */ > > xfs_extlen_t tlen; /* length of busy extent */ > > xfs_agblock_t tend; /* end block of busy extent */ > > @@ -1597,7 +1597,7 @@ xfs_alloc_ag_vextent_small( > > int error; > > xfs_agblock_t fbno; > > xfs_extlen_t flen; > > - int i; > > + int i = 0; > > > > if ((error = xfs_btree_decrement(ccur, 0, &i))) > > goto error0; > > @@ -1704,8 +1704,8 @@ xfs_free_ag_extent( > > xfs_btree_cur_t *bno_cur; /* cursor for by-block btree */ > > xfs_btree_cur_t *cnt_cur; /* cursor for by-size btree */ > > int error; /* error return value */ > > - xfs_agblock_t gtbno; /* start of right neighbor block */ > > - xfs_extlen_t gtlen; /* length of right neighbor block */ > > + xfs_agblock_t gtbno=0; /* start of right neighbor block */ > > + xfs_extlen_t gtlen=0; /* length of right neighbor block */ > > int haveleft; /* have a left neighbor block */ > > int haveright; /* have a right neighbor block */ > > int i; /* temp, result code */ > > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c > > index 9bb0745f1ad2..78a7381d6ca0 100644 > > --- a/fs/xfs/scrub/agheader.c > > +++ b/fs/xfs/scrub/agheader.c > > @@ -388,8 +388,8 @@ xfs_scrub_agf_xref_cntbt( > > struct xfs_scrub_context *sc) > > { > > struct xfs_agf *agf = XFS_BUF_TO_AGF(sc->sa.agf_bp); > > - xfs_agblock_t agbno; > > - xfs_extlen_t blocks; > > + xfs_agblock_t agbno = 0; > > + xfs_extlen_t blocks = 0; > > int have; > > int error; > > > > diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c > > index 50e4f7fa06f0..c82347da400f 100644 > > --- a/fs/xfs/scrub/alloc.c > > +++ b/fs/xfs/scrub/alloc.c > > @@ -47,7 +47,7 @@ xfs_scrub_allocbt_xref_other( > > xfs_extlen_t len) > > { > > struct xfs_btree_cur **pcur; > > - xfs_agblock_t fbno; > > + xfs_agblock_t fbno = 0; > > xfs_extlen_t flen; > > int has_otherrec; > > int error; > > -- > > 2.14.3 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c index eef466260d43..ffdd50f5af32 100644 --- a/fs/xfs/libxfs/xfs_alloc.c +++ b/fs/xfs/libxfs/xfs_alloc.c @@ -421,8 +421,8 @@ xfs_alloc_fixup_trees( { int error; /* error code */ int i; /* operation results */ - xfs_agblock_t nfbno1; /* first new free startblock */ - xfs_agblock_t nfbno2; /* second new free startblock */ + xfs_agblock_t nfbno1=0; /* first new free startblock */ + xfs_agblock_t nfbno2=0; /* second new free startblock */ xfs_extlen_t nflen1=0; /* first new free length */ xfs_extlen_t nflen2=0; /* second new free length */ struct xfs_mount *mp; @@ -783,8 +783,8 @@ xfs_alloc_ag_vextent_exact( xfs_btree_cur_t *bno_cur;/* by block-number btree cursor */ xfs_btree_cur_t *cnt_cur;/* by count btree cursor */ int error; - xfs_agblock_t fbno; /* start block of found extent */ - xfs_extlen_t flen; /* length of found extent */ + xfs_agblock_t fbno=0; /* start block of found extent */ + xfs_extlen_t flen=0; /* length of found extent */ xfs_agblock_t tbno; /* start block of busy extent */ xfs_extlen_t tlen; /* length of busy extent */ xfs_agblock_t tend; /* end block of busy extent */ @@ -1597,7 +1597,7 @@ xfs_alloc_ag_vextent_small( int error; xfs_agblock_t fbno; xfs_extlen_t flen; - int i; + int i = 0; if ((error = xfs_btree_decrement(ccur, 0, &i))) goto error0; @@ -1704,8 +1704,8 @@ xfs_free_ag_extent( xfs_btree_cur_t *bno_cur; /* cursor for by-block btree */ xfs_btree_cur_t *cnt_cur; /* cursor for by-size btree */ int error; /* error return value */ - xfs_agblock_t gtbno; /* start of right neighbor block */ - xfs_extlen_t gtlen; /* length of right neighbor block */ + xfs_agblock_t gtbno=0; /* start of right neighbor block */ + xfs_extlen_t gtlen=0; /* length of right neighbor block */ int haveleft; /* have a left neighbor block */ int haveright; /* have a right neighbor block */ int i; /* temp, result code */ diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c index 9bb0745f1ad2..78a7381d6ca0 100644 --- a/fs/xfs/scrub/agheader.c +++ b/fs/xfs/scrub/agheader.c @@ -388,8 +388,8 @@ xfs_scrub_agf_xref_cntbt( struct xfs_scrub_context *sc) { struct xfs_agf *agf = XFS_BUF_TO_AGF(sc->sa.agf_bp); - xfs_agblock_t agbno; - xfs_extlen_t blocks; + xfs_agblock_t agbno = 0; + xfs_extlen_t blocks = 0; int have; int error; diff --git a/fs/xfs/scrub/alloc.c b/fs/xfs/scrub/alloc.c index 50e4f7fa06f0..c82347da400f 100644 --- a/fs/xfs/scrub/alloc.c +++ b/fs/xfs/scrub/alloc.c @@ -47,7 +47,7 @@ xfs_scrub_allocbt_xref_other( xfs_extlen_t len) { struct xfs_btree_cur **pcur; - xfs_agblock_t fbno; + xfs_agblock_t fbno = 0; xfs_extlen_t flen; int has_otherrec; int error;
Initialize a few variables before pass them by reference in other functions. This quiets the following Coverity reports: CID 100898 CID 1437081 CID 1437129 CID 1437191 CID 1437201 CID 1437212 CID 1437341 Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com> --- There is another coverity issue (CID 1437358), which actually looks more important, which really looks to pass an uninitialized value to xfs_getfsmap_rtdev_rtbitmap_helper(), where such looks to expect the variable to be already initialized. I'm familiar with RT code, so looking into it yet, but I think this one deservers a separated patch if that can really trigger a bug. fs/xfs/libxfs/xfs_alloc.c | 14 +++++++------- fs/xfs/scrub/agheader.c | 4 ++-- fs/xfs/scrub/alloc.c | 2 +- 3 files changed, 10 insertions(+), 10 deletions(-)