Re: [PATCH 06/18] libmultipath: use shared_ptr for checker classes

[Benjamin Marzinski <bmarzins@redhat.com>]

On Sun, May 17, 2026 at 11:57:25AM +0200, Martin Wilck wrote:
> On Tue, 2026-05-12 at 20:49 -0400, Benjamin Marzinski wrote:
> > On Tue, May 05, 2026 at 05:43:20PM +0200, Martin Wilck wrote:
> > > Utilize the share_ptr code for tracking the refcount of checker
> > > classes.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/checkers.c | 98 +++++++++----------------------------
> > > ----
> > >  libmultipath/checkers.h |  2 -
> > >  2 files changed, 20 insertions(+), 80 deletions(-)
> > > 
> > > diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> > > index 8f18ca6..a3b9cc8 100644
> > > --- a/libmultipath/checkers.c
> > > +++ b/libmultipath/checkers.c
> > [...]
> > > @@ -163,8 +143,7 @@ static struct checker_class
> > > *add_checker_class(const char *name)
> > >  		goto out;
> > >  
> > >  	c->mp_init = (int (*)(struct checker *)) dlsym(c->handle,
> > > "libcheck_mp_init");
> > > -	c->reset = (void (*)(void)) dlsym(c->handle,
> > > "libcheck_reset");
> > > -	c->thread = (void *(*)(void*)) dlsym(c->handle,
> > > "libcheck_thread");
> > > +	c->reset = (void (*)(void))dlsym(c->handle,
> > > "libcheck_reset");
> > 
> > Nitpick. Dropped a space here.
> 
> I didn't do that. clang-format did, as it also reformats context lines
> in patches. I think clang-format is correct. We usually don't use space
> after cast, do we? (As usual, our code base is inconsistent in this
> respect).
> 
> While I don't want to enforce formatting style throughout the entire
> code base, I tend to keep the changes that clang-format applies to
> context lines (be it only to avoid red lights from the GitHub CI).
> 
> Do you disagree with this policy?

It's fine. It just doesn't match the dlsym lines around it, which is fine.
-Ben

> 
> Regards
> Martin
> 
> > 
> > >  	c->pending = (int (*)(struct checker *)) dlsym(c->handle,
> > > "libcheck_pending");
> > >  	c->need_wait = (bool (*)(struct checker *)) dlsym(c-
> > > >handle, "libcheck_need_wait");
> > >  	/* These 5 functions can be NULL. call dlerror() to clear
> > > out any
> > [...]
> > > @@ -371,43 +350,6 @@ bad_id:
> > >  	return generic_msg[CHECKER_MSGID_NONE];
> > >  }
> > 
> > Removing the checker thread stuff is more of a cleanup from d1ebd977.
> > It
> > should probably either be it's own cleanup patch, or it should at
> > least
> > get mentioned in the commit message.
> >   
> > > -static void checker_cleanup_thread(void *arg)
> > > -{
> > > -	struct checker_class *cls = arg;
> > > -
> > > -	free_checker_class(cls);
> > > -	rcu_unregister_thread();
> > > -}
> > > -
> > > -static void *checker_thread_entry(void *arg)
> > > -{
> > > -	struct checker_context *ctx = arg;
> > > -	void *rv;
> > > -
> > > -	rcu_register_thread();
> > > -	pthread_cleanup_push(checker_cleanup_thread, ctx->cls);
> > > -	rv = ctx->cls->thread(ctx);
> > > -	pthread_cleanup_pop(1);
> > > -	return rv;
> > > -}
> > > -
> > 
> > Also, start_checker_thread() is stil declared in
> > libmultipath/checkers.h,
> > along with a comment.
> > 
> > -Ben
> > 
> > > -int start_checker_thread(pthread_t *thread, const pthread_attr_t
> > > *attr,
> > > -			 struct checker_context *ctx)
> > > -{
> > > -	int rv;
> > > -
> > > -	assert(ctx && ctx->cls && ctx->cls->thread);
> > > -	/* Take a ref here, lest the class be freed before the
> > > thread starts */
> > > -	(void)checker_class_ref(ctx->cls);
> > > -	rv = pthread_create(thread, attr, checker_thread_entry,
> > > ctx);
> > > -	if (rv != 0) {
> > > -		condlog(1, "failed to start checker thread for %s:
> > > %m",
> > > -			ctx->cls->name);
> > > -		checker_class_unref(ctx->cls);
> > > -	}
> > > -	return rv;
> > > -}
> > > -
> > >  void checker_clear_message (struct checker *c)
> > >  {
> > >  	if (!c)