On Thu, Jun 08, 2023 at 08:45:53AM -0700, Linus Torvalds wrote:
> > DEFINE_CLASS(kfree, void *, kfree(THIS), p, void *p)
> >
> > smart_ptr(kfree, mem) = kzalloc_node(...);
> > if (!mem)
> > return -ENOMEM;
>
> No, the above is broken, and would result in us using "void *" in
> situations where we really *really* don't want that.
>
> For automatic freeing, you want something that can handle different
> types properly, and without having to constantly declare the types
> somewhere else before use.
Ah, I see what you did with the no_free_ptr(), that avoids having to
have two pointers around, nice!
> So for convenient automatic pointer freeing, you want an interface
> much more akin to
>
> struct whatever *ptr __automatic_kfree = kmalloc(...);
>
> which is much more legible, doesn't have any type mis-use issues, and
> is also just trivially dealt with by a
>
> static inline void automatic_kfree_wrapper(void *pp)
> { void *p = *(void **)pp; if (p) kfree(p); }
> #define __automatic_kfree \
> __attribute__((__cleanup__(automatic_kfree_wrapper)))
> #define no_free_ptr(p) \
> ({ __auto_type __ptr = (p); (p) = NULL; __ptr; })
>
> which I just tested generates the sane code even for the "set the ptr
> to NULL and return success" case.
>
> The above allows you to trivially do things like
>
> struct whatever *p __automatic_kfree = kmalloc(..);
>
> if (!do_something(p))
> return -ENOENT;
>
> return no_free_ptr(p);
>
> and it JustWorks(tm).
OK, something like so then?
#define DEFINE_FREE(name, type, free) \
static inline __free_##name(type *p) { type _P = *p; free; }
#define __free(name) __cleanup(__free_##name)
#define no_free_ptr(p) \
({ __auto_type __ptr = (p); (p) = NULL; __ptr; })
DEFINE_FREE(kfree, void *, if (_P) kfree(_P));
struct obj *p __free(kfree) = kmalloc(...);
if (!do_something(p))
return -ENOENT;
return no_free_ptr(p);
DEFINE_CLASS(find_get_context, struct perf_event_context *,
if (!IS_ERR_OR_NULL(_C)) put_ctx(_C),
find_get_context(task, event), struct task_struct *task, struct perf_event *event)
DEFINE_FREE(free_event, struct perf_event *,
if (!IS_ERR_OR_NULL(_P)) free_event(_P));
struct perf_event *event __free(free_event) = perf_event_alloc(...)
if (IS_ERR(event))
return event;
class(find_get_context, ctx)(task, event);
if (IS_ERR(ctx))
return (void*)ctx;
if (!task && !container_of(ctx, struct perf_cpu_context, ctx)->online)
return -ENODEV;
...
event->ctx = get_ctx(ctx);
return no_free_ptr(event);
works for me, I'll go make it happen.
