[2/3] cleanup: Introduce cond_no_free_ptr()

Commit Message

Dan Williams Feb. 27, 2024, 4:48 p.m. UTC

The no_free_ptr() helper cancels automatic cleanup for cases where
assigning the pointer transfers ownership for freeing it. However, it
gets awkward to use when multiple allocations need to be cancelled in
response to one registration call. For example:

    1/ name = kasprintf(...);
    2/ res = kmalloc(...);
    3/ res->name = name;
    4/ rc = insert_resource(..., res);
    5/ if (rc) return rc;

no_free_ptr() cannot be used for 3 since insert_resource() does not
cleanup on failure. no_free_ptr() could be used at 4, but if
insert_resource() fails, the no_free_ptr() was premature. After 5 is
when it is known that it is safe to free @res and @name. However,
no_free_ptr() is awkward there as well because of __must_check().

The options are:
 * Just open code @res = NULL and @name = NULL, but that is a
   non-idiomatic way to use the cleanup helpers.
 * Introduce a no_free_ptr() variant that drops the __must_check, but
   that defeats the purpose of mandating that the caller understands
   that responsibility for freeing has been handed off.
 * Introduce a new helper that combines a condition check to supersede
   the __must_check of no_free_ptr()

So, per that last option, line 5/ from the example becomes:

    5/ cond_no_free_ptr(rc == 0, return rc, res, name);

...and that handles calling no_free_ptr() while also mandating the
negative condition be handled. It is inspired by scoped_cond_guard()
which also takes a statement for the negative condition case.

Cc: Peter Zijlstra <[e-mail]>
Cc: Jonathan Cameron <[e-mail]>
Signed-off-by: Dan Williams <[e-mail]>
---
 include/linux/cleanup.h |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Linus Torvalds Feb. 27, 2024, 8:40 p.m. UTC | #1

On Tue, 27 Feb 2024 at 08:49, Dan Williams <[e-mail]> wrote:
>
>     5/ cond_no_free_ptr(rc == 0, return rc, res, name);

Ugh. Honestly, this is all too ugly for words.

The whole - and only - point for the cond_guard() is to make mistakes
less likely.

This is not it. This makes mistakes unreadable and undebuggable.

             Linus

Patch

diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index 602afb85da34..a6d593a60611 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -77,6 +77,28 @@  const volatile void * __must_check_fn(const volatile void *val)
 
 #define return_ptr(p)	return no_free_ptr(p)
 
+#define __cond_no_free_ptrs(p) ({__auto_type __always_unused __ptr = no_free_ptr(p);})
+#define __cond_no_free_ptrs1(p, ...) __cond_no_free_ptrs(p)
+#define __cond_no_free_ptrs2(p, ...) \
+	__cond_no_free_ptrs(p), __cond_no_free_ptrs1(__VA_ARGS__)
+#define __cond_no_free_ptrs3(p, ...) \
+	__cond_no_free_ptrs(p), __cond_no_free_ptrs2(__VA_ARGS__)
+
+/*
+ * When an object is built up by an amalgamation of multiple allocations
+ * each of those need to be cleaned up on error, but there are occasions
+ * where once the object is registered all of those cleanups can be
+ * cancelled.  cond_no_free_ptr() arranges to call no_free_ptr() on all
+ * its arguments (up to 3) if @condition is true and runs @_fail
+ * otherwise (typically to return and trigger auto-cleanup).
+ */
+#define cond_no_free_ptr(condition, _fail, ...)                           \
+	if (condition) {                                                  \
+		CONCATENATE(__cond_no_free_ptrs, COUNT_ARGS(__VA_ARGS__)) \
+		(__VA_ARGS__);                                            \
+	} else {                                                          \
+		_fail;                                                    \
+	}
 
 /*
  * DEFINE_CLASS(name, type, exit, init, init_args...):