Re: [PATCH 10/14] libmpathpersist: only clear the key if we are using the prkeys file

[Benjamin Marzinski <bmarzins@redhat.com>]

On Mon, Aug 25, 2025 at 06:21:59PM +0200, Martin Wilck wrote:
> On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> > Otherwise this request will create a useless prkeys file.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index dfdadab6..f901b955 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -831,7 +831,8 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  		break;
> >  	case MPATH_PROUT_CLEAR_SA:
> >  		update_prflag(mpp->alias, 0);
> > -		update_prkey(mpp->alias, 0);
> > +		if (mpp->prkey_source == PRKEY_SOURCE_FILE)
> > +			update_prkey(mpp->alias, 0);
> 
> Should this condition be checked in update_prkey_flags() directly?

We could, but it would just end up being a pointless extra check most of
the time. Aside from the CLEAR command, we only set the prkey when we
are registering a key or reverting the prkey if that registration
failed. When we first set it during register, we need to check
prkey_source early, since there is a bunch of things we can't do if we
aren't using the prkeys file. And we don't want to revert it unless we
updated it and have an old value to revert to.  In both cases we already
had to do the necessary check before calling update_prkey(). The CLEAR
command is the only one where we don't need to check if we are using the
prkeys file earlier. So, I'd prefer to leave the check outside of
update_prkey_flags() here as well, to avoid the unnecessary extra work.

-Ben

> 
> Martin