On Feb 13, 2016, at 11:33 PM, green@xxxxxxxxxxxxxx wrote: > I was long annoyed by this and so using my lustre tree as benchmark > here is the difference in output before and after the patch for this file > for wrong line numbers: > http://git.whamcloud.com/fs/lustre-release.git/blob/943612ac470a1f916d44f551eee01c3d51fc9546:/lustre/lfsck/lfsck_layout.c > > -lustre/lfsck/lfsck_layout.c:2390 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2386) > -lustre/lfsck/lfsck_layout.c:2415 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2411) > +lustre/lfsck/lfsck_layout.c:2390 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2242) > +lustre/lfsck/lfsck_layout.c:2415 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2242) > > Granted, this still remains a false positive, but thta would need a more > involved fix in this particular check module. so far the biggest class of issues here is when we allocate something, but check for errptr, and there exists another path that does not allocate anything and exit code path checks for NULL to see if the allocation needs to be unwinded (transaction stopped and such). I do not think assuming IS_ERR is also NULL pointer check is such a great idea since that might fail to find some bugs where we can get NULL or err ptr? Kind of like: ptr *handle = NULL; if (something) { handle = transaction_start(blah); if (IS_ERR(handle)) goto err; do_some_stuff(handle); … } if (handle) <== false positive hits here. transaction_stop(handle); err: ... > And here's the false positive that is now disappeared for this file: > http://git.whamcloud.com/fs/lustre-release.git/blob/943612ac470a1f916d44f551eee01c3d51fc9546:/lustre/llite/llite_lib.c > i > lustre/llite/llite_lib.c:1709 ll_setattr_raw() warn: 'mutex:&inode->i_mutex' is sometimes locked here and sometimes unlocked. > > smatch_slist.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/smatch_slist.c b/smatch_slist.c > index 55becc9..f6e60ac 100644 > --- a/smatch_slist.c > +++ b/smatch_slist.c > @@ -374,7 +374,14 @@ struct sm_state *merge_sm_states(struct sm_state *one, struct sm_state *two) > if (one == two) > return one; > s = merge_states(one->owner, one->name, one->sym, one->state, two->state); > - result = alloc_state_no_name(one->owner, one->name, one->sym, s); > + > + if (s == one->state) > + result = clone_sm(one); > + else if (s == two->state) // What about custom merge? Ah, this reminds me - the merging code also has a "custom merge" option. Should it decide to return either state1 or state2 - should we inherit it back there, or are there cases where we want that state to only start from current line and not from the original state-originating line? And finally - this change does not appear to break anything, but there's a number of failures that I see when doing "make check" even without the patch, all of them supposedly not being known failures, I am not sure what to do about that. Bye, Oleg-- To unsubscribe from this list: send the line "unsubscribe smatch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
