Powered by Linux
Re: Recent wave of additional false negative warnings? — Semantic Matching Tool

Re: Recent wave of additional false negative warnings?

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

The code looks like:

   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);
   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;
   655                  if (ma->ma_attr.la_valid != 0)
   656                          GOTO(out_put, rc = -EPROTO);
   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;
   668                  if (ma->ma_attr.la_valid != 0)

This condition is known already.  No need to check again.

   669                          GOTO(out_put, rc = -EPROTO);

>    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?

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

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux