| Message ID | 20230901180235.23980-19-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 Fri, Sep 01, 2023 at 08:02:31PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > Rather than opening the bindings file every time we must retrieve > a binding, keep the contents in memory and write the file only > if additions have been made. This simplifies the code, and should speed up > alias lookups significantly. As a side effect, the aliases will be stored > sorted by alias, which changes the way aliases are allocated if there are > unused "holes" in the sequence of aliases. For example, if the bindings file > contains mpathb, mpathy, and mpatha, in this order, the next new alias used to > be mpathz and is now mpathc. > > Another side effect is that multipathd will not automatically pick up changes > to the bindings file at runtime without a reconfigure operation. It is > questionable whether these on-the-fly changes were a good idea in the first > place, as inconsistent configurations may easily come to pass. It desired, > it would be feasible to implement automatic update of the bindings using the > existing inotify approach. I'm not so much worried about someone manually changing the bindings file outside of multipath. But it is possible for multipathd to miss changes made by the multipath command. For instance, say that someone has find_multipaths set to "yes" and adds a new device, but only a single path comes up. They know there will be more paths later, so they run # multipath <new_path_dev> to create a multipath device for this. Multipathd won't pick up this new binding. If, for some reason the path goes away, and comes back later, it will now be in the bindings file, but multipathd will still have no record of the correct binding for it, which already exists in the bindings file. I don't know if this needs something as complex as inotify to handle. We could just record the mtime of the bindings file when we read it. If it has changed when we call get_user_friendly_alias() or get_user_friendly_wwid() we would call check_alias_settings(). additional comments below. > The new implementation of get_user_friendly_alias() is slightly different > than before. The logic is summarized in a comment in the code. Unit tests > will be provided that illustrate the changes. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/alias.c | 369 ++++++++++++++++----------------------- > libmultipath/alias.h | 2 +- > libmultipath/configure.c | 3 +- > 3 files changed, 157 insertions(+), 217 deletions(-) > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c > index 2f9bc82..6003df0 100644 > --- a/libmultipath/alias.c > +++ b/libmultipath/alias.c > @@ -50,8 +50,6 @@ > "# alias wwid\n" \ > "#\n" > > -static const char bindings_file_header[] = BINDINGS_FILE_HEADER; > - > struct binding { > char *alias; > char *wwid; > @@ -80,6 +78,45 @@ static void _free_binding(struct binding *bdg) > free(bdg); > } > > +static const struct binding *get_binding_for_alias(const Bindings *bindings, > + const char *alias) > +{ > + const struct binding *bdg; > + int i; > + > + if (!alias) > + return NULL; > + vector_foreach_slot(bindings, bdg, i) { > + if (!strncmp(bdg->alias, alias, WWID_SIZE)) { > + condlog(3, "Found matching alias [%s] in bindings file." > + " Setting wwid to %s", alias, bdg->wwid); > + return bdg; > + } > + } > + > + condlog(3, "No matching alias [%s] in bindings file.", alias); > + return NULL; > +} > + > +static const struct binding *get_binding_for_wwid(const Bindings *bindings, > + const char *wwid) > +{ > + const struct binding *bdg; > + int i; > + > + if (!wwid) > + return NULL; > + vector_foreach_slot(bindings, bdg, i) { > + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) { > + condlog(3, "Found matching wwid [%s] in bindings file." > + " Setting alias to %s", wwid, bdg->alias); > + return bdg; > + } > + } > + condlog(3, "No matching wwid [%s] in bindings file.", wwid); > + return NULL; > +} > + > static int add_binding(Bindings *bindings, const char *alias, const char *wwid) > { > struct binding *bdg; > @@ -115,6 +152,24 @@ static int add_binding(Bindings *bindings, const char *alias, const char *wwid) > return BINDING_ERROR; > } > > +static int delete_binding(Bindings *bindings, const char *wwid) > +{ > + struct binding *bdg; > + int i; > + > + vector_foreach_slot(bindings, bdg, i) { > + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) { > + _free_binding(bdg); > + break; > + } > + } > + if (i >= VECTOR_SIZE(bindings)) > + return BINDING_NOTFOUND; > + > + vector_del_slot(bindings, i); > + return BINDING_DELETED; > +} > + > static int write_bindings_file(const Bindings *bindings, int fd) > { > struct binding *bnd; > @@ -267,38 +322,15 @@ static bool id_already_taken(int id, const char *prefix, const char *map_wwid) > return alias_already_taken(alias, map_wwid); > } > > -/* > - * Returns: 0 if matching entry in WWIDs file found > - * -1 if an error occurs > - * >0 a free ID that could be used for the WWID at hand > - * *map_alias is set to a freshly allocated string with the matching alias if > - * the function returns 0, or to NULL otherwise. > - */ > -static int > -lookup_binding(FILE *f, const char *map_wwid, char **map_alias, > - const char *prefix, int check_if_taken) > +int get_free_id(const Bindings *bindings, const char *prefix, const char *map_wwid) > { > - char buf[LINE_MAX]; > - unsigned int line_nr = 0; > - int id = 1; > + const struct binding *bdg; > + int i, id = 1; > int biggest_id = 1; > int smallest_bigger_id = INT_MAX; > > - *map_alias = NULL; > - > - rewind(f); > - while (fgets(buf, LINE_MAX, f)) { > - const char *alias, *wwid; > - char *c, *saveptr; > - int curr_id; > - > - line_nr++; > - c = strpbrk(buf, "#\n\r"); > - if (c) > - *c = '\0'; > - alias = strtok_r(buf, " \t", &saveptr); > - if (!alias) /* blank line */ > - continue; > + vector_foreach_slot(bindings, bdg, i) { > + int curr_id = scan_devname(bdg->alias, prefix); If we know that the bindings will be sorted by alias order, we don't need to do all this. Something like this should work: if (curr_id <= 0) continue; while (id < curr_id) { if (!id_already_taken(id, prefix, map_wwid)) return id; id++; } if (id == INT_MAX) break; id++; } if (id == INT_MAX) condlog(0, "no more available user_friendly_names"); return id < INT_MAX ? id : -1; } > > /* > * Find an unused index - explanation of the algorithm > @@ -333,8 +365,6 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, > * biggest_id is always > smallest_bigger_id, except in the > * "perfectly ordered" case. > */ > - > - curr_id = scan_devname(alias, prefix); > if (curr_id == id) { > if (id < INT_MAX) > id++; > @@ -345,36 +375,15 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, > } > if (curr_id > biggest_id) > biggest_id = curr_id; > + > if (curr_id > id && curr_id < smallest_bigger_id) > smallest_bigger_id = curr_id; > - wwid = strtok_r(NULL, " \t", &saveptr); > - if (!wwid){ > - condlog(3, > - "Ignoring malformed line %u in bindings file", > - line_nr); > - continue; > - } > - if (strcmp(wwid, map_wwid) == 0){ > - condlog(3, "Found matching wwid [%s] in bindings file." > - " Setting alias to %s", wwid, alias); > - *map_alias = strdup(alias); > - if (*map_alias == NULL) { > - condlog(0, "Cannot copy alias from bindings " > - "file: out of memory"); > - return -1; > - } > - return 0; > - } > } > - if (!prefix && check_if_taken) > - id = -1; > - if (id >= smallest_bigger_id) { > - if (biggest_id < INT_MAX) > - id = biggest_id + 1; > - else > - id = -1; > - } > - if (id > 0 && check_if_taken) { > + > + if (id >= smallest_bigger_id) > + id = biggest_id < INT_MAX ? biggest_id + 1 : -1; > + > + if (id > 0) { > while(id_already_taken(id, prefix, map_wwid)) { > if (id == INT_MAX) { > id = -1; > @@ -391,63 +400,16 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, > } > } > } > - if (id < 0) { > + > + if (id < 0) > condlog(0, "no more available user_friendly_names"); > - return -1; > - } else > - condlog(3, "No matching wwid [%s] in bindings file.", map_wwid); > return id; > } > > -static int > -rlookup_binding(FILE *f, char *buff, const char *map_alias) > -{ > - char line[LINE_MAX]; > - unsigned int line_nr = 0; > - > - buff[0] = '\0'; > - > - while (fgets(line, LINE_MAX, f)) { > - char *c, *saveptr; > - const char *alias, *wwid; > - > - line_nr++; > - c = strpbrk(line, "#\n\r"); > - if (c) > - *c = '\0'; > - alias = strtok_r(line, " \t", &saveptr); > - if (!alias) /* blank line */ > - continue; > - wwid = strtok_r(NULL, " \t", &saveptr); > - if (!wwid){ > - condlog(3, > - "Ignoring malformed line %u in bindings file", > - line_nr); > - continue; > - } > - if (strlen(wwid) > WWID_SIZE - 1) { > - condlog(3, > - "Ignoring too large wwid at %u in bindings file", line_nr); > - continue; > - } > - if (strcmp(alias, map_alias) == 0){ > - condlog(3, "Found matching alias [%s] in bindings file." > - " Setting wwid to %s", alias, wwid); > - strlcpy(buff, wwid, WWID_SIZE); > - return 0; > - } > - } > - condlog(3, "No matching alias [%s] in bindings file.", map_alias); > - > - return -1; > -} > - > static char * > -allocate_binding(int fd, const char *wwid, int id, const char *prefix) > +allocate_binding(const char *filename, const char *wwid, int id, const char *prefix) > { > STRBUF_ON_STACK(buf); > - off_t offset; > - ssize_t len; > char *alias, *c; > > if (id <= 0) { > @@ -460,29 +422,22 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix) > format_devname(&buf, id) == -1) > return NULL; > > - if (print_strbuf(&buf, " %s\n", wwid) < 0) > - return NULL; > - > - offset = lseek(fd, 0, SEEK_END); > - if (offset < 0){ > - condlog(0, "Cannot seek to end of bindings file : %s", > - strerror(errno)); > - return NULL; > - } > - > - len = get_strbuf_len(&buf); > alias = steal_strbuf_str(&buf); > > - if (write(fd, alias, len) != len) { > - condlog(0, "Cannot write binding to bindings file : %s", > - strerror(errno)); > - /* clear partial write */ > - if (ftruncate(fd, offset)) > - condlog(0, "Cannot truncate the header : %s", > - strerror(errno)); > + if (add_binding(&global_bindings, alias, wwid) != BINDING_ADDED) { > + condlog(0, "%s: cannot allocate new binding %s for %s", > + __func__, alias, wwid); > free(alias); > return NULL; > } > + > + if (update_bindings_file(&global_bindings, filename) == -1) { > + condlog(1, "%s: deleting binding %s for %s", __func__, alias, wwid); > + delete_binding(&global_bindings, wwid); > + free(alias); > + return NULL; > + } > + We no longer need to end the alias at the space, since we're not printing the wwid in the buffer. > c = strchr(alias, ' '); > if (c) > *c = '\0'; > @@ -491,144 +446,130 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix) > return alias; > } > > +/* > + * get_user_friendly_alias() action table > + * > + * The table shows the various cases, the actions taken, and the CI > + * functions from tests/alias.c that represent them. > + * > + * - O: old alias given > + * - A: old alias in table (y: yes, correct WWID; X: yes, wrong WWID) > + * - W: wwid in table > + * - U: old alias is used > + * > + * | No | O | A | W | U | action | function gufa_X | > + * |----+---+---+---+---+--------------------------------------------+------------------------------| > + * | 1 | n | - | n | - | get new alias | nomatch_Y | > + * | 2 | n | - | y | - | use alias from bindings | match_a_Y | > + * | 3 | y | n | n | n | add binding for old alias | old_nomatch_nowwidmatch | > + * | 4 | y | n | n | y | error, no alias (can't add entry) | old_nomatch_nowwidmatch_used | > + * | 5 | y | n | y | - | use alias from bindings (avoid duplicates) | old_nomatch_wwidmatch | > + * | 6 | y | y | n | - | [ impossible ] | - | > + * | 7 | y | y | y | n | use old alias == alias from bindings | old_match | > + * | 8 | y | y | y | y | error, no alias (would be duplicate) | old_match_used | > + * | 9 | y | X | n | - | get new alias | old_match_other | > + * | 10 | y | X | y | - | use alias from bindings | old_match_other_wwidmatch | > + * > + * Notes: > + * - "use alias from bindings" means that the alias from the bindings file will > + * be tried; if it is in use, the alias selection will fail. No other > + * bindings will be attempted. > + * - "get new alias" fails if all aliases are used up, or if writing the > + * bindings file fails. > + */ > + > char *get_user_friendly_alias(const char *wwid, const char *file, const char *alias_old, > const char *prefix, bool bindings_read_only) > { > char *alias = NULL; > int id = 0; > - int fd, can_write; > bool new_binding = false; > - char buff[WWID_SIZE]; > - FILE *f; > + bool old_alias_taken = false; > + const struct binding *bdg; > > - fd = open_file(file, &can_write, bindings_file_header); > - if (fd < 0) > - return NULL; > - > - f = fdopen(fd, "r"); > - if (!f) { > - condlog(0, "cannot fdopen on bindings file descriptor"); > - close(fd); > - return NULL; > - } > - > - if (!strlen(alias_old)) > + if (!*alias_old) > goto new_alias; > > - /* lookup the binding. if it exists, the wwid will be in buff > - * either way, id contains the id for the alias > - */ > - rlookup_binding(f, buff, alias_old); > - > - if (strlen(buff) > 0) { > - /* if buff is our wwid, it's already > - * allocated correctly > - */ > - if (strcmp(buff, wwid) == 0) { > + /* See if there's a binding matching both alias_old and wwid */ > + bdg = get_binding_for_alias(&global_bindings, alias_old); > + if (bdg) { > + if (!strcmp(bdg->wwid, wwid)) { I'm still not convinced that it is possible for mpp->alias_old to be set when there isn't a multipath device with the alias_old and the desired wwid. Unless I'm missing something we should be able to skip the alias_already_taken() check. > if (!alias_already_taken(alias_old, wwid)) > alias = strdup(alias_old); > else > condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", > alias_old, wwid); > goto out; > - > } else { > condlog(0, "alias %s already bound to wwid %s, cannot reuse", > - alias_old, buff); > - goto new_alias; ; > + alias_old, bdg->wwid); > + goto new_alias; > } > } > > - /* > - * Look for an existing alias in the bindings file. > - * Pass prefix = NULL, so lookup_binding() won't try to allocate a new id. > - */ > - id = lookup_binding(f, wwid, &alias, NULL, 0); > - if (alias) { > - if (alias_already_taken(alias, wwid)) { > - free(alias); > - alias = NULL; > - } else > - condlog(3, "Use existing binding [%s] for WWID [%s]", > - alias, wwid); > - goto out; > - } > - > /* allocate the existing alias in the bindings file */ > - id = scan_devname(alias_old, prefix); > - if (id > 0 && id_already_taken(id, prefix, wwid)) { again, I think it's safe to skip this check, since we're checking alias_old. > + if (alias_already_taken(alias_old, wwid)) { > condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", > alias_old, wwid); > + old_alias_taken = true; > + } else > + id = scan_devname(alias_old, prefix); > + > +new_alias: > + /* Check for existing binding of WWID */ > + bdg = get_binding_for_wwid(&global_bindings, wwid); > + if (bdg) { > + if (!alias_already_taken(bdg->alias, wwid)) { > + condlog(3, "Use existing binding [%s] for WWID [%s]", > + bdg->alias, wwid); > + alias = strdup(bdg->alias); > + } > goto out; > } > > -new_alias: > - if (id <= 0) { > + /* > + * old_alias_taken means that the WWID is not in the bindings file, but > + * alias_old is currently taken by a different WWID. We shouldn't added > + * a new binding in this case. > + */ > + if (id <= 0 && !old_alias_taken) { > /* > * no existing alias was provided, or allocating it > * failed. Try a new one. > */ > - id = lookup_binding(f, wwid, &alias, prefix, 1); > - if (id == 0 && alias_already_taken(alias, wwid)) { > - free(alias); > - alias = NULL; > - } > + id = get_free_id(&global_bindings, prefix, wwid); > if (id <= 0) > goto out; > else > new_binding = true; > } > > - if (fflush(f) != 0) { > - condlog(0, "cannot fflush bindings file stream : %s", > - strerror(errno)); > - goto out; > - } > + if (!bindings_read_only && id > 0) > + alias = allocate_binding(file, wwid, id, prefix); > > - if (can_write && !bindings_read_only) { > - alias = allocate_binding(fd, wwid, id, prefix); > - if (alias && !new_binding) > - condlog(2, "Allocated existing binding [%s] for WWID [%s]", > - alias, wwid); > - } > + if (alias && !new_binding) > + condlog(2, "Allocated existing binding [%s] for WWID [%s]", > + alias, wwid); > > out: > - pthread_cleanup_push(free, alias); > - fclose(f); > - pthread_cleanup_pop(0); > return alias; > } > > -int > -get_user_friendly_wwid(const char *alias, char *buff, const char *file) > +int get_user_friendly_wwid(const char *alias, char *buff) > { > - int fd, unused; > - FILE *f; > + const struct binding *bdg; > > if (!alias || *alias == '\0') { > condlog(3, "Cannot find binding for empty alias"); > return -1; > } > > - fd = open_file(file, &unused, bindings_file_header); > - if (fd < 0) > - return -1; > - > - f = fdopen(fd, "r"); > - if (!f) { > - condlog(0, "cannot fdopen on bindings file descriptor : %s", > - strerror(errno)); > - close(fd); > + bdg = get_binding_for_alias(&global_bindings, alias); > + if (!bdg) { > + *buff = '\0'; > return -1; > } > - > - rlookup_binding(f, buff, alias); > - if (!strlen(buff)) { > - fclose(f); > - return -1; > - } > - > - fclose(f); I think you mean bdg->wwid here. -Ben > + strlcpy(buff, bdg->alias, WWID_SIZE); > return 0; > } > > diff --git a/libmultipath/alias.h b/libmultipath/alias.h > index 37b49d9..5ef6720 100644 > --- a/libmultipath/alias.h > +++ b/libmultipath/alias.h > @@ -2,7 +2,7 @@ > #define _ALIAS_H > > int valid_alias(const char *alias); > -int get_user_friendly_wwid(const char *alias, char *buff, const char *file); > +int get_user_friendly_wwid(const char *alias, char *buff); > char *get_user_friendly_alias(const char *wwid, const char *file, > const char *alias_old, > const char *prefix, bool bindings_read_only); > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index 029fbbd..d809490 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -1378,8 +1378,7 @@ static int _get_refwwid(enum mpath_cmds cmd, const char *dev, > refwwid = tmpwwid; > > /* or may be a binding */ > - else if (get_user_friendly_wwid(dev, tmpwwid, > - conf->bindings_file) == 0) > + else if (get_user_friendly_wwid(dev, tmpwwid) == 0) > refwwid = tmpwwid; > > /* or may be an alias */ > -- > 2.41.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote: > On Fri, Sep 01, 2023 at 08:02:31PM +0200, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > Rather than opening the bindings file every time we must retrieve > > a binding, keep the contents in memory and write the file only > > if additions have been made. This simplifies the code, and should > > speed up > > alias lookups significantly. As a side effect, the aliases will be > > stored > > sorted by alias, which changes the way aliases are allocated if > > there are > > unused "holes" in the sequence of aliases. For example, if the > > bindings file > > contains mpathb, mpathy, and mpatha, in this order, the next new > > alias used to > > be mpathz and is now mpathc. > > > > Another side effect is that multipathd will not automatically pick > > up changes > > to the bindings file at runtime without a reconfigure operation. It > > is > > questionable whether these on-the-fly changes were a good idea in > > the first > > place, as inconsistent configurations may easily come to pass. It > > desired, > > it would be feasible to implement automatic update of the bindings > > using the > > existing inotify approach. > > I'm not so much worried about someone manually changing the bindings > file outside of multipath. But it is possible for multipathd to miss > changes made by the multipath command. For instance, say that > someone > has find_multipaths set to "yes" and adds a new device, but only a > single path comes up. They know there will be more paths later, so > they > run > > # multipath <new_path_dev> > > to create a multipath device for this. Multipathd won't pick up this > new binding. If, for some reason the path goes away, and comes back > later, it will now be in the bindings file, but multipathd will still > have no record of the correct binding for it, which already exists in > the bindings file. I don't know if this needs something as complex as > inotify to handle. We could just record the mtime of the bindings > file > when we read it. If it has changed when we call > get_user_friendly_alias() or get_user_friendly_wwid() we would call > check_alias_settings(). > > additional comments below. > > The new implementation of get_user_friendly_alias() is slightly > > different > > than before. The logic is summarized in a comment in the code. Unit > > tests > > will be provided that illustrate the changes. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > libmultipath/alias.c | 369 ++++++++++++++++------------------- > > ---- > > libmultipath/alias.h | 2 +- > > libmultipath/configure.c | 3 +- > > 3 files changed, 157 insertions(+), 217 deletions(-) > > > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c > > index 2f9bc82..6003df0 100644 > > --- a/libmultipath/alias.c > > +++ b/libmultipath/alias.c > > @@ -50,8 +50,6 @@ > > "# alias wwid\n" \ > > "#\n" > > > > -static const char bindings_file_header[] = BINDINGS_FILE_HEADER; > > - > > struct binding { > > char *alias; > > char *wwid; > > @@ -80,6 +78,45 @@ static void _free_binding(struct binding *bdg) > > free(bdg); > > } > > > > +static const struct binding *get_binding_for_alias(const Bindings > > *bindings, > > + const char > > *alias) > > +{ > > + const struct binding *bdg; > > + int i; > > + > > + if (!alias) > > + return NULL; > > + vector_foreach_slot(bindings, bdg, i) { > > + if (!strncmp(bdg->alias, alias, WWID_SIZE)) { > > + condlog(3, "Found matching alias [%s] in > > bindings file." > > + " Setting wwid to %s", alias, bdg- > > >wwid); > > + return bdg; > > + } > > + } > > + > > + condlog(3, "No matching alias [%s] in bindings file.", > > alias); > > + return NULL; > > +} > > + > > +static const struct binding *get_binding_for_wwid(const Bindings > > *bindings, > > + const char *wwid) > > +{ > > + const struct binding *bdg; > > + int i; > > + > > + if (!wwid) > > + return NULL; > > + vector_foreach_slot(bindings, bdg, i) { > > + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) { > > + condlog(3, "Found matching wwid [%s] in > > bindings file." > > + " Setting alias to %s", wwid, bdg- > > >alias); > > + return bdg; > > + } > > + } > > + condlog(3, "No matching wwid [%s] in bindings file.", > > wwid); > > + return NULL; > > +} > > + > > static int add_binding(Bindings *bindings, const char *alias, > > const char *wwid) > > { > > struct binding *bdg; > > @@ -115,6 +152,24 @@ static int add_binding(Bindings *bindings, > > const char *alias, const char *wwid) > > return BINDING_ERROR; > > } > > > > +static int delete_binding(Bindings *bindings, const char *wwid) > > +{ > > + struct binding *bdg; > > + int i; > > + > > + vector_foreach_slot(bindings, bdg, i) { > > + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) { > > + _free_binding(bdg); > > + break; > > + } > > + } > > + if (i >= VECTOR_SIZE(bindings)) > > + return BINDING_NOTFOUND; > > + > > + vector_del_slot(bindings, i); > > + return BINDING_DELETED; > > +} > > + > > static int write_bindings_file(const Bindings *bindings, int fd) > > { > > struct binding *bnd; > > @@ -267,38 +322,15 @@ static bool id_already_taken(int id, const > > char *prefix, const char *map_wwid) > > return alias_already_taken(alias, map_wwid); > > } > > > > -/* > > - * Returns: 0 if matching entry in WWIDs file found > > - * -1 if an error occurs > > - * >0 a free ID that could be used for the WWID at hand > > - * *map_alias is set to a freshly allocated string with the > > matching alias if > > - * the function returns 0, or to NULL otherwise. > > - */ > > -static int > > -lookup_binding(FILE *f, const char *map_wwid, char **map_alias, > > - const char *prefix, int check_if_taken) > > +int get_free_id(const Bindings *bindings, const char *prefix, > > const char *map_wwid) > > { > > - char buf[LINE_MAX]; > > - unsigned int line_nr = 0; > > - int id = 1; > > + const struct binding *bdg; > > + int i, id = 1; > > int biggest_id = 1; > > int smallest_bigger_id = INT_MAX; > > > > - *map_alias = NULL; > > - > > - rewind(f); > > - while (fgets(buf, LINE_MAX, f)) { > > - const char *alias, *wwid; > > - char *c, *saveptr; > > - int curr_id; > > - > > - line_nr++; > > - c = strpbrk(buf, "#\n\r"); > > - if (c) > > - *c = '\0'; > > - alias = strtok_r(buf, " \t", &saveptr); > > - if (!alias) /* blank line */ > > - continue; > > + vector_foreach_slot(bindings, bdg, i) { > > + int curr_id = scan_devname(bdg->alias, prefix); > > If we know that the bindings will be sorted by alias order, we don't > need to do all this. Something like this should work: That's true. Unfortunately, we can't ensure ordering "by alias order". The reason is that our configuration allows using different alias prefixes for different devices. The current sorting is simply alphabetical. It detects duplicates, but it ensures "alias order" only between "mpatha" and "mpathz". I've thought of just removing the "different aliases for different devices" feature. I don't know if any users out there actually set device-specific alias_prefix values in the devices section of multipath.conf. I don't recall having seen such a configuration so far; almost every config I have seen simply uses "mpath" everywhere. But I recognize that it may feel tempting in some cases. One could use the "NETAPP" prefix for some arrays and the "EMC" prefix for others, for example, making it easier to see which is what. And we simply don't know if we'd break existing user setups with such a change. So if at all, we can't do it in a minor release without deprecating it first. When we add a binding in add_binding(), we don't know which alias_prefix is configured for a given WWID, and we have no easy way to figure it out. We know the alias, but we don't know which portion of it is the prefix and which is the ID (it's not forbidden to use "aaaa" as alias_prefix). I wouldn't want to start guessing. If you can think of a way to keep the bindings cleanly sorted, please let me know. > > if (curr_id <= 0) > continue; > > while (id < curr_id) { > if (!id_already_taken(id, prefix, map_wwid)) > return id; > id++; > } > if (id == INT_MAX) > break; > id++; > } > if (id == INT_MAX) > condlog(0, "no more available user_friendly_names"); > return id < INT_MAX ? id : -1; > } > > > > /* > > * Find an unused index - explanation of the > > algorithm > > @@ -333,8 +365,6 @@ lookup_binding(FILE *f, const char *map_wwid, > > char **map_alias, > > * biggest_id is always > smallest_bigger_id, > > except in the > > * "perfectly ordered" case. > > */ > > - > > - curr_id = scan_devname(alias, prefix); > > if (curr_id == id) { > > if (id < INT_MAX) > > id++; > > @@ -345,36 +375,15 @@ lookup_binding(FILE *f, const char *map_wwid, > > char **map_alias, > > } > > if (curr_id > biggest_id) > > biggest_id = curr_id; > > + > > if (curr_id > id && curr_id < smallest_bigger_id) > > smallest_bigger_id = curr_id; > > - wwid = strtok_r(NULL, " \t", &saveptr); > > - if (!wwid){ > > - condlog(3, > > - "Ignoring malformed line %u in > > bindings file", > > - line_nr); > > - continue; > > - } > > - if (strcmp(wwid, map_wwid) == 0){ > > - condlog(3, "Found matching wwid [%s] in > > bindings file." > > - " Setting alias to %s", wwid, > > alias); > > - *map_alias = strdup(alias); > > - if (*map_alias == NULL) { > > - condlog(0, "Cannot copy alias from > > bindings " > > - "file: out of memory"); > > - return -1; > > - } > > - return 0; > > - } > > } > > - if (!prefix && check_if_taken) > > - id = -1; > > - if (id >= smallest_bigger_id) { > > - if (biggest_id < INT_MAX) > > - id = biggest_id + 1; > > - else > > - id = -1; > > - } > > - if (id > 0 && check_if_taken) { > > + > > + if (id >= smallest_bigger_id) > > + id = biggest_id < INT_MAX ? biggest_id + 1 : -1; > > + > > + if (id > 0) { > > while(id_already_taken(id, prefix, map_wwid)) { > > if (id == INT_MAX) { > > id = -1; > > @@ -391,63 +400,16 @@ lookup_binding(FILE *f, const char *map_wwid, > > char **map_alias, > > } > > } > > } > > - if (id < 0) { > > + > > + if (id < 0) > > condlog(0, "no more available > > user_friendly_names"); > > - return -1; > > - } else > > - condlog(3, "No matching wwid [%s] in bindings > > file.", map_wwid); > > return id; > > } > > > > -static int > > -rlookup_binding(FILE *f, char *buff, const char *map_alias) > > -{ > > - char line[LINE_MAX]; > > - unsigned int line_nr = 0; > > - > > - buff[0] = '\0'; > > - > > - while (fgets(line, LINE_MAX, f)) { > > - char *c, *saveptr; > > - const char *alias, *wwid; > > - > > - line_nr++; > > - c = strpbrk(line, "#\n\r"); > > - if (c) > > - *c = '\0'; > > - alias = strtok_r(line, " \t", &saveptr); > > - if (!alias) /* blank line */ > > - continue; > > - wwid = strtok_r(NULL, " \t", &saveptr); > > - if (!wwid){ > > - condlog(3, > > - "Ignoring malformed line %u in > > bindings file", > > - line_nr); > > - continue; > > - } > > - if (strlen(wwid) > WWID_SIZE - 1) { > > - condlog(3, > > - "Ignoring too large wwid at %u in > > bindings file", line_nr); > > - continue; > > - } > > - if (strcmp(alias, map_alias) == 0){ > > - condlog(3, "Found matching alias [%s] in > > bindings file." > > - " Setting wwid to %s", alias, > > wwid); > > - strlcpy(buff, wwid, WWID_SIZE); > > - return 0; > > - } > > - } > > - condlog(3, "No matching alias [%s] in bindings file.", > > map_alias); > > - > > - return -1; > > -} > > - > > static char * > > -allocate_binding(int fd, const char *wwid, int id, const char > > *prefix) > > +allocate_binding(const char *filename, const char *wwid, int id, > > const char *prefix) > > { > > STRBUF_ON_STACK(buf); > > - off_t offset; > > - ssize_t len; > > char *alias, *c; > > > > if (id <= 0) { > > @@ -460,29 +422,22 @@ allocate_binding(int fd, const char *wwid, > > int id, const char *prefix) > > format_devname(&buf, id) == -1) > > return NULL; > > > > - if (print_strbuf(&buf, " %s\n", wwid) < 0) > > - return NULL; > > - > > - offset = lseek(fd, 0, SEEK_END); > > - if (offset < 0){ > > - condlog(0, "Cannot seek to end of bindings file : > > %s", > > - strerror(errno)); > > - return NULL; > > - } > > - > > - len = get_strbuf_len(&buf); > > alias = steal_strbuf_str(&buf); > > > > - if (write(fd, alias, len) != len) { > > - condlog(0, "Cannot write binding to bindings file : > > %s", > > - strerror(errno)); > > - /* clear partial write */ > > - if (ftruncate(fd, offset)) > > - condlog(0, "Cannot truncate the header : > > %s", > > - strerror(errno)); > > + if (add_binding(&global_bindings, alias, wwid) != > > BINDING_ADDED) { > > + condlog(0, "%s: cannot allocate new binding %s for > > %s", > > + __func__, alias, wwid); > > free(alias); > > return NULL; > > } > > + > > + if (update_bindings_file(&global_bindings, filename) == -1) > > { > > + condlog(1, "%s: deleting binding %s for %s", > > __func__, alias, wwid); > > + delete_binding(&global_bindings, wwid); > > + free(alias); > > + return NULL; > > + } > > + > > We no longer need to end the alias at the space, since we're not > printing the wwid in the buffer. Right, thanks. This makes me realize that we don't sanity-check the alias prefix, but that's a different issue. > > > c = strchr(alias, ' '); > > if (c) > > *c = '\0'; > > @@ -491,144 +446,130 @@ allocate_binding(int fd, const char *wwid, > > int id, const char *prefix) > > return alias; > > } > > > > +/* > > + * get_user_friendly_alias() action table > > + * > > + * The table shows the various cases, the actions taken, and the > > CI > > + * functions from tests/alias.c that represent them. > > + * > > + * - O: old alias given > > + * - A: old alias in table (y: yes, correct WWID; X: yes, wrong > > WWID) > > + * - W: wwid in table > > + * - U: old alias is used > > + * > > + * | No | O | A | W | U | > > action | function > > gufa_X | > > + * |----+---+---+---+---+---------------------------------------- > > ----+------------------------------| > > + * | 1 | n | - | n | - | get new > > alias | nomatch_Y | > > + * | 2 | n | - | y | - | use alias from > > bindings | match_a_Y | > > + * | 3 | y | n | n | n | add binding for old > > alias | old_nomatch_nowwidmatch | > > + * | 4 | y | n | n | y | error, no alias (can't add > > entry) | old_nomatch_nowwidmatch_used | > > + * | 5 | y | n | y | - | use alias from bindings (avoid > > duplicates) | old_nomatch_wwidmatch | > > + * | 6 | y | y | n | - | [ impossible > > ] | - | > > + * | 7 | y | y | y | n | use old alias == alias from > > bindings | old_match | > > + * | 8 | y | y | y | y | error, no alias (would be > > duplicate) | old_match_used | > > + * | 9 | y | X | n | - | get new > > alias | old_match_other | > > + * | 10 | y | X | y | - | use alias from > > bindings | old_match_other_wwidmatch | > > + * > > + * Notes: > > + * - "use alias from bindings" means that the alias from the > > bindings file will > > + * be tried; if it is in use, the alias selection will fail. > > No other > > + * bindings will be attempted. > > + * - "get new alias" fails if all aliases are used up, or if > > writing the > > + * bindings file fails. > > + */ > > + > > char *get_user_friendly_alias(const char *wwid, const char *file, > > const char *alias_old, > > const char *prefix, bool > > bindings_read_only) > > { > > char *alias = NULL; > > int id = 0; > > - int fd, can_write; > > bool new_binding = false; > > - char buff[WWID_SIZE]; > > - FILE *f; > > + bool old_alias_taken = false; > > + const struct binding *bdg; > > > > - fd = open_file(file, &can_write, bindings_file_header); > > - if (fd < 0) > > - return NULL; > > - > > - f = fdopen(fd, "r"); > > - if (!f) { > > - condlog(0, "cannot fdopen on bindings file > > descriptor"); > > - close(fd); > > - return NULL; > > - } > > - > > - if (!strlen(alias_old)) > > + if (!*alias_old) > > goto new_alias; > > > > - /* lookup the binding. if it exists, the wwid will be in > > buff > > - * either way, id contains the id for the alias > > - */ > > - rlookup_binding(f, buff, alias_old); > > - > > - if (strlen(buff) > 0) { > > - /* if buff is our wwid, it's already > > - * allocated correctly > > - */ > > - if (strcmp(buff, wwid) == 0) { > > + /* See if there's a binding matching both alias_old and > > wwid */ > > + bdg = get_binding_for_alias(&global_bindings, alias_old); > > + if (bdg) { > > + if (!strcmp(bdg->wwid, wwid)) { > > I'm still not convinced that it is possible for mpp->alias_old to be > set > when there isn't a multipath device with the alias_old and the > desired > wwid. Unless I'm missing something we should be able to skip the > alias_already_taken() check. Let's follow up this discussion in the 4/21 thread. > > > if (!alias_already_taken(alias_old, wwid)) > > alias = strdup(alias_old); > > else > > condlog(0, "ERROR: old alias [%s] > > for wwid [%s] is used by other map", > > alias_old, wwid); > > goto out; > > - > > } else { > > condlog(0, "alias %s already bound to wwid > > %s, cannot reuse", > > - alias_old, buff); > > - goto new_alias; ; > > + alias_old, bdg->wwid); > > + goto new_alias; > > } > > } > > > > - /* > > - * Look for an existing alias in the bindings file. > > - * Pass prefix = NULL, so lookup_binding() won't try to > > allocate a new id. > > - */ > > - id = lookup_binding(f, wwid, &alias, NULL, 0); > > - if (alias) { > > - if (alias_already_taken(alias, wwid)) { > > - free(alias); > > - alias = NULL; > > - } else > > - condlog(3, "Use existing binding [%s] for > > WWID [%s]", > > - alias, wwid); > > - goto out; > > - } > > - > > /* allocate the existing alias in the bindings file */ > > - id = scan_devname(alias_old, prefix); > > - if (id > 0 && id_already_taken(id, prefix, wwid)) { > again, I think it's safe to skip this check, since we're checking > alias_old. > > + if (alias_already_taken(alias_old, wwid)) { > > condlog(0, "ERROR: old alias [%s] for wwid [%s] is > > used by other map", > > alias_old, wwid); > > + old_alias_taken = true; > > + } else > > + id = scan_devname(alias_old, prefix); > > + > > +new_alias: > > + /* Check for existing binding of WWID */ > > + bdg = get_binding_for_wwid(&global_bindings, wwid); > > + if (bdg) { > > + if (!alias_already_taken(bdg->alias, wwid)) { > > + condlog(3, "Use existing binding [%s] for > > WWID [%s]", > > + bdg->alias, wwid); > > + alias = strdup(bdg->alias); > > + } > > goto out; > > } > > > > -new_alias: > > - if (id <= 0) { > > + /* > > + * old_alias_taken means that the WWID is not in the > > bindings file, but > > + * alias_old is currently taken by a different WWID. We > > shouldn't added > > + * a new binding in this case. > > + */ > > + if (id <= 0 && !old_alias_taken) { > > /* > > * no existing alias was provided, or allocating it > > * failed. Try a new one. > > */ > > - id = lookup_binding(f, wwid, &alias, prefix, 1); > > - if (id == 0 && alias_already_taken(alias, wwid)) { > > - free(alias); > > - alias = NULL; > > - } > > + id = get_free_id(&global_bindings, prefix, wwid); > > if (id <= 0) > > goto out; > > else > > new_binding = true; > > } > > > > - if (fflush(f) != 0) { > > - condlog(0, "cannot fflush bindings file stream : > > %s", > > - strerror(errno)); > > - goto out; > > - } > > + if (!bindings_read_only && id > 0) > > + alias = allocate_binding(file, wwid, id, prefix); > > > > - if (can_write && !bindings_read_only) { > > - alias = allocate_binding(fd, wwid, id, prefix); > > - if (alias && !new_binding) > > - condlog(2, "Allocated existing binding [%s] > > for WWID [%s]", > > - alias, wwid); > > - } > > + if (alias && !new_binding) > > + condlog(2, "Allocated existing binding [%s] for > > WWID [%s]", > > + alias, wwid); > > > > out: > > - pthread_cleanup_push(free, alias); > > - fclose(f); > > - pthread_cleanup_pop(0); > > return alias; > > } > > > > -int > > -get_user_friendly_wwid(const char *alias, char *buff, const char > > *file) > > +int get_user_friendly_wwid(const char *alias, char *buff) > > { > > - int fd, unused; > > - FILE *f; > > + const struct binding *bdg; > > > > if (!alias || *alias == '\0') { > > condlog(3, "Cannot find binding for empty alias"); > > return -1; > > } > > > > - fd = open_file(file, &unused, bindings_file_header); > > - if (fd < 0) > > - return -1; > > - > > - f = fdopen(fd, "r"); > > - if (!f) { > > - condlog(0, "cannot fdopen on bindings file > > descriptor : %s", > > - strerror(errno)); > > - close(fd); > > + bdg = get_binding_for_alias(&global_bindings, alias); > > + if (!bdg) { > > + *buff = '\0'; > > return -1; > > } > > - > > - rlookup_binding(f, buff, alias); > > - if (!strlen(buff)) { > > - fclose(f); > > - return -1; > > - } > > - > > - fclose(f); > I think you mean bdg->wwid here. > Argh, thanks for spotting this. Martin > -Ben > > > + strlcpy(buff, bdg->alias, WWID_SIZE); > > return 0; > > } > > > > diff --git a/libmultipath/alias.h b/libmultipath/alias.h > > index 37b49d9..5ef6720 100644 > > --- a/libmultipath/alias.h > > +++ b/libmultipath/alias.h > > @@ -2,7 +2,7 @@ > > #define _ALIAS_H > > > > int valid_alias(const char *alias); > > -int get_user_friendly_wwid(const char *alias, char *buff, const > > char *file); > > +int get_user_friendly_wwid(const char *alias, char *buff); > > char *get_user_friendly_alias(const char *wwid, const char *file, > > const char *alias_old, > > const char *prefix, bool > > bindings_read_only); > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > > index 029fbbd..d809490 100644 > > --- a/libmultipath/configure.c > > +++ b/libmultipath/configure.c > > @@ -1378,8 +1378,7 @@ static int _get_refwwid(enum mpath_cmds cmd, > > const char *dev, > > refwwid = tmpwwid; > > > > /* or may be a binding */ > > - else if (get_user_friendly_wwid(dev, tmpwwid, > > - conf- > > >bindings_file) == 0) > > + else if (get_user_friendly_wwid(dev, tmpwwid) == 0) > > refwwid = tmpwwid; > > > > /* or may be an alias */ > > -- > > 2.41.0 > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 07, 2023 at 12:30:53PM +0200, Martin Wilck wrote: > On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote: > > On Fri, Sep 01, 2023 at 08:02:31PM +0200, mwilck@suse.com wrote: > > > From: Martin Wilck <mwilck@suse.com> > > > > > > + vector_foreach_slot(bindings, bdg, i) { > > > + int curr_id = scan_devname(bdg->alias, prefix); > > > > If we know that the bindings will be sorted by alias order, we don't > > need to do all this. Something like this should work: > > That's true. Unfortunately, we can't ensure ordering "by alias order". > The reason is that our configuration allows using different alias > prefixes for different devices. The current sorting is simply > alphabetical. It detects duplicates, but it ensures "alias order" only > between "mpatha" and "mpathz". > > I've thought of just removing the "different aliases for different > devices" feature. I don't know if any users out there actually set > device-specific alias_prefix values in the devices section of > multipath.conf. I don't recall having seen such a configuration so far; > almost every config I have seen simply uses "mpath" everywhere. But I > recognize that it may feel tempting in some cases. One could use the > "NETAPP" prefix for some arrays and the "EMC" prefix for others, for > example, making it easier to see which is what. And we simply don't > know if we'd break existing user setups with such a change. So if at > all, we can't do it in a minor release without deprecating it first. > > When we add a binding in add_binding(), we don't know which > alias_prefix is configured for a given WWID, and we have no easy way to > figure it out. We know the alias, but we don't know which portion of it > is the prefix and which is the ID (it's not forbidden to use "aaaa" as > alias_prefix). I wouldn't want to start guessing. > > If you can think of a way to keep the bindings cleanly sorted, please > let me know. Doh! You even mentioned this issue in your "fix alias tests" commit. I just didn't pay enough attention to it. But I believe there is a way to make sure that we are dealing with a correctly sorted list of potential bindings when calling get_free_id(), if we decide it's worth the extra work. The global_bindings isn't guaranteed to have all the bindings for our prefix correctly sorted, but they will all be in a group. If we wanted, we could create a new vector that just included these bindings, and sort it using the current prefix and a comparison function to do alias-sytle sorting. the msort_r() function takes an extra argument to pass to the compar() function, which could be the prefix. It would be great if we could just sort part of the global_bindings vector in place, once we knew the prefix. Unfortunately, that would only work if we could know that no prefix can ever match a starting substring of another prefix. Otherwise, sorting using one can make the other not be in a single continuous group. For instance, if some device configs used "mpatha" as a prefix, they would expect "mpathaaa" to follow immediately after "mapthaz", but if the bindings file had already been sorted using the "mpath" prefix, then "mpathba" would follow "mpathaz", and "mpathaaa" would come much later. Keeping the global file alphabetically sorted guarantees that no matter the prefix that devices were added under, all device aliases that are valid with that prefix will be in a group together. So, is it worth it to make another vector, copy the alises which are valid with the current prefix into it, and then sort it? get_free_id() will be cleaner, and gap detection won't break down after you get past mpathaa, which it currently does with an alpahbetically sorted bindings list. Alternatively, we could just decide that setting a prefix in a device config that matches the starting substring of a another prefix is a config error (obviously using the exact same prefix is fine), and ignore that prefix from the device config when we parse the configuration. Then we could read in the bindings alphabetically like we currently do, find the prefix groups in them, and sort them once, in-place. When allocating a binding, we would need to search backwards through the bindings till we found the end of the group matching our prefix (or an alias that comes alphabetically before our prefix). Then we would have to search backwards through our prefix group using the id, till we found a binding with a smaller id. Thoughts? -Ben > > > > if (curr_id <= 0) > > continue; > > > > while (id < curr_id) { > > if (!id_already_taken(id, prefix, map_wwid)) > > return id; > > id++; > > } > > if (id == INT_MAX) > > break; > > id++; > > } > > if (id == INT_MAX) > > condlog(0, "no more available user_friendly_names"); > > return id < INT_MAX ? id : -1; > > } -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 07, 2023 at 02:14:04PM -0500, Benjamin Marzinski wrote: > On Thu, Sep 07, 2023 at 12:30:53PM +0200, Martin Wilck wrote: > > On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote: > > > On Fri, Sep 01, 2023 at 08:02:31PM +0200, mwilck@suse.com wrote: > > > > From: Martin Wilck <mwilck@suse.com> > > > > > > > > > + vector_foreach_slot(bindings, bdg, i) { > > > > + int curr_id = scan_devname(bdg->alias, prefix); > > > > > > If we know that the bindings will be sorted by alias order, we don't > > > need to do all this. Something like this should work: > > > > That's true. Unfortunately, we can't ensure ordering "by alias order". > > The reason is that our configuration allows using different alias > > prefixes for different devices. The current sorting is simply > > alphabetical. It detects duplicates, but it ensures "alias order" only > > between "mpatha" and "mpathz". > > > > I've thought of just removing the "different aliases for different > > devices" feature. I don't know if any users out there actually set > > device-specific alias_prefix values in the devices section of > > multipath.conf. I don't recall having seen such a configuration so far; > > almost every config I have seen simply uses "mpath" everywhere. But I > > recognize that it may feel tempting in some cases. One could use the > > "NETAPP" prefix for some arrays and the "EMC" prefix for others, for > > example, making it easier to see which is what. And we simply don't > > know if we'd break existing user setups with such a change. So if at > > all, we can't do it in a minor release without deprecating it first. > > > > When we add a binding in add_binding(), we don't know which > > alias_prefix is configured for a given WWID, and we have no easy way to > > figure it out. We know the alias, but we don't know which portion of it > > is the prefix and which is the ID (it's not forbidden to use "aaaa" as > > alias_prefix). I wouldn't want to start guessing. > > > > If you can think of a way to keep the bindings cleanly sorted, please > > let me know. > > Doh! You even mentioned this issue in your "fix alias tests" commit. I > just didn't pay enough attention to it. But I believe there is a way to > make sure that we are dealing with a correctly sorted list of potential > bindings when calling get_free_id(), if we decide it's worth the extra > work. > > The global_bindings isn't guaranteed to have all the bindings for our > prefix correctly sorted, but they will all be in a group. If we wanted, > we could create a new vector that just included these bindings, and sort > it using the current prefix and a comparison function to do alias-sytle > sorting. the msort_r() function takes an extra argument to pass to the > compar() function, which could be the prefix. > > It would be great if we could just sort part of the global_bindings > vector in place, once we knew the prefix. Unfortunately, that would > only work if we could know that no prefix can ever match a starting > substring of another prefix. Otherwise, sorting using one can make the > other not be in a single continuous group. For instance, if some device > configs used "mpatha" as a prefix, they would expect "mpathaaa" to > follow immediately after "mapthaz", but if the bindings file had already > been sorted using the "mpath" prefix, then "mpathba" would follow > "mpathaz", and "mpathaaa" would come much later. Keeping the global file > alphabetically sorted guarantees that no matter the prefix that devices > were added under, all device aliases that are valid with that prefix > will be in a group together. > > So, is it worth it to make another vector, copy the alises which are > valid with the current prefix into it, and then sort it? get_free_id() > will be cleaner, and gap detection won't break down after you get past > mpathaa, which it currently does with an alpahbetically sorted bindings > list. > > Alternatively, we could just decide that setting a prefix in a device > config that matches the starting substring of a another prefix is a > config error (obviously using the exact same prefix is fine), and ignore > that prefix from the device config when we parse the configuration. Then > we could read in the bindings alphabetically like we currently do, find > the prefix groups in them, and sort them once, in-place. When allocating > a binding, we would need to search backwards through the bindings till > we found the end of the group matching our prefix (or an alias that > comes alphabetically before our prefix). Then we would have to search > backwards through our prefix group using the id, till we found a binding > with a smaller id. So, thinking about this a little more, assuming we would be writing out the bindings file in full alias sorted order, it's stupid to sort it alphabetically, and then resort it back to alias order. We should obviously read it in using the same method as when allocating a binding. Where you search backwards till you find your prefix group, and then find the proper spot in the prefix group using alias sorting. Aliases that aren't valid for any configured prefix (these could have been added by hand to the bindings file, or possibly they were created with a different multipath configuration) will just get sorted alphabetically into the bindings list, between the prefix groups. Also, it's not enough to just ignore any device config prefix that is a starting substring of another prefix. The device config prefixes also cannot be superstrings of the default prefix, so if the default prefix is "mpath", both of the device config prefixes "mp" and "mpathdev" would need to be ignored. -Ben > Thoughts? > > -Ben > > > > > > > if (curr_id <= 0) > > > continue; > > > > > > while (id < curr_id) { > > > if (!id_already_taken(id, prefix, map_wwid)) > > > return id; > > > id++; > > > } > > > if (id == INT_MAX) > > > break; > > > id++; > > > } > > > if (id == INT_MAX) > > > condlog(0, "no more available user_friendly_names"); > > > return id < INT_MAX ? id : -1; > > > } -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, 2023-09-07 at 15:02 -0500, Benjamin Marzinski wrote: > On Thu, Sep 07, 2023 at 02:14:04PM -0500, Benjamin Marzinski wrote: > > On Thu, Sep 07, 2023 at 12:30:53PM +0200, Martin Wilck wrote: > > > On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote: > > > > On Fri, Sep 01, 2023 at 08:02:31PM +0200, > > > > mwilck@suse.com wrote: > > > > > From: Martin Wilck <mwilck@suse.com> > > > > > > > > > > > > + vector_foreach_slot(bindings, bdg, i) { > > > > > + int curr_id = scan_devname(bdg->alias, > > > > > prefix); > > > > > > > > If we know that the bindings will be sorted by alias order, we > > > > don't > > > > need to do all this. Something like this should work: > > > > > > That's true. Unfortunately, we can't ensure ordering "by alias > > > order". > > > The reason is that our configuration allows using different alias > > > prefixes for different devices. The current sorting is simply > > > alphabetical. It detects duplicates, but it ensures "alias order" > > > only > > > between "mpatha" and "mpathz". > > > > > > I've thought of just removing the "different aliases for > > > different > > > devices" feature. I don't know if any users out there actually > > > set > > > device-specific alias_prefix values in the devices section of > > > multipath.conf. I don't recall having seen such a configuration > > > so far; > > > almost every config I have seen simply uses "mpath" everywhere. > > > But I > > > recognize that it may feel tempting in some cases. One could use > > > the > > > "NETAPP" prefix for some arrays and the "EMC" prefix for others, > > > for > > > example, making it easier to see which is what. And we simply > > > don't > > > know if we'd break existing user setups with such a change. So if > > > at > > > all, we can't do it in a minor release without deprecating it > > > first. > > > > > > When we add a binding in add_binding(), we don't know which > > > alias_prefix is configured for a given WWID, and we have no easy > > > way to > > > figure it out. We know the alias, but we don't know which portion > > > of it > > > is the prefix and which is the ID (it's not forbidden to use > > > "aaaa" as > > > alias_prefix). I wouldn't want to start guessing. > > > > > > If you can think of a way to keep the bindings cleanly sorted, > > > please > > > let me know. > > > > Doh! You even mentioned this issue in your "fix alias tests" > > commit. I > > just didn't pay enough attention to it. But I believe there is a > > way to > > make sure that we are dealing with a correctly sorted list of > > potential > > bindings when calling get_free_id(), if we decide it's worth the > > extra > > work. > > > > The global_bindings isn't guaranteed to have all the bindings for > > our > > prefix correctly sorted, but they will all be in a group. If we > > wanted, > > we could create a new vector that just included these bindings, and > > sort > > it using the current prefix and a comparison function to do alias- > > sytle > > sorting. the msort_r() function takes an extra argument to pass to > > the > > compar() function, which could be the prefix. > > > > It would be great if we could just sort part of the global_bindings > > vector in place, once we knew the prefix. Unfortunately, that > > would > > only work if we could know that no prefix can ever match a starting > > substring of another prefix. Otherwise, sorting using one can make > > the > > other not be in a single continuous group. For instance, if some > > device > > configs used "mpatha" as a prefix, they would expect "mpathaaa" to > > follow immediately after "mapthaz", but if the bindings file had > > already > > been sorted using the "mpath" prefix, then "mpathba" would follow > > "mpathaz", and "mpathaaa" would come much later. Keeping the global > > file > > alphabetically sorted guarantees that no matter the prefix that > > devices > > were added under, all device aliases that are valid with that > > prefix > > will be in a group together. > > > > So, is it worth it to make another vector, copy the alises which > > are > > valid with the current prefix into it, and then sort it? > > get_free_id() > > will be cleaner, and gap detection won't break down after you get > > past > > mpathaa, which it currently does with an alpahbetically sorted > > bindings > > list. > > > > Alternatively, we could just decide that setting a prefix in a > > device > > config that matches the starting substring of a another prefix is a > > config error (obviously using the exact same prefix is fine), and > > ignore > > that prefix from the device config when we parse the configuration. > > Then > > we could read in the bindings alphabetically like we currently do, > > find > > the prefix groups in them, and sort them once, in-place. When > > allocating > > a binding, we would need to search backwards through the bindings > > till > > we found the end of the group matching our prefix (or an alias that > > comes alphabetically before our prefix). Then we would have to > > search > > backwards through our prefix group using the id, till we found a > > binding > > with a smaller id. > > So, thinking about this a little more, assuming we would be writing > out > the bindings file in full alias sorted order, it's stupid to sort it > alphabetically, and then resort it back to alias order. We should > obviously read it in using the same method as when allocating a > binding. > Where you search backwards till you find your prefix group, and then > find the proper spot in the prefix group using alias sorting. > Aliases > that aren't valid for any configured prefix (these could have been > added > by hand to the bindings file, or possibly they were created with a > different multipath configuration) will just get sorted > alphabetically > into the bindings list, between the prefix groups. > > Also, it's not enough to just ignore any device config prefix that is > a > starting substring of another prefix. The device config prefixes also > cannot be superstrings of the default prefix, so if the default > prefix > is "mpath", both of the device config prefixes "mp" and "mpathdev" > would > need to be ignored. I don't quite understand yet. Assume we read the bindings file and encounter an alias "mpathab". Is this alias #28 with prefix "mpath", or is it #2 with prefix "mpatha", or perhaps alias guess-what (order "thab") with prefix "mpa"? How would we know at this point in the code? Are you suggesting that we create a list of all configured prefixes and compare the bindings we read with each of them? Our bindings list is now partially sorted, which is an improvement wrt the previous situation. "missing the gap" is not really an awful problem [*]. Perhaps we could postpone this for after this patch set, and give it some more time to sink in? Martin [*] I admit that with my patch, we _know_ now that the bindings list will be sub-optimally sorted as soon as mpathaa is reached, whereas before the ordering might be perfect even with a large number of aliases, depending on the history of the bindings file. That's not a change for the better; it will cause the gap to be missed in some situations where we don't miss it now. I am not sure how bad this is. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Sep 07, 2023 at 10:43:27PM +0200, Martin Wilck wrote: > On Thu, 2023-09-07 at 15:02 -0500, Benjamin Marzinski wrote: > > On Thu, Sep 07, 2023 at 02:14:04PM -0500, Benjamin Marzinski wrote: > > > On Thu, Sep 07, 2023 at 12:30:53PM +0200, Martin Wilck wrote: > > > > On Wed, 2023-09-06 at 17:47 -0500, Benjamin Marzinski wrote: > > > > > On Fri, Sep 01, 2023 at 08:02:31PM +0200, > > > > > mwilck@suse.com wrote: > > > > > > From: Martin Wilck <mwilck@suse.com> > > > Doh! You even mentioned this issue in your "fix alias tests" > > > commit. I > > > just didn't pay enough attention to it. But I believe there is a > > > way to > > > make sure that we are dealing with a correctly sorted list of > > > potential > > > bindings when calling get_free_id(), if we decide it's worth the > > > extra > > > work. > > > > > > The global_bindings isn't guaranteed to have all the bindings for > > > our > > > prefix correctly sorted, but they will all be in a group. If we > > > wanted, > > > we could create a new vector that just included these bindings, and > > > sort > > > it using the current prefix and a comparison function to do alias- > > > sytle > > > sorting. the msort_r() function takes an extra argument to pass to > > > the > > > compar() function, which could be the prefix. > > > > > > It would be great if we could just sort part of the global_bindings > > > vector in place, once we knew the prefix. Unfortunately, that > > > would > > > only work if we could know that no prefix can ever match a starting > > > substring of another prefix. Otherwise, sorting using one can make > > > the > > > other not be in a single continuous group. For instance, if some > > > device > > > configs used "mpatha" as a prefix, they would expect "mpathaaa" to > > > follow immediately after "mapthaz", but if the bindings file had > > > already > > > been sorted using the "mpath" prefix, then "mpathba" would follow > > > "mpathaz", and "mpathaaa" would come much later. Keeping the global > > > file > > > alphabetically sorted guarantees that no matter the prefix that > > > devices > > > were added under, all device aliases that are valid with that > > > prefix > > > will be in a group together. > > > > > > So, is it worth it to make another vector, copy the alises which > > > are > > > valid with the current prefix into it, and then sort it? > > > get_free_id() > > > will be cleaner, and gap detection won't break down after you get > > > past > > > mpathaa, which it currently does with an alpahbetically sorted > > > bindings > > > list. > > > > > > Alternatively, we could just decide that setting a prefix in a > > > device > > > config that matches the starting substring of a another prefix is a > > > config error (obviously using the exact same prefix is fine), and > > > ignore > > > that prefix from the device config when we parse the configuration. > > > Then > > > we could read in the bindings alphabetically like we currently do, > > > find > > > the prefix groups in them, and sort them once, in-place. When > > > allocating > > > a binding, we would need to search backwards through the bindings > > > till > > > we found the end of the group matching our prefix (or an alias that > > > comes alphabetically before our prefix). Then we would have to > > > search > > > backwards through our prefix group using the id, till we found a > > > binding > > > with a smaller id. > > > > So, thinking about this a little more, assuming we would be writing > > out > > the bindings file in full alias sorted order, it's stupid to sort it > > alphabetically, and then resort it back to alias order. We should > > obviously read it in using the same method as when allocating a > > binding. > > Where you search backwards till you find your prefix group, and then > > find the proper spot in the prefix group using alias sorting. > > Aliases > > that aren't valid for any configured prefix (these could have been > > added > > by hand to the bindings file, or possibly they were created with a > > different multipath configuration) will just get sorted > > alphabetically > > into the bindings list, between the prefix groups. > > > > Also, it's not enough to just ignore any device config prefix that is > > a > > starting substring of another prefix. The device config prefixes also > > cannot be superstrings of the default prefix, so if the default > > prefix > > is "mpath", both of the device config prefixes "mp" and "mpathdev" > > would > > need to be ignored. > > I don't quite understand yet. Assume we read the bindings file and > encounter an alias "mpathab". Is this alias #28 with prefix "mpath", or > is it #2 with prefix "mpatha", or perhaps alias guess-what (order > "thab") with prefix "mpa"? How would we know at this point in the code? > Are you suggesting that we create a list of all configured prefixes and > compare the bindings we read with each of them? So, the crux of my idea is that we disallow setting the alias_prefix to conflicting values like this in the config. After we parse the configuration, we have to go through all the alias_prefix settings and sanitize them. If there's a alias_prefix in the overrides section, there's nothing to do, since all devices will use the same prefix. Otherwise, we need to make sure that no prefixes in the devices section are either starting substrings or starting superstrings of the default alias prefix. Additionally, we need to make sure that no devices section alias prefixes are starting substrings of another devices section alias prefix. If there is an invalid devices section alias_prefix, we need to print an error message and ignore it. For instance, say that the default prefix is "mpath". If there was a devices section prefix that was "mp" or "mpathdev", we would print a error and simply ignore that setting, since "mp" is a starting substring of the default prefix, and "mpathdev" is a staring superstring of the default prefix. Devices of that type would just use the default "mpath" prefix. Similarly, if there were two separate device section configs that had alias prefixes of "emc" and "emcdev", we would ignore the "emc" prefix, since it is a staring substring of "emcdev" (or we could choose to ignore the superstring prefixes for conflicts between two device section configs, it doesn't really matter which one we ignore). I realize that this is a new limitiation we are adding to multipath. Users were previously able to have device specific alias prefixes of "mp", for instance. But I would argue that we don't need to support conflicting prefixes. Obviously, distrubutions may want to avoid adding this change until they are able to safely make non-backwards compatible changs. But I have pretty strong doubts that this change would negatively effect anyone. After parsing the config file, we will have a list of valid prefixes that do not conflict. Thus if strncmp(bdg->alias, prefix, strlen(prefix)) matches for one of the prefixes, it will not match for any other. Armed with this list of valid prefixes, when we add bindings to our list, we are able to sort them in alias order. Now, when we read the the bindings from the bindings file, it's possible that some of them will not match any of our valid prefixes, but that's fine. If an alias doesn't match a valid prefix, we won't sort it in alias order, because we can't. But since we only give out aliases that use the valid prefixes, we will never need to search through those non-matching aliases to find a free one. So we don't care how they are ordered, just so long as they don't come in the middle of a valid prefix group. To add a new binding, we first figure out if its alias matches a valid prefix. If it doesn't, we simply scan backwards through our existing list of bindings till we find an alias that comes alphabetically before it, and add the binding after that. If it does match a valid prefix, we scan backwards looking for the first alias that matches the prefix or is before it alphabetically. If we find a binding whose alias comes before it, then there are no devices with this prefix, and we add the new binding after it. If we find a binding with an alias whose start matches this prefix, we found our prefix group. Then we start searching backwards by ID until we either find a binding with an ID smaller than ours or we find a binding whose alias doesn't match our prefix, and we add the binding after it. > Our bindings list is now partially sorted, which is an improvement wrt > the previous situation. "missing the gap" is not really an awful > problem [*]. Perhaps we could postpone this for after this patch set, > and give it some more time to sink in? Yep. I'm fine with going ahead with this patchset as it is. Both sorting the bindings in alias order and updating the bindings if /etc/multipath/bindings has changed are things that can get looked at afterwards. And I'm fine with doing this work, if you want. -Ben > Martin > > [*] I admit that with my patch, we _know_ now that the bindings list > will be sub-optimally sorted as soon as mpathaa is reached, whereas > before the ordering might be perfect even with a large number of > aliases, depending on the history of the bindings file. That's not a > change for the better; it will cause the gap to be missed in some > situations where we don't miss it now. I am not sure how bad this is. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Fri, 2023-09-08 at 12:22 -0500, Benjamin Marzinski wrote: > On Thu, Sep 07, 2023 at 10:43:27PM +0200, Martin Wilck wrote: > > Our bindings list is now partially sorted, which is an improvement > > wrt > > the previous situation. "missing the gap" is not really an awful > > problem [*]. Perhaps we could postpone this for after this patch > > set, > > and give it some more time to sink in? > > Yep. I'm fine with going ahead with this patchset as it is. Both > sorting > the bindings in alias order and updating the bindings if > /etc/multipath/bindings has changed are things that can get looked at > afterwards. And I'm fine with doing this work, if you want. It so happens that, by sudden inspiration, I've found an elegant solution to this problem (I think). We can take advantage of the fact that, for any given prefix, aliases with shorter string length will be sorted before longer ones ("mpathz" < "mpathaa"). By sorting the aliases by string length, and use alphabetical sorting only for strings of equal length, we obtain total ordering for any given prefix. This works for any number of different prefixes, and even if some prefixes are substrings of others. In the ordered list, the aliases with a given prefix will not necessarily be in a contiguous block, but that doesn't matter. For every prefix, the sub-list of aliases starting with that prefix is cleanly ordered. This way we avoid the complexity to have to parse or compare configured prefixes. I'll post a new patch set with this ordering scheme hopefully later today. Regards, Martin > > -Ben > > > Martin > > > > [*] I admit that with my patch, we _know_ now that the bindings > > list > > will be sub-optimally sorted as soon as mpathaa is reached, whereas > > before the ordering might be perfect even with a large number of > > aliases, depending on the history of the bindings file. That's not > > a > > change for the better; it will cause the gap to be missed in some > > situations where we don't miss it now. I am not sure how bad this > > is. > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, Sep 11, 2023 at 08:25:05AM +0200, Martin Wilck wrote: > On Fri, 2023-09-08 at 12:22 -0500, Benjamin Marzinski wrote: > > On Thu, Sep 07, 2023 at 10:43:27PM +0200, Martin Wilck wrote: > > > Our bindings list is now partially sorted, which is an improvement > > > wrt > > > the previous situation. "missing the gap" is not really an awful > > > problem [*]. Perhaps we could postpone this for after this patch > > > set, > > > and give it some more time to sink in? > > > > Yep. I'm fine with going ahead with this patchset as it is. Both > > sorting > > the bindings in alias order and updating the bindings if > > /etc/multipath/bindings has changed are things that can get looked at > > afterwards. And I'm fine with doing this work, if you want. > > It so happens that, by sudden inspiration, I've found an elegant > solution to this problem (I think). We can take advantage of the fact > that, for any given prefix, aliases with shorter string length will be > sorted before longer ones ("mpathz" < "mpathaa"). By sorting the > aliases by string length, and use alphabetical sorting only for strings > of equal length, we obtain total ordering for any given prefix. This > works for any number of different prefixes, and even if some prefixes > are substrings of others. In the ordered list, the aliases with a given > prefix will not necessarily be in a contiguous block, but that doesn't > matter. For every prefix, the sub-list of aliases starting with that > prefix is cleanly ordered. This way we avoid the complexity to have to > parse or compare configured prefixes. Clever. That seems like a good solution. -Ben > > I'll post a new patch set with this ordering scheme hopefully later > today. > > Regards, > Martin > > > > > -Ben > > > > > Martin > > > > > > [*] I admit that with my patch, we _know_ now that the bindings > > > list > > > will be sub-optimally sorted as soon as mpathaa is reached, whereas > > > before the ordering might be perfect even with a large number of > > > aliases, depending on the history of the bindings file. That's not > > > a > > > change for the better; it will cause the gap to be missed in some > > > situations where we don't miss it now. I am not sure how bad this > > > is. > > -- 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 2f9bc82..6003df0 100644 --- a/libmultipath/alias.c +++ b/libmultipath/alias.c @@ -50,8 +50,6 @@ "# alias wwid\n" \ "#\n" -static const char bindings_file_header[] = BINDINGS_FILE_HEADER; - struct binding { char *alias; char *wwid; @@ -80,6 +78,45 @@ static void _free_binding(struct binding *bdg) free(bdg); } +static const struct binding *get_binding_for_alias(const Bindings *bindings, + const char *alias) +{ + const struct binding *bdg; + int i; + + if (!alias) + return NULL; + vector_foreach_slot(bindings, bdg, i) { + if (!strncmp(bdg->alias, alias, WWID_SIZE)) { + condlog(3, "Found matching alias [%s] in bindings file." + " Setting wwid to %s", alias, bdg->wwid); + return bdg; + } + } + + condlog(3, "No matching alias [%s] in bindings file.", alias); + return NULL; +} + +static const struct binding *get_binding_for_wwid(const Bindings *bindings, + const char *wwid) +{ + const struct binding *bdg; + int i; + + if (!wwid) + return NULL; + vector_foreach_slot(bindings, bdg, i) { + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) { + condlog(3, "Found matching wwid [%s] in bindings file." + " Setting alias to %s", wwid, bdg->alias); + return bdg; + } + } + condlog(3, "No matching wwid [%s] in bindings file.", wwid); + return NULL; +} + static int add_binding(Bindings *bindings, const char *alias, const char *wwid) { struct binding *bdg; @@ -115,6 +152,24 @@ static int add_binding(Bindings *bindings, const char *alias, const char *wwid) return BINDING_ERROR; } +static int delete_binding(Bindings *bindings, const char *wwid) +{ + struct binding *bdg; + int i; + + vector_foreach_slot(bindings, bdg, i) { + if (!strncmp(bdg->wwid, wwid, WWID_SIZE)) { + _free_binding(bdg); + break; + } + } + if (i >= VECTOR_SIZE(bindings)) + return BINDING_NOTFOUND; + + vector_del_slot(bindings, i); + return BINDING_DELETED; +} + static int write_bindings_file(const Bindings *bindings, int fd) { struct binding *bnd; @@ -267,38 +322,15 @@ static bool id_already_taken(int id, const char *prefix, const char *map_wwid) return alias_already_taken(alias, map_wwid); } -/* - * Returns: 0 if matching entry in WWIDs file found - * -1 if an error occurs - * >0 a free ID that could be used for the WWID at hand - * *map_alias is set to a freshly allocated string with the matching alias if - * the function returns 0, or to NULL otherwise. - */ -static int -lookup_binding(FILE *f, const char *map_wwid, char **map_alias, - const char *prefix, int check_if_taken) +int get_free_id(const Bindings *bindings, const char *prefix, const char *map_wwid) { - char buf[LINE_MAX]; - unsigned int line_nr = 0; - int id = 1; + const struct binding *bdg; + int i, id = 1; int biggest_id = 1; int smallest_bigger_id = INT_MAX; - *map_alias = NULL; - - rewind(f); - while (fgets(buf, LINE_MAX, f)) { - const char *alias, *wwid; - char *c, *saveptr; - int curr_id; - - line_nr++; - c = strpbrk(buf, "#\n\r"); - if (c) - *c = '\0'; - alias = strtok_r(buf, " \t", &saveptr); - if (!alias) /* blank line */ - continue; + vector_foreach_slot(bindings, bdg, i) { + int curr_id = scan_devname(bdg->alias, prefix); /* * Find an unused index - explanation of the algorithm @@ -333,8 +365,6 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, * biggest_id is always > smallest_bigger_id, except in the * "perfectly ordered" case. */ - - curr_id = scan_devname(alias, prefix); if (curr_id == id) { if (id < INT_MAX) id++; @@ -345,36 +375,15 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, } if (curr_id > biggest_id) biggest_id = curr_id; + if (curr_id > id && curr_id < smallest_bigger_id) smallest_bigger_id = curr_id; - wwid = strtok_r(NULL, " \t", &saveptr); - if (!wwid){ - condlog(3, - "Ignoring malformed line %u in bindings file", - line_nr); - continue; - } - if (strcmp(wwid, map_wwid) == 0){ - condlog(3, "Found matching wwid [%s] in bindings file." - " Setting alias to %s", wwid, alias); - *map_alias = strdup(alias); - if (*map_alias == NULL) { - condlog(0, "Cannot copy alias from bindings " - "file: out of memory"); - return -1; - } - return 0; - } } - if (!prefix && check_if_taken) - id = -1; - if (id >= smallest_bigger_id) { - if (biggest_id < INT_MAX) - id = biggest_id + 1; - else - id = -1; - } - if (id > 0 && check_if_taken) { + + if (id >= smallest_bigger_id) + id = biggest_id < INT_MAX ? biggest_id + 1 : -1; + + if (id > 0) { while(id_already_taken(id, prefix, map_wwid)) { if (id == INT_MAX) { id = -1; @@ -391,63 +400,16 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, } } } - if (id < 0) { + + if (id < 0) condlog(0, "no more available user_friendly_names"); - return -1; - } else - condlog(3, "No matching wwid [%s] in bindings file.", map_wwid); return id; } -static int -rlookup_binding(FILE *f, char *buff, const char *map_alias) -{ - char line[LINE_MAX]; - unsigned int line_nr = 0; - - buff[0] = '\0'; - - while (fgets(line, LINE_MAX, f)) { - char *c, *saveptr; - const char *alias, *wwid; - - line_nr++; - c = strpbrk(line, "#\n\r"); - if (c) - *c = '\0'; - alias = strtok_r(line, " \t", &saveptr); - if (!alias) /* blank line */ - continue; - wwid = strtok_r(NULL, " \t", &saveptr); - if (!wwid){ - condlog(3, - "Ignoring malformed line %u in bindings file", - line_nr); - continue; - } - if (strlen(wwid) > WWID_SIZE - 1) { - condlog(3, - "Ignoring too large wwid at %u in bindings file", line_nr); - continue; - } - if (strcmp(alias, map_alias) == 0){ - condlog(3, "Found matching alias [%s] in bindings file." - " Setting wwid to %s", alias, wwid); - strlcpy(buff, wwid, WWID_SIZE); - return 0; - } - } - condlog(3, "No matching alias [%s] in bindings file.", map_alias); - - return -1; -} - static char * -allocate_binding(int fd, const char *wwid, int id, const char *prefix) +allocate_binding(const char *filename, const char *wwid, int id, const char *prefix) { STRBUF_ON_STACK(buf); - off_t offset; - ssize_t len; char *alias, *c; if (id <= 0) { @@ -460,29 +422,22 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix) format_devname(&buf, id) == -1) return NULL; - if (print_strbuf(&buf, " %s\n", wwid) < 0) - return NULL; - - offset = lseek(fd, 0, SEEK_END); - if (offset < 0){ - condlog(0, "Cannot seek to end of bindings file : %s", - strerror(errno)); - return NULL; - } - - len = get_strbuf_len(&buf); alias = steal_strbuf_str(&buf); - if (write(fd, alias, len) != len) { - condlog(0, "Cannot write binding to bindings file : %s", - strerror(errno)); - /* clear partial write */ - if (ftruncate(fd, offset)) - condlog(0, "Cannot truncate the header : %s", - strerror(errno)); + if (add_binding(&global_bindings, alias, wwid) != BINDING_ADDED) { + condlog(0, "%s: cannot allocate new binding %s for %s", + __func__, alias, wwid); free(alias); return NULL; } + + if (update_bindings_file(&global_bindings, filename) == -1) { + condlog(1, "%s: deleting binding %s for %s", __func__, alias, wwid); + delete_binding(&global_bindings, wwid); + free(alias); + return NULL; + } + c = strchr(alias, ' '); if (c) *c = '\0'; @@ -491,144 +446,130 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix) return alias; } +/* + * get_user_friendly_alias() action table + * + * The table shows the various cases, the actions taken, and the CI + * functions from tests/alias.c that represent them. + * + * - O: old alias given + * - A: old alias in table (y: yes, correct WWID; X: yes, wrong WWID) + * - W: wwid in table + * - U: old alias is used + * + * | No | O | A | W | U | action | function gufa_X | + * |----+---+---+---+---+--------------------------------------------+------------------------------| + * | 1 | n | - | n | - | get new alias | nomatch_Y | + * | 2 | n | - | y | - | use alias from bindings | match_a_Y | + * | 3 | y | n | n | n | add binding for old alias | old_nomatch_nowwidmatch | + * | 4 | y | n | n | y | error, no alias (can't add entry) | old_nomatch_nowwidmatch_used | + * | 5 | y | n | y | - | use alias from bindings (avoid duplicates) | old_nomatch_wwidmatch | + * | 6 | y | y | n | - | [ impossible ] | - | + * | 7 | y | y | y | n | use old alias == alias from bindings | old_match | + * | 8 | y | y | y | y | error, no alias (would be duplicate) | old_match_used | + * | 9 | y | X | n | - | get new alias | old_match_other | + * | 10 | y | X | y | - | use alias from bindings | old_match_other_wwidmatch | + * + * Notes: + * - "use alias from bindings" means that the alias from the bindings file will + * be tried; if it is in use, the alias selection will fail. No other + * bindings will be attempted. + * - "get new alias" fails if all aliases are used up, or if writing the + * bindings file fails. + */ + char *get_user_friendly_alias(const char *wwid, const char *file, const char *alias_old, const char *prefix, bool bindings_read_only) { char *alias = NULL; int id = 0; - int fd, can_write; bool new_binding = false; - char buff[WWID_SIZE]; - FILE *f; + bool old_alias_taken = false; + const struct binding *bdg; - fd = open_file(file, &can_write, bindings_file_header); - if (fd < 0) - return NULL; - - f = fdopen(fd, "r"); - if (!f) { - condlog(0, "cannot fdopen on bindings file descriptor"); - close(fd); - return NULL; - } - - if (!strlen(alias_old)) + if (!*alias_old) goto new_alias; - /* lookup the binding. if it exists, the wwid will be in buff - * either way, id contains the id for the alias - */ - rlookup_binding(f, buff, alias_old); - - if (strlen(buff) > 0) { - /* if buff is our wwid, it's already - * allocated correctly - */ - if (strcmp(buff, wwid) == 0) { + /* See if there's a binding matching both alias_old and wwid */ + bdg = get_binding_for_alias(&global_bindings, alias_old); + if (bdg) { + if (!strcmp(bdg->wwid, wwid)) { if (!alias_already_taken(alias_old, wwid)) alias = strdup(alias_old); else condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", alias_old, wwid); goto out; - } else { condlog(0, "alias %s already bound to wwid %s, cannot reuse", - alias_old, buff); - goto new_alias; ; + alias_old, bdg->wwid); + goto new_alias; } } - /* - * Look for an existing alias in the bindings file. - * Pass prefix = NULL, so lookup_binding() won't try to allocate a new id. - */ - id = lookup_binding(f, wwid, &alias, NULL, 0); - if (alias) { - if (alias_already_taken(alias, wwid)) { - free(alias); - alias = NULL; - } else - condlog(3, "Use existing binding [%s] for WWID [%s]", - alias, wwid); - goto out; - } - /* allocate the existing alias in the bindings file */ - id = scan_devname(alias_old, prefix); - if (id > 0 && id_already_taken(id, prefix, wwid)) { + if (alias_already_taken(alias_old, wwid)) { condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", alias_old, wwid); + old_alias_taken = true; + } else + id = scan_devname(alias_old, prefix); + +new_alias: + /* Check for existing binding of WWID */ + bdg = get_binding_for_wwid(&global_bindings, wwid); + if (bdg) { + if (!alias_already_taken(bdg->alias, wwid)) { + condlog(3, "Use existing binding [%s] for WWID [%s]", + bdg->alias, wwid); + alias = strdup(bdg->alias); + } goto out; } -new_alias: - if (id <= 0) { + /* + * old_alias_taken means that the WWID is not in the bindings file, but + * alias_old is currently taken by a different WWID. We shouldn't added + * a new binding in this case. + */ + if (id <= 0 && !old_alias_taken) { /* * no existing alias was provided, or allocating it * failed. Try a new one. */ - id = lookup_binding(f, wwid, &alias, prefix, 1); - if (id == 0 && alias_already_taken(alias, wwid)) { - free(alias); - alias = NULL; - } + id = get_free_id(&global_bindings, prefix, wwid); if (id <= 0) goto out; else new_binding = true; } - if (fflush(f) != 0) { - condlog(0, "cannot fflush bindings file stream : %s", - strerror(errno)); - goto out; - } + if (!bindings_read_only && id > 0) + alias = allocate_binding(file, wwid, id, prefix); - if (can_write && !bindings_read_only) { - alias = allocate_binding(fd, wwid, id, prefix); - if (alias && !new_binding) - condlog(2, "Allocated existing binding [%s] for WWID [%s]", - alias, wwid); - } + if (alias && !new_binding) + condlog(2, "Allocated existing binding [%s] for WWID [%s]", + alias, wwid); out: - pthread_cleanup_push(free, alias); - fclose(f); - pthread_cleanup_pop(0); return alias; } -int -get_user_friendly_wwid(const char *alias, char *buff, const char *file) +int get_user_friendly_wwid(const char *alias, char *buff) { - int fd, unused; - FILE *f; + const struct binding *bdg; if (!alias || *alias == '\0') { condlog(3, "Cannot find binding for empty alias"); return -1; } - fd = open_file(file, &unused, bindings_file_header); - if (fd < 0) - return -1; - - f = fdopen(fd, "r"); - if (!f) { - condlog(0, "cannot fdopen on bindings file descriptor : %s", - strerror(errno)); - close(fd); + bdg = get_binding_for_alias(&global_bindings, alias); + if (!bdg) { + *buff = '\0'; return -1; } - - rlookup_binding(f, buff, alias); - if (!strlen(buff)) { - fclose(f); - return -1; - } - - fclose(f); + strlcpy(buff, bdg->alias, WWID_SIZE); return 0; } diff --git a/libmultipath/alias.h b/libmultipath/alias.h index 37b49d9..5ef6720 100644 --- a/libmultipath/alias.h +++ b/libmultipath/alias.h @@ -2,7 +2,7 @@ #define _ALIAS_H int valid_alias(const char *alias); -int get_user_friendly_wwid(const char *alias, char *buff, const char *file); +int get_user_friendly_wwid(const char *alias, char *buff); char *get_user_friendly_alias(const char *wwid, const char *file, const char *alias_old, const char *prefix, bool bindings_read_only); diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 029fbbd..d809490 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -1378,8 +1378,7 @@ static int _get_refwwid(enum mpath_cmds cmd, const char *dev, refwwid = tmpwwid; /* or may be a binding */ - else if (get_user_friendly_wwid(dev, tmpwwid, - conf->bindings_file) == 0) + else if (get_user_friendly_wwid(dev, tmpwwid) == 0) refwwid = tmpwwid; /* or may be an alias */