On Wed, Aug 19, 2015 at 09:26:30PM -0400, Oleg Drokin wrote:
> Hello!
>
> I pulled in the smatch update today (I was at 73de935ee75c55e09be67a995cd9c8a88aea5451 before) and
> immediately I got a lot of extra warnings where random integer comparisons were marked as e.g.:
> lustre/mdt/mdt_reint.c:668 mdt_reint_setattr() warn: we tested 'ma->ma_attr.la_valid' before and it was 'false'
> if ((ma->ma_attr.la_valid & LA_SIZE) ||
> ...
> } else if ((ma->ma_valid & MA_LMV) && (ma->ma_valid & MA_INODE)) {
> struct lu_buf *buf = &info->mti_buf;
> struct mdt_lock_handle *lh;
>
> 668 if (ma->ma_attr.la_valid != 0)
>
>
> Well, that's not really a boolean, could it be smart enough that when we use bitwise logical operands on the value,
> or compare it to stuff other than 0 and 1, the variable becomes "not boolean" and does not generate those warnings
> unless we compare with the exact same value (and then that value is printed)?
It's not really a false positive, it's just that the warning is sucky
and perhaps not very useful.
It's not looking at boolean variables, it's looking at "simplified"
conditions, and saying if they are known to be true or false. Normally
checks deal with only simplified conditions. So instead of seeing:
if (!!foo || (baz & FLAG) == 0) {
The checks only see "foo" and "baz & FLAG". The plumbing of negation,
zero compares and logical OR and AND are removed and handled
automatically.
The code looks like:
lustre/mdt/mdt_reint.c
645 if ((ma->ma_valid & MA_INODE) && ma->ma_attr.la_valid) {
If MA_INODE is set and la_valid is true we go down this patch.
646 if (ma->ma_valid & MA_LOV)
647 GOTO(out_put, rc = -EPROTO);
648
649 rc = mdt_attr_set(info, mo, ma);
650 if (rc)
651 GOTO(out_put, rc);
652 } else if ((ma->ma_valid & MA_LOV) && (ma->ma_valid & MA_INODE)) {
653 struct lu_buf *buf = &info->mti_buf;
654
655 if (ma->ma_attr.la_valid != 0)
656 GOTO(out_put, rc = -EPROTO);
657
658 buf->lb_buf = ma->ma_lmm;
659 buf->lb_len = ma->ma_lmm_size;
660 rc = mo_xattr_set(info->mti_env, mdt_object_child(mo),
661 buf, XATTR_NAME_LOV, 0);
662 if (rc)
663 GOTO(out_put, rc);
664 } else if ((ma->ma_valid & MA_LMV) && (ma->ma_valid & MA_INODE)) {
Since we see that MA_INODE is set, it means that la_valid is zero.
665 struct lu_buf *buf = &info->mti_buf;
666 struct mdt_lock_handle *lh;
667
668 if (ma->ma_attr.la_valid != 0)
This condition is known already. No need to check again.
669 GOTO(out_put, rc = -EPROTO);
670
>
> I guess ability to look at struct members now also enables another class of false positives best seen as:
> if (lqe->lqe_qunit == 0) {
> /* lqe was read from disk, but neither qunit, nor edquot flag
> * were initialized */
> qmt_adjust_qunit(env, lqe);
> if (lqe->lqe_qunit != 0)
> qmt_adjust_edquot(lqe, cfs_time_current_sec());
>
> Which now generates:
> lustre/quota/qmt_entry.c:695 qmt_revalidate() warn: we tested 'lqe->lqe_qunit' before and it was 'false'
>
> I guess it has no knowledge that lqe might be modified in that call and I'd just have to live with this one (to add
> insult here, the field is not a boolean thing anyway and holds size).
>
It should have seen the modify. What is the output from:
`~/path/to/smatch/smatch_data/db/smdb.py return_states qmt_adjust_qunit`
I will look at ways to silence this if the cross function db is not
set up.
>
> Also a couple of really bizzare warnings that I have no explanation for:
> lustre/lfsck/lfsck_layout.c:2388 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2384)
> lustre/lfsck/lfsck_layout.c:2413 lfsck_layout_recreate_lovea() warn: variable dereferenced before check 'handle' (see line 2409)
>
> The code here is (full file: http://git.whamcloud.com/fs/lustre-release.git/blob/refs/heads/master:/lustre/lfsck/lfsck_layout.c):
> 2409 if (bk->lb_param & LPF_DRYRUN)
> GOTO(unlock_parent, rc = 1);
>
> dt_write_unlock(env, parent);
> 2413 if (handle != NULL)
> dt_trans_stop(env, dt, handle);
>
> huh?!
Yeah. The line numbers are off... We originally dereference "handle"
on line 2236, but only if the LPF_DRYRUN flag is not set. The line 2409
that is mentioned is the test to say that the the flag is not set.
I think the warning seems sort of correct (the NULL check on line 2413
could be removed). But it still shouldn't be printing this warning, I
think, because dt_trans_create() doesn't return NULL. What does the
output of
`~/path/to/smatch/smatch_data/db/smdb.py return_states dt_trans_create`
look like?
regards,
dan carpenter
--
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
