On Wed, 2021-03-10 at 15:15 -0600, Benjamin Marzinski wrote:
> If there are multipath devices that have user_friendly_names but do
> not
> have their bindings in the bindings_file, get_user_friendly_alias()
> can
> currently give out those names again. This can result in an incorrect
> entry in the bindings file, and a device that gets created with a
> WWID
> alias instead of a user_friendly_name. This situation can happen
> after
> the pivot root, if a multipath device is created in the initramfs.
> If
> this device doesn't have a binding in the regular filesystem
> bindings_file and a new multipath device is created before it can add
> its binding, the new device can steal that user_friendly_name during
> multipathd's initial configure.
>
> To solve this, get_user_friendly_alias() now calls lookup_binding()
> with
> a new paramter, telling it to check if the id it found is already in
> use
> by a diffent device. If so, lookup_binding() will continue to check
> open
> ids, until it finds one that it not currently in use by a dm device.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
>
> /*
> * Returns: 0 if matching entry in WWIDs file found
> * -1 if an error occurs
> @@ -128,7 +151,7 @@ scan_devname(const char *alias, const char
> *prefix)
> */
> static int
> lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
> - const char *prefix)
> + const char *prefix, int check_if_taken)
> {
> char buf[LINE_MAX];
> unsigned int line_nr = 0;
> @@ -183,12 +206,31 @@ lookup_binding(FILE *f, const char *map_wwid,
> char **map_alias,
> 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) {
> + while(id_already_taken(id, prefix, map_wwid)) {
> + if (id == INT_MAX) {
> + id = -1;
> + break;
> + }
> + id++;
> + if (id == smallest_bigger_id) {
> + if (biggest_id == INT_MAX) {
> + id = -1;
> + break;
> + }
> + if (biggest_id >= smallest_bigger_id)
> + id = biggest_id + 1;
> + }
> + }
> + }
This seems to be correct, but I am never certain when I look at this
code. I truly dislike the awkward logic of lookup_binding(), and adding
more complexity to it doesn't improve matters. I really only trust that
logic because of the unit test.
Rather than re-reading the file on every lookup, we should cache its
contents in a sorted array. That would make it much easier to
write an algorithm for locating free slots that could be reviewed by
average human beings. We could even add the aliases of detected
existing maps to that array, so that no additional "already taken"
check would be necessary.
I'd actually started working on that some time ago, but never finished
that work.
Anyway, it't not a cause against your patch.
Martin
Reviewed-by: Martin Wilck <mwilck@xxxxxxxx>>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
