| Message ID | 20230911163846.27197-23-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: user-friendly names rework | expand |
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? -Ben > + const char *alias1 = *((char * const *)p1); > + const char *alias2 = *((char * const *)p2); > + > + if (alias1 && alias2) { > + ssize_t ldif = strlen(alias1) - strlen(alias2); > + > + if (ldif) > + return ldif; > + return strcmp(alias1, alias2); > + } else > + /* Move NULL alias to the end */ > + return alias1 ? -1 : alias2 ? 1 : 0; > +} > + > static int add_binding(Bindings *bindings, const char *alias, const char *wwid) > { > struct binding *bdg; > @@ -128,7 +157,7 @@ static int add_binding(Bindings *bindings, const char *alias, const char *wwid) > * sorted already. > */ > vector_foreach_slot_backwards(bindings, bdg, i) { > - if ((cmp = strcmp(bdg->alias, alias)) <= 0) > + if ((cmp = alias_compar(&bdg->alias, &alias)) <= 0) > break; > } > > @@ -657,16 +686,10 @@ static int _check_bindings_file(const struct config *conf, FILE *file, > return rc; > } > > -static int alias_compar(const void *p1, const void *p2) > +static int mp_alias_compar(const void *p1, const void *p2) > { > - const char *alias1 = (*(struct mpentry * const *)p1)->alias; > - const char *alias2 = (*(struct mpentry * const *)p2)->alias; > - > - if (alias1 && alias2) > - return strcmp(alias1, alias2); > - else > - /* Move NULL alias to the end */ > - return alias1 ? -1 : alias2 ? 1 : 0; > + return alias_compar(&((*(struct mpentry * const *)p1)->alias), > + &((*(struct mpentry * const *)p2)->alias)); > } > > /* > @@ -700,7 +723,7 @@ int check_alias_settings(const struct config *conf) > pthread_cleanup_push_cast(free_bindings, &bindings); > pthread_cleanup_push(cleanup_vector_free, mptable); > > - vector_sort(mptable, alias_compar); > + vector_sort(mptable, mp_alias_compar); > vector_foreach_slot(mptable, mpe, i) { > if (!mpe->alias) > /* > -- > 2.42.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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(). Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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
On Wed, 2023-09-13 at 09:38 -0500, Benjamin Marzinski wrote: > 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: > > > > + */ > > > > +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. Can I take this as a reviewed-by? Thanks, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, Sep 13, 2023 at 09:07:27PM +0200, Martin Wilck wrote: > On Wed, 2023-09-13 at 09:38 -0500, Benjamin Marzinski wrote: > > 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: > > > > > + */ > > > > > +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. > > Can I take this as a reviewed-by? Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> > > Thanks, > Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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) +{ + const char *alias1 = *((char * const *)p1); + const char *alias2 = *((char * const *)p2); + + if (alias1 && alias2) { + ssize_t ldif = strlen(alias1) - strlen(alias2); + + if (ldif) + return ldif; + return strcmp(alias1, alias2); + } else + /* Move NULL alias to the end */ + return alias1 ? -1 : alias2 ? 1 : 0; +} + static int add_binding(Bindings *bindings, const char *alias, const char *wwid) { struct binding *bdg; @@ -128,7 +157,7 @@ static int add_binding(Bindings *bindings, const char *alias, const char *wwid) * sorted already. */ vector_foreach_slot_backwards(bindings, bdg, i) { - if ((cmp = strcmp(bdg->alias, alias)) <= 0) + if ((cmp = alias_compar(&bdg->alias, &alias)) <= 0) break; } @@ -657,16 +686,10 @@ static int _check_bindings_file(const struct config *conf, FILE *file, return rc; } -static int alias_compar(const void *p1, const void *p2) +static int mp_alias_compar(const void *p1, const void *p2) { - const char *alias1 = (*(struct mpentry * const *)p1)->alias; - const char *alias2 = (*(struct mpentry * const *)p2)->alias; - - if (alias1 && alias2) - return strcmp(alias1, alias2); - else - /* Move NULL alias to the end */ - return alias1 ? -1 : alias2 ? 1 : 0; + return alias_compar(&((*(struct mpentry * const *)p1)->alias), + &((*(struct mpentry * const *)p2)->alias)); } /* @@ -700,7 +723,7 @@ int check_alias_settings(const struct config *conf) pthread_cleanup_push_cast(free_bindings, &bindings); pthread_cleanup_push(cleanup_vector_free, mptable); - vector_sort(mptable, alias_compar); + vector_sort(mptable, mp_alias_compar); vector_foreach_slot(mptable, mpe, i) { if (!mpe->alias) /*