On Fri, May 26, 2023 at 2:25 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>> > + \
> > +static inline void ptr_guard_##_type##_cleanup(_Type **_ptr) \
> > +{ \
> > + _Type *_G = *_ptr; \
> > + if (_G) \
> > + _Put(_G); \
> > +}
> > +
> > +#define ptr_guard(_type, _name) \
> > + ptr_guard_##_type##_t _name __cleanup(ptr_guard_##_type##_cleanup)
> > +
>
> Ha, and then I wanted to create an fdput() guard... :/
I have to say, I'm not super-convinced about the macros to create
these guard functions and types.
I suspect that it would be better to literally just declare the guard
types directly. For the fdget idiom, you really just want the guard
type to *be* the 'struct fd'.
And I think you made those macros just make too many assumptions.
It's not just "struct fd", for example. If you want to do things like
wrap 'local_irq_save', again it's not a pointer, the context is just
'unsigned long irqflags'.
And I really don't think those type-creation macros buy you anything.
What's wrong with just writing it out:
typedef struct fd guard_fdget_type_t;
static inline struct fd guard_fdget_init(int fd)
{ return fdget(fd); }
static inline void guard_fdget_exit(struct fd fd)
{ fdput(fd); }
and
typedef struct mutex *guard_mutex_type_t;
static inline struct mutex *guard_mutex_init(struct mutex *mutex)
{ mutex_lock(mutex); return mutex; }
static inline void guard_mutex_exit(struct mutex *mutex)
{ mutex_unlock(mutex); }
I don't think the latter is in the *least* less readable than doing
DEFINE_LOCK_GUARD_1(mutex, struct mutex,
mutex_lock(_G->lock),
mutex_unlock(_G->lock))
which is this magic macro that creates those, and makes them
completely ungreppable - and makes the type system very inflexible.
So I really think that it would be a lot easier to write out the
wrappers explicitly for the few types that really want this.
I dunno.
Once again, I have written example code in my MUA that I have not
tested AT ALL, and that may be fundamentally broken for some very
obvious reason, and I'm just too stupid to see it.
Linus
