Bug 191953

Summary:

[vfs] [patch] better KASSERT msg in _vn_lock()

Product:

Base System

Reporter:

luke.tw

Component:

kern

Assignee:

freebsd-bugs (Nobody) <bugs>

Status:

Closed Works As Intended

Severity:

Affects Only Me

CC:

cem, hiren, markj

Priority:

---

Keywords:

patch

Version:

CURRENT

Hardware:

Any

OS:

Any

Attachments:

Description	Flags
patch for _vn_lock()	none

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.