Re: [dm-devel] [PATCH v2 22/37] libmultipath: sort aliases by length and strcmp

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Sep 13, 2023 at 03:53:25PM +0200, Martin Wilck wrote:
> On Tue, 2023-09-12 at 18:00 -0500, Benjamin Marzinski wrote:
> > On Mon, Sep 11, 2023 at 06:38:31PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > The current sort order of aliases is alphabetical, which is does
> > > not match
> > > the actual order of aliases, where "mpathaa" > "mpathz". Change the
> > > ordering as
> > > follows: first sort by string length, then alphabetically. This
> > > will make
> > > sure that for aliases with the same prefix, alias order is correct
> > > ("mpathaaa"
> > > will be sorted after "mpathzz", etc). Even for mixed prefixes, the
> > > alias
> > > order will be correct for every individual prefix, even though
> > > aliases with
> > > different prefixes may alternate in the file.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/alias.c | 45 +++++++++++++++++++++++++++++++++-------
> > > ----
> > >  1 file changed, 34 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> > > index 58436ec..af6565b 100644
> > > --- a/libmultipath/alias.c
> > > +++ b/libmultipath/alias.c
> > > @@ -117,6 +117,35 @@ static const struct binding
> > > *get_binding_for_wwid(const Bindings *bindings,
> > >         return NULL;
> > >  }
> > >  
> > > +/*
> > > + * Sort order for aliases.
> > > + *
> > > + * The "numeric" ordering of aliases for a given prefix P is
> > > + * Pa, ..., Pz, Paa, ..., Paz, Pba, ... , Pzz, Paaa, ..., Pzzz,
> > > Paaaa, ...
> > > + * We use the fact that for equal prefix, longer strings are
> > > always
> > > + * higher than shorter ones. Strings of equal length are sorted
> > > alphabetically.
> > > + * This is achieved by sorting be length first, then using
> > > strcmp().
> > > + * If multiple prefixes are in use, the aliases with a given
> > > prefix will
> > > + * not necessarily be in a contiguous range of the vector, but
> > > they will
> > > + * be ordered such that for a given prefix, numercally higher
> > > aliases will
> > > + * always be sorted after lower ones.
> > > + */
> > > +static int alias_compar(const void *p1, const void *p2)
> > > +{
> > 
> > I'm confused as to why we need to pass p1 and p2 and pointers to
> > pointers to chars, instead of simply as pointers to chars. We always
> > derefence them immediately, and only use the dereferenced pointers.
> > Am I
> > missing something?
> 
> I wanted to make the relationship of alias_compar() and
> mp_alias_compar() as obvious as possible. mp_alias_compar() takes 
> (struct mpentry **) arguments, because it's used as an argument to
> vector_sort() aka msort(), which has the same calling convention as
> qsort()'s "compar" argument. Therefore I wrote alias_compar() such that
> it takes (char **) pointers. This way we could use alias_compar() as an
> argument to vector_sort() as well, even though we currently don't.
> 
> 
> Does this make sense? If not, I can change it, but I think the function
> should not be named alias_compar() if it can't be passed to
> vector_sort().

It's fine as it is. I was just confused as to why.

-Ben

> 
> Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel