On Sat, May 27, 2023 at 12:18:04PM -0700, Linus Torvalds wrote:
> So how about we take a step back, and say "what if we don't create a
> new scope at all"?
Note that the lock_guard() and ptr_guard() options I have don't require
the new scope thing. The scope thing is optional.
> I think it actually improves on everything. The macros become
> *trivial*. The code becomes more legible.
>
> And you can have multiple different scopes very naturally, or you can
> just share a scope.
>
> Let me build up my argument here. Let's start with this *trivial* helper:
>
> /* Trivial "generic" auto-release macro */
> #define auto_release_name(type, name, init, exit) \
> type name __attribute__((__cleanup__(exit))) = (init)
>
> it's truly stupid: it creates a new variable of type 'type', with name
> 'name', and with the initial value 'init', and with the cleanup
> function 'exit'.
>
> It looks stupid, but it's the *good* stupid. It's really really
> obvious, and there are no games here.
I really don't like the auto naming since C++/C23 use auto for type
inference.
> Let me then introduce *one* other helper, because it turns out that
> much of the time, you don't really want to pick a name. So we'll use
> the above macro, but make a version of it that just automatically
> picks a unique name for you:
>
> #define auto_release(type, init, exit) \
> auto_release_name(type, \
> __UNIQUE_ID(auto) __maybe_unused, \
> init, exit)
I like that option.
> And it turns out that the above two trivial macros are actually quite
> useful in themselves. You want to do an auto-cleanup version of
> 'struct fd'? It's trivial:
>
> /* Trivial "getfd()" wrapper */
> static inline void release_fd(struct fd *fd)
> { fdput(*fd); }
>
> #define auto_getfd(name, n) \
> auto_release_name(struct fd, name, fdget(n), release_fd)
>
> - I think the above is simpler and objectively better in every way
> from the explicitly scoped thing
Well, I think having that as a option would still be very nice.
> - I also suspect that to get maximum advantage of this all, we would
> have to get rid of -Wdeclaration-after-statement
>
> That last point is something that some people will celebrate, but I do
> think it has helped keep our code base somewhat controlled, and made
> people think more about the variables they declare.
>
> But if you start doing consecutive cleanup operations, you really end
> up wanting to do thigns like this:
>
> int testfd2(int fd1, int fd2)
> {
> auto_getfd(f1, fd1);
> if (!f1.file)
> return -EBADF;
> auto_getfd(f2, fd2);
> if (!f2.file)
> return -EBADF;
> return do_something (f1, f2);
> }
If you extend the ptr_guard() idea you don't need to get rid of
-Wdeclaration-after-statement and we could write it like:
int testfd2(int fd1, int fd2)
{
ptr_guard(fdput, f1) = fdget(fd1);
ptr_guard(fdput, f2) = null_ptr(fdput);
if (!f1.file)
return -EBADF;
f2 = fdget(f2);
if (!f2.file)
return -EBADF;
return do_something(f1, f2);
}
Yes, the macros would be a little more involved, but not horribly so I
think.
typedef struct fd guard_fdput_t;
static const struct fd guard_fdput_null = __to_fd(0);
static inline void guard_fdput_cleanup(struct fd *fd)
{
fdput(*fd);
}
#define ptr_guard(_guard, _name) \
guard##_guard##_t _name __cleanup(guard##_guard##_cleanup)
#define null_ptr(_guard) guard##_guard##_null;
And for actual pointer types (as opposed to fat pointer wrappers like
struct fd) we can have a regular helper macro like earlier:
#define DEFINE_PTR_GUARD(_guard, _type, _put) \
typdef _type *guard##_guard##_t; \
static const _type *guard##_guard##_null = NULL; \
static inline void guard##_guard##_cleanup(_type **ptr) \
{ if (*ptr) _put(*ptr); }
[NOTE: value propagation gets rid of the above conditional where
appropriate]
eg.:
DEFINE_PTR_GUARD(put_task, struct task_struct, put_task_struct);
Possibly with a helper:
#define null_guard(_guard, _name) \
ptr_guard(_guard, _name) = null_ptr(_guard)
Now, ptr_guard() per the above, is asymmetric in that it only cares
about release, let guard() be the symmetric thing that also cares about
init like so:
#define DEFINE_GUARD(_guard, _type, _put, _get) \
DEFINE_PTR_GUARD(_guard, _type, _put) \
static inline void guard##_guard##_init(guard##_guard##_t arg) \
{ _get(arg); return arg; }
#define guard(_guard, _name, _var...) \
ptr_guard(_guard, _name) = guard##_guard@##_init(_var)
#define anon_guard(_guard, _var..) \
guard(_guard, __UNIQUE_ID(guard), _var)
for eg.:
DEFINE_GUARD(mutex, struct mutex, mutex_unlock, mutex_lock);
which then lets one write:
int testfd2(int fd1, int fd2)
{
anon_guard(mutex, &cgroup_mutex);
ptr_guard(fdput, f1) = fdget(fd1);
null_guard(fdput, f2);
if (!f1.file)
return -EBADF;
f2 = fdget(fd2);
if (!f2.file)
return -EBADf;
return do_something(f1,f2);
}
The RCU thing can then either be manually done like:
struct rcu_guard;
typedef struct rcu_guard *guard_rcu_t;
static const guard_rcu_null = NULL;
static inline guard_rcu_cleanup(struct rcu_guard **ptr)
{ if (*ptr) rcu_read_unlock(); }
static inline struct rcu_guard *guard_rcu_init(void)
{ rcu_read_lock(); return (void*)1; }
(or because we'll need this pattern a few more times, with yet another
DEFINE_foo_GUARD helper)
and:
anon_guard(rcu);
works.
And at this point the previous scope() things are just one helper macro
away:
#define scope(_guard, _var..) \
for (guard##_guard##_t *done = NULL, scope = guard##_guard##_init(var); \
!done; done++)
to be used where appropriate etc..
Yes, it's a wee bit more involved, but I'm thinking it gives a fair
amount of flexibility and we don't need to ret rid of
-Wdeclaration-after-statement.
Hmm?
