Re: [PATCH 10/15] multipathd: split check_path into two functions

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Sep 04, 2024 at 02:51:19PM -0400, Benjamin Marzinski wrote:
> On Wed, Sep 04, 2024 at 05:38:52PM +0200, Martin Wilck wrote:
> > On Wed, 2024-08-28 at 18:17 -0400, Benjamin Marzinski wrote:
> > > Split out the code that updates a path's state and sets up the next
> > > check time into its own function, update_path().
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > ---
> > >  multipathd/main.c | 31 ++++++++++++++++++++++---------
> > >  1 file changed, 22 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > index 94d4e421..300f8247 100644
> > > --- a/multipathd/main.c
> > > +++ b/multipathd/main.c
> > > @@ -2390,6 +2390,7 @@ sync_mpp(struct vectors * vecs, struct
> > > multipath *mpp, unsigned int ticks)
> > >  }
> > >  
> > >  enum check_path_return {
> > > +	CHECK_PATH_STARTED,
> > >  	CHECK_PATH_CHECKED,
> > >  	CHECK_PATH_SKIPPED,
> > >  	CHECK_PATH_REMOVED,
> > > @@ -2629,13 +2630,10 @@ update_path_state (struct vectors * vecs,
> > > struct path * pp)
> > >  }
> > >  
> > >  static int
> > > -check_path (struct vectors * vecs, struct path * pp, unsigned int
> > > ticks,
> > > -	    time_t start_secs)
> > > +check_path (struct path * pp, unsigned int ticks)
> > 
> > check_path() used to be one of our core functions, and you now re-
> > introduce it with quite different semantics. 
> > 
> > Perhaps choose a new name?
> 
> Sure. Although the new check_path() is just the beginning part of the
> old check_path(), where we actually run the checker, so it seems
> reasonable to me. But your objection is also reasonable. I was just
> getting sick of coming up with new function names by this point.
> 
> -Ben 

Do you have ideas for the name, because I can't think of anything that
makes more sense then check_path() in the code paths. Here's the code
paths

checkerloop (initialized paths)
-----------------------------------------
check_paths             update_paths
check_path              update_path
start_path_check        update_path_state
start_checker           check_path_state
checker_check           get_state
                        checker_get_state

checkerloop (uninitialized paths)
-----------------------------------------
check_paths                     update_paths
check_uninitialized_path        update_uninitialized_path
start_path_check                check_path_state
start_checker                   get_state
checker_check                   checker_get_state

pathinfo
----------------
start_checker   get_state
checker_check   checker_get_state

The only function that stands out to me as misnamed is
"check_path_state", which I think I'm going to change to
"get_updated_state", since that's a better description of what it's
doing, and it avoids using "check" in the update code path.

Also, like I said before, if you're looking for the function that gets
run to see if a path is due for checking, and runs the path checker if
it is, that's check_path(), just like it used to be. It just no longer
updates the path and mpp state based on the checker result like it used
to. That's now done by update_path() and update_mpp_prio(). 

> 
> > 
> > Martin
>