Page MenuHomeFreeBSD

queue: New debug macros for STAILQ
Needs ReviewPublic

Authored by olce on Oct 3 2024, 5:18 PM.
Tags
None
Referenced Files
F102101631: D46889.diff
Thu, Nov 7, 3:24 PM
Unknown Object (File)
Mon, Oct 28, 9:32 AM
Unknown Object (File)
Tue, Oct 22, 4:45 PM
Unknown Object (File)
Fri, Oct 18, 10:21 AM
Unknown Object (File)
Thu, Oct 10, 11:46 PM
Unknown Object (File)
Wed, Oct 9, 12:55 PM
Unknown Object (File)
Oct 7 2024, 5:08 AM
Unknown Object (File)
Oct 5 2024, 1:38 PM
Subscribers

Details

Summary

The new STAILQ_ASSERT_EMPTY() macro allows callers to assert that some
STAILQ is empty. It leverages the new QMD_STAILQ_CHECK_EMPTY() internal
macro.

QMD_STAILQ_CHECK_EMPTY() is a check for empty STAILQ, where heads's
'stqh_last' field must point to the 'stqh_first' one. Use it in
STAILQ_ASSERT_EMPTY().

QMD_STAILQ_CHECK_TAIL() checks that the tail pointed by 'head' does not
have a next element. It is similar to the already existing
QMD_TAILQ_CHECK_TAIL(), but without the superfluous 'field' argument and
clearer documentation. Use it in STAILQ_INSERT_TAIL().

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60329
Build 57213: arc lint + arc unit

Event Timeline

olce requested review of this revision.Oct 3 2024, 5:18 PM
sys/sys/queue.h
349

This doesn't seem to be used anywhere? Other QMD_*_CHECK macros are internal macros used in queue.h directly. Is this intended to be used in consumers, or is the patch incomplete?

sys/sys/queue.h
349

Sorry, I should have provided more context, and probably not have submitted this as is.

Initially, I intended QMD_STAILQ_CHECK_EMPTY() to be called by STAILQ_EMPTY() (only in the case the STAILQ is indeed determined empty), but (too) quickly inferred that this didn't seem possible without using statement expressions, which I suspected were not allowed in sys/queue.h.

Then, I began using it directly in some consumer (which is not in this series) but didn't change the name.

Ideally, I would like this check to be performed by STAILQ_EMPTY(). So, is it possible to use statement expressions in sys/queue.h? If not, would you see an alternative to make it possible? One way may be to express the check in terms of the ternary operator by wrapping panic() into an inline function returning a boolean, which should work but is not particularly elegant...

QMD_STAILQ_CHECK_EMPTY() should also be called from STAILQ_INIT(), which is easy to do as the latter already expands to a statement, and I intend to add that after your feedback to this message.

Regardless, I've experienced it is also useful to call QMD_STAILQ_CHECK_EMPTY() directly at points where the callers know that the list must be empty, which are not necessarily initializations or explicit empty tests. Re-thinking about that, I'm now finding that a better solution would be to create something like a STAILQ_ASSERT_EMPTY() macro and have consumers call it instead. This macro would call STAILQ_EMPTY() and panic on it returning false. As already evoked above, STAILQ_EMPTY() would call QMD_STAILQ_CHECK_EMPTY() to perform the additional integrity check when the list is empty. This would also have the benefit that QMD_*_CHECK() macros would continue to be called by sys/queue.h exclusively.

Does that sound like an acceptable plan?

olce edited the summary of this revision. (Show Details)
  • Introduce STAILQ_ASSERT_EMPTY() macro.
  • Use statement expression for STAILQ_EMPTY(), to have it call QMD_STAILQ_CHECK_EMPTY().
  • Revise comments and panic messages to be clearer.
olce marked an inline comment as done.Fri, Nov 1, 2:12 PM
sys/sys/queue.h
349

The new version illustrates what I'd like to do. In the meantime, reading sys/cdefs.h, I saw some statement expression, so I was probably too conservative in assuming it can't be used in sys/queue.h.

355–359

As QMD_STAILQ_CHECK_TAIL() is internal, I've removed its unused field parameter, making it diverge from its QMD_TAILQ_CHECK_TAIL() model.

sys/sys/queue.h
349

If by statement expressions you mean the '({ ..., a; })' GNU C extension, yes we use that a lot. It's true that <sys/queue.h> is widely used outside of the base system, but so is <sys/cdefs.h>.