Bug 191953 - [vfs] [patch] better KASSERT msg in _vn_lock()
Summary: [vfs] [patch] better KASSERT msg in _vn_lock()
Status: Closed Works As Intended
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-07-18 14:45 UTC by luke.tw
Modified: 2016-08-02 07:00 UTC (History)
3 users (show)

See Also:


Attachments
patch for _vn_lock() (612 bytes, patch)
2014-07-18 14:46 UTC, luke.tw
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description luke.tw 2014-07-18 14:45:20 UTC
According to the expression, the message should be an 'and' relation.
Comment 1 luke.tw 2014-07-18 14:46:13 UTC
Created attachment 144779 [details]
patch for _vn_lock()
Comment 2 Hiren Panchasara freebsd_committer 2014-07-20 22:49:33 UTC
Thanks for your submission. 

I am assuming that the comment is correct but the KASSERT itself is wrong. We probably want to fail when either of the events fail. 

I am proposing following change to KASSERT. While here, fixing 80-column wrapping.

-               KASSERT((flags & LK_RETRY) == 0 || error == 0,
-                   ("LK_RETRY set with incompatible flags (0x%x) or an error occured (%d)",
-                   flags, error));
+               KASSERT((flags & LK_RETRY) == 0 && error == 0,
+                   ("LK_RETRY set with incompatible flags (0x%x) or an error "
+                   "occured (%d)", flags, error));
Comment 3 Mark Johnston freebsd_committer 2015-07-17 22:38:50 UTC
I believe the current assertion message is correct. An error from vop_stdlock indicates that the lock operation could not be completed, for example because LK_NOWAIT was set in the flags, or the caller specified an interruptible sleep and a signal was received. In this case, the use of LK_RETRY is incompatible with other flags.

If the FS provides its own vop_lock implementation, the assertion says that it must not return an error if LK_RETRY is set.
Comment 4 Conrad Meyer freebsd_committer 2016-08-01 23:50:50 UTC
Should this be closed as Invalid?
Comment 5 Andriy Gapon freebsd_committer 2016-08-02 07:00:24 UTC
It seems that we could benefit from having a macro like KASSERT_IMPLY which would assert A => B, that is, if A is true, then B must also be true (and B can be anything if A is false).  In FreeBSD kernel this gets written as KASSERT(!A || B, ...) and sometimes there is a confusion when mapping back the code to the logical statement.