On Wed, 2013-09-25 at 09:28 -0500, Eric Sandeen wrote: > On 9/25/13 2:32 AM, Li Zhong wrote: > > This patch tries to fix CID 997012, 997013 and 997014 reported by Coverity scan, > > as suggested by sekharan. > > > > v2: as Mark pointed out, out in the for loop before also needs list to > > be freed. Also remove out lable as it is not referenced any more. > > Fix itself looks good, thanks! Love to see the scan numbers change > for the better. ;) Thank you for all the education below. :) > Nitpicks, though: Patch changelogs usually goes below the "---" so > the history of trial and error isn't in the commit log. Not that big > a deal, it's just convention as mentioned in the kernel SubmittingPatches > doc: I think I'll practice it with a 3rd verion. > > > The "---" marker line serves the essential purpose of marking for patch > > handling tools where the changelog message ends. > > > > One good use for the additional comments after the "---" marker is for > > a diffstat, to show what files have changed, and the number of > > inserted and deleted lines per file. A diffstat is especially useful > > on bigger patches. Other comments relevant only to the moment or the > > maintainer, not suitable for the permanent changelog, should also go > > here. A good example of such comments might be "patch changelogs" > > which describe what has changed between the v1 and v2 version of the > > patch. > > > And since we're on the topic of commit messages lately, this one could > be improved too I think. > > "CID 997012" won't mean anything to a reader in the future. It'd be > better to describe what you're fixing on its own terms. Something like: > > === > If verify_set_primary_sb() completes the secondary sb scanning loop with > too few valid secondaries found (num_ok < num_sbs / 2), it will immediately > return without freeing any of the previously allocated memory (variables > sb, checked, and any items on the geo list). This was reported by > the Coverity scanner as CID 997012, 997013 and 997014. > > Fix this by using the out_free_list: goto target for this error case. > > Earlier, if get_sb() fails in the secondary scan loop, it goes to > the out: target which does not free any items on the geo list. Fix > this by using the out_free_list: target as well, and remove the now-unused > out: target. > === > > On the one hand, the fix isn't that complicated so it probably speaks for > itself. But it was complicated enough to warrant discussion & V2 on the list, > so probably worth including that detail in the final changelog. OK, I'll use the above as the changelog. > > > Also, in looking at this, I wonder if there's another minor buglet. > > in phase1.c, we turn the return value from verify_set_primary_sb() into > an error string via err_string(rval). This handles the various > error returns such as XR_INSUFF_SEC_SB, XR_EOF, etc. But in the 2nd > case above (get_sb failure), it simply returns "1" which will be interpreted > as XR_BAD_MAGIC ("bad magic number"). > > get_sb() actually returns several XR_* values, so we should probably capture > it and use that return value? That'd be a different patch though. > > I guess the comment for verify_set_primary_sb() would be changed > then too, now it says: > > * returns 1 if bad, 0 if ok > > but today we actually return 0, 1, or XR_INSUFF_SEC_SB. > > Not that big a deal, but it seems like the error returns, their handling, > and associated comments aren't quite consistent. I'll try to make another patch for the above issue. Thanks, Zhong > > Thanks, > -Eric > > > > Signed-off-by: Li Zhong <zhong@xxxxxxxxxxxxxxxxxx> > > --- > > repair/sb.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/repair/sb.c b/repair/sb.c > > index aa550e3..d34d7a2 100644 > > --- a/repair/sb.c > > +++ b/repair/sb.c > > @@ -733,7 +733,7 @@ verify_set_primary_sb(xfs_sb_t *rsb, > > > > if (get_sb(sb, off, size, agno) == XR_EOF) { > > retval = 1; > > - goto out; > > + goto out_free_list; > > } > > > > if (verify_sb(sb, 0) == XR_OK) { > > @@ -756,8 +756,10 @@ verify_set_primary_sb(xfs_sb_t *rsb, > > /* > > * see if we have enough superblocks to bother with > > */ > > - if (num_ok < num_sbs / 2) > > - return(XR_INSUFF_SEC_SB); > > + if (num_ok < num_sbs / 2) { > > + retval = XR_INSUFF_SEC_SB; > > + goto out_free_list; > > + } > > > > current = get_best_geo(list); > > > > @@ -841,7 +843,6 @@ verify_set_primary_sb(xfs_sb_t *rsb, > > > > out_free_list: > > free_geo(list); > > -out: > > free(sb); > > free(checked); > > return(retval); > > > _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs