linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dan Williams <dan.j.williams@intel.com>
To: torvalds@linux-foundation.org, peterz@infradead.org,
	gregkh@linuxfoundation.org
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	linux-kernel@vger.kernel.org, linux-cxl@vger.kernel.org
Subject: [PATCH 2/3] cleanup: Introduce cond_no_free_ptr()
Date: Tue, 27 Feb 2024 08:48:59 -0800	[thread overview]
Message-ID: <170905253897.2268463.13371523233762430828.stgit@dwillia2-xfh.jf.intel.com> (raw)
In-Reply-To: <170905252721.2268463.6714121678946763402.stgit@dwillia2-xfh.jf.intel.com>

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 <peterz@infradead.org>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/cleanup.h |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

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...):


  parent reply	other threads:[~2024-02-27 16:48 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-27 16:48 [PATCH 0/2] cleanup: A couple extensions for conditional resource management Dan Williams
2024-02-27 16:48 ` [PATCH 1/3] cleanup: Add cond_guard() to conditional guards Dan Williams
2024-02-27 20:49   ` Linus Torvalds
2024-02-27 16:48 ` Dan Williams [this message]
2024-02-27 20:40   ` [PATCH 2/3] cleanup: Introduce cond_no_free_ptr() Linus Torvalds
2024-02-27 16:49 ` [PATCH 3/3] cxl/region: Use cond_guard() in show_targetN() Dan Williams
2024-02-27 20:55   ` Linus Torvalds
2024-02-27 21:41     ` Dan Williams
2024-02-27 22:34       ` Linus Torvalds
2024-02-27 23:56         ` Dan Williams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=170905253897.2268463.13371523233762430828.stgit@dwillia2-xfh.jf.intel.com \
    --to=dan.j.williams@intel.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-cxl@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).