Linux Device Mapper development
 help / color / mirror / Atom feed
* [dm-devel] [PATCH v3 00/38] multipath-tools: user-friendly names rework
@ 2023-09-14 14:51 mwilck
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings mwilck
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: mwilck @ 2023-09-14 14:51 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

This patch set contains a two-step rework of the user-friendly
names code. Patch 2-5 change the current code such that it (well, almost)
never attempts to use an alias that is currently in use by another
map. We already have some checks for this, but they don't cover all
possible scenarios. Patch 6-10 add some units tests for
get_user_friendly_alias(), and restructure the unit tests somewhat.

In the second part of the set, Patch 11-18 restructure the alias code such
that we don't read the bindings file every time an alias is
requested. Instead, we just use a memory cache of the current bindings, and
re-write the file only if a new binding is added.  This reduces the number of
system calls and simplifies the code.

Patch 19 applies the changes to the unit tests that become
necessary because of the previous patches. Patch 20/21 add another
optimization for the alias_already_taken() test.

Patch 22-23 introduce a solution for the problem discussed in the review
of v1 of this patch set, the partial ordering of aliases in memory.
By sorting first be string length, and then alphabetically, we obtain
a total "numeric" ordering for every given alias prefix. This ordering
allows simplifying the search for "gaps" in the list of aliases, which
is done in patch 24-26.

As discussed in the opening letter of the v1 patch series, the patch
series has the effect that the bindings file is not re-read every time
a new binding is attempted. Patch 28 and 29 introduce a notification
mechanism for multipathd which causes the bindings to be re-read
when the bindings file has been updated.

Patch 30-36 are a series of minor fixes for the build system and the
documentation. Most imporantly, the man pages will now show the correct
paths for various built-in defaults.

Finally, patch 37 (which is deliberately kept separate and at the end of the
series, in order to allow downstream integrators to skip it) eventually
deprecates the runtime configuration options for bindings_file, wwids_file,
and prkeys_file.

Patch 1 is a separate, independent fix.

NOTE: my main test bed is currently unavailable, therefore this set has
yet reveived less testing than usual. I have tested this to the extent
I currently can, but further testing will be highly appreciated.

To avoid spam, I'm not re-posting the patches that already got Ben's
"Reviewed-by:" tag.

Reviews and comments welcome.

# Changes v1 -> v2:

* Patch 03: skipped superfluous test for the case where a map already
  has an alias (Ben Marzinski)
* Patch 07: removed invalid tests for the case where the already assigned
  alias of a given map is assigned to a different wwid (Ben Marzinski)
* Patch 09/10: moved some macro-usage fixes from 10 to 09
* Patch 18:
  - skipped superfluous tests like in patch 03 (Ben Marzinski)
  - fix bug in get_user_friendly_wwid() (Ben Marzinski)
* Added patch 22 ff. (see above).

# Changes v2 -> v3:

* Patch 26: avoid negative array index (Ben Marzinski)
* Patch 27:
  - assign old_bindings under bindings_mutex (Ben Marzinski)
  - move condlog after timestamp update in update_bindings_file() (Ben, me)
  - add read_bindings_file() call in get_user_friendly_wwid() (Ben)
  - fix return value check of fread() (Ben)
  - remove superfluous test in _read_bindings_file() (Ben)
  - avoid compilation error on Jessie for initialization of struct timespec (me)
* Patch 38: added to fix a compilation error with gcc 12 and liburcu < 0.14

Martin Wilck (38):
  libmultipath: sysfs_set_scsi_tmo: do nothing for ACT_DRY_RUN
  libmultipath: add alias_already_taken()
  libmultipath: unify use_existing_alias() and get_user_friendly_alias()
  libmultipath: never allocate an alias that's already taken
  libmultipath: lookup_binding: add comment about the algorithm
  multipath-tools test: simplify debugging for condlog mismatch
  multipath-tools tests: add tests for get_user_friendly_alias()
  multipath-tools test: consistent use of macros in alias test
  multipath-tools tests: convert mock_{failed,used}_alias to macros
  multipath-tools test: use mock_bindings_file() consistently
  libmultipath: add global variable for current bindings
  libmultipath: rename fix_bindings_file() to update_bindings_file()
  libmultipath: alias.c: move bindings related code up
  libmultipath: update_bindings_file: take filename argument
  libmultipath: update_bindings_file: use a single write()
  libmultipath: update_bindings_file: don't log temp file name
  libmultipath: alias.c: factor out read_binding()
  libmultipath: keep bindings in memory
  multipath-tools tests: fix alias tests
  libmultipath: dm_get_uuid(): return emtpy UUID for non-existing maps
  libmultipath: adapt to new semantics of dm_get_uuid()
  libmultipath: sort aliases by length and strcmp
  multipath-tools tests: fix alias test after sort order change
  libmultipath: simplify get_free_id() assuming total ordering
  multipath-tools tests: adapt alias tests for total ordering
  multipath-tools tests: add test for ordering of bindings
  multipathd: watch bindings file with inotify + timestamp
  multipath-tools tests: mock pthread_mutex_{lock,unlock}
  multipath-tools Makefile: sanitize paths for configuration files
  multipath-tools: add compile time configuration for "/etc/multipath"
  multipath-tools man pages: generate with correct paths
  libdmmp/Makefile: fix bug in install section
  multipath-tools: README.md: improve documentation for compile-time
    options
  libmultipath: print built-in values for deprecated options
  multipath: add a missing newline
  multipath-tools: allow prefixes with and w/o trailing slash
  libmultipath: deprecate bindings_file, wwids_file, prkeys_file
  libmultipath: avoid -Warray-bounds error in uatomic operations

 .gitignore                                    |    4 +
 Makefile.inc                                  |   45 +-
 README.md                                     |   38 +-
 create-config.mk                              |    5 +
 libdmmp/Makefile                              |    2 +-
 libmultipath/alias.c                          | 1043 +++++++-----
 libmultipath/alias.h                          |   14 +-
 libmultipath/config.c                         |   18 -
 libmultipath/config.h                         |    3 -
 libmultipath/configure.c                      |    7 +-
 libmultipath/defaults.h                       |    7 +-
 libmultipath/devmapper.c                      |   10 +-
 libmultipath/dict.c                           |   55 +-
 libmultipath/discovery.c                      |    3 +
 libmultipath/libmultipath.version             |    4 +-
 libmultipath/lock.h                           |   23 +-
 libmultipath/prkey.c                          |    7 +-
 libmultipath/prkey.h                          |    7 +-
 libmultipath/propsel.c                        |   20 +-
 libmultipath/wwids.c                          |   18 +-
 mpathpersist/Makefile                         |    5 +-
 .../{mpathpersist.8 => mpathpersist.8.in}     |    2 +-
 multipath/Makefile                            |   13 +-
 multipath/main.c                              |    6 +-
 multipath/{multipath.8 => multipath.8.in}     |   10 +-
 .../{multipath.conf.5 => multipath.conf.5.in} |   44 +-
 multipathd/Makefile                           |   11 +-
 multipathd/main.c                             |    1 +
 multipathd/{multipathd.8 => multipathd.8.in}  |    8 +-
 multipathd/uxlsnr.c                           |   29 +-
 tests/Makefile                                |    1 +
 tests/alias.c                                 | 1491 +++++++++++++----
 tests/test-log.c                              |    4 +-
 33 files changed, 1983 insertions(+), 975 deletions(-)
 rename mpathpersist/{mpathpersist.8 => mpathpersist.8.in} (99%)
 rename multipath/{multipath.8 => multipath.8.in} (97%)
 rename multipath/{multipath.conf.5 => multipath.conf.5.in} (98%)
 rename multipathd/{multipathd.8 => multipathd.8.in} (97%)

-- 
2.42.0

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings
  2023-09-14 14:51 [dm-devel] [PATCH v3 00/38] multipath-tools: user-friendly names rework mwilck
@ 2023-09-14 14:51 ` mwilck
  2023-09-14 21:25   ` Benjamin Marzinski
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 27/38] multipathd: watch bindings file with inotify + timestamp mwilck
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations mwilck
  2 siblings, 1 reply; 7+ messages in thread
From: mwilck @ 2023-09-14 14:51 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

As the assignment of free aliases now relies on the bindings being
properly sorted, add some unit tests to make sure the sorting algorithm
works.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/alias.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 209 insertions(+), 3 deletions(-)

diff --git a/tests/alias.c b/tests/alias.c
index dff5f93..4d0adba 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -13,6 +13,9 @@
 #include "globals.c"
 #include "../libmultipath/alias.c"
 
+/* For verbose printing of all aliases in the ordering tests */
+#define ALIAS_DEBUG 0
+
 #if INT_MAX == 0x7fffffff
 /* user_friendly_name for map #INT_MAX */
 #define MPATH_ID_INT_MAX "fxshrxw"
@@ -439,11 +442,12 @@ static void mock_self_alias(const char *alias, const char *wwid)
 		expect_condlog(3, USED_STR(alias, wwid));		\
 	} while(0)
 
-static void __mock_bindings_file(const char *content)
+static void __mock_bindings_file(const char *content, bool conflict_ok)
 {
 	char *cnt __attribute__((cleanup(cleanup_charp))) = NULL;
 	char *token, *savep = NULL;
 	int i;
+	uintmax_t values[] = { BINDING_ADDED, BINDING_CONFLICT };
 
 	cnt = strdup(content);
 	assert_ptr_not_equal(cnt, NULL);
@@ -459,12 +463,12 @@ static void __mock_bindings_file(const char *content)
 			continue;
 
 		rc = add_binding(&global_bindings, alias, wwid);
-		assert_int_equal(rc, BINDING_ADDED);
+		assert_in_set(rc, values, conflict_ok ? 2 : 1);
 	}
 }
 
 static void mock_bindings_file(const char *content) {
-	return __mock_bindings_file(content);
+	return __mock_bindings_file(content, false);
 }
 
 static int teardown_bindings(void **state)
@@ -1744,6 +1748,207 @@ static int test_get_user_friendly_alias()
 	return cmocka_run_group_tests(tests, NULL, NULL);
 }
 
+/* Numbers 1-1000, randomly shuffled */
+static int random_numbers[1000] = {
+	694, 977, 224, 178, 841, 818, 914, 549, 831, 942, 263, 834, 919, 800,
+	111, 517, 719, 297, 988, 98, 332, 516, 754, 772, 495, 488, 331, 529,
+	142, 747, 848, 618, 375, 624, 74, 753, 782, 944, 623, 468, 862, 997,
+	417, 258, 298, 774, 673, 904, 883, 766, 867, 400, 11, 950, 14, 784,
+	655, 155, 396, 9, 743, 93, 651, 245, 968, 306, 785, 581, 880, 486,
+	168, 631, 203, 4, 663, 294, 702, 762, 619, 684, 48, 181, 21, 443, 643,
+	863, 1000, 327, 26, 126, 382, 765, 586, 76, 49, 925, 319, 865, 797,
+	876, 693, 334, 433, 243, 419, 901, 854, 326, 985, 347, 874, 527, 282,
+	290, 380, 167, 95, 3, 257, 936, 60, 426, 227, 345, 577, 492, 467, 580,
+	967, 422, 823, 718, 610, 64, 700, 412, 163, 288, 506, 828, 432, 51,
+	356, 348, 539, 478, 17, 945, 602, 123, 450, 660, 429, 113, 310, 358,
+	512, 758, 508, 19, 542, 304, 286, 446, 918, 723, 333, 603, 731, 978,
+	230, 697, 109, 872, 175, 853, 947, 965, 121, 222, 101, 811, 117, 601,
+	191, 752, 384, 415, 938, 278, 915, 715, 240, 552, 912, 838, 150, 840,
+	627, 29, 636, 464, 861, 481, 992, 249, 934, 82, 368, 724, 807, 593,
+	157, 147, 199, 637, 41, 62, 902, 505, 621, 342, 174, 260, 729, 961,
+	219, 311, 629, 789, 81, 739, 860, 712, 223, 165, 741, 981, 485, 363,
+	346, 709, 125, 369, 279, 634, 399, 162, 193, 769, 149, 314, 868, 612,
+	524, 675, 341, 343, 476, 606, 388, 613, 850, 264, 903, 451, 908, 779,
+	453, 148, 497, 46, 132, 43, 885, 955, 269, 395, 72, 128, 767, 989,
+	929, 423, 742, 55, 13, 79, 924, 182, 295, 563, 668, 169, 974, 154,
+	970, 54, 674, 52, 437, 570, 550, 531, 554, 793, 678, 218, 367, 105,
+	197, 315, 958, 892, 86, 47, 284, 37, 561, 522, 198, 689, 817, 573,
+	877, 201, 803, 501, 881, 546, 530, 523, 780, 579, 953, 135, 23, 620,
+	84, 698, 303, 656, 357, 323, 494, 58, 131, 913, 995, 120, 70, 1, 195,
+	365, 210, 25, 898, 173, 307, 239, 77, 418, 952, 963, 92, 455, 425, 12,
+	536, 161, 328, 933, 401, 251, 735, 725, 362, 322, 557, 681, 302, 53,
+	786, 801, 391, 946, 748, 133, 717, 851, 7, 372, 993, 387, 906, 373,
+	667, 33, 670, 389, 209, 611, 896, 652, 69, 999, 344, 845, 633, 36,
+	487, 192, 180, 45, 640, 427, 707, 805, 188, 152, 905, 217, 30, 252,
+	386, 665, 299, 541, 410, 787, 5, 857, 751, 392, 44, 595, 146, 745,
+	641, 957, 866, 773, 806, 815, 659, 102, 704, 430, 106, 296, 129, 847,
+	130, 990, 669, 236, 225, 680, 159, 213, 438, 189, 447, 600, 232, 594,
+	32, 56, 390, 647, 855, 428, 330, 714, 738, 706, 666, 461, 469, 482,
+	558, 814, 559, 177, 575, 538, 309, 383, 261, 156, 420, 761, 630, 893,
+	10, 116, 940, 844, 71, 377, 662, 312, 520, 244, 143, 759, 119, 186,
+	592, 909, 864, 376, 768, 254, 265, 394, 511, 760, 574, 6, 436, 514,
+	59, 226, 644, 956, 578, 825, 548, 145, 736, 597, 378, 821, 987, 897,
+	354, 144, 722, 895, 589, 503, 826, 498, 543, 617, 763, 231, 808, 528,
+	89, 479, 607, 737, 170, 404, 371, 65, 103, 340, 283, 141, 313, 858,
+	289, 124, 971, 687, 954, 732, 39, 926, 176, 100, 267, 519, 890, 535,
+	276, 448, 27, 457, 899, 385, 184, 275, 770, 544, 614, 449, 160, 658,
+	259, 973, 108, 604, 24, 207, 562, 757, 744, 324, 444, 962, 591, 480,
+	398, 409, 998, 253, 325, 445, 979, 8, 35, 118, 73, 683, 208, 85, 190,
+	791, 408, 871, 657, 179, 18, 556, 496, 475, 20, 894, 484, 775, 889,
+	463, 241, 730, 57, 907, 551, 859, 943, 185, 416, 870, 590, 435, 471,
+	932, 268, 381, 626, 502, 565, 273, 534, 672, 778, 292, 473, 566, 104,
+	172, 285, 832, 411, 329, 628, 397, 472, 271, 910, 711, 690, 969, 585,
+	809, 941, 923, 555, 228, 685, 242, 94, 96, 211, 140, 61, 922, 795,
+	869, 34, 255, 38, 984, 676, 15, 560, 632, 434, 921, 355, 582, 351,
+	212, 200, 819, 960, 649, 852, 75, 771, 361, 996, 238, 316, 720, 671,
+	462, 112, 569, 171, 664, 625, 588, 405, 553, 270, 533, 353, 842, 114,
+	972, 83, 937, 63, 194, 237, 537, 980, 802, 916, 959, 688, 839, 350,
+	917, 650, 545, 615, 151, 352, 686, 726, 266, 509, 439, 491, 935, 608,
+	518, 653, 339, 609, 277, 635, 836, 88, 407, 440, 642, 927, 229, 727,
+	360, 477, 846, 413, 454, 616, 28, 598, 567, 540, 790, 424, 247, 317,
+	746, 911, 798, 321, 547, 248, 734, 829, 220, 138, 756, 500, 691, 196,
+	740, 930, 843, 733, 221, 827, 50, 813, 949, 525, 349, 474, 134, 875,
+	695, 513, 414, 515, 638, 99, 366, 490, 975, 246, 465, 206, 281, 583,
+	256, 587, 749, 2, 951, 679, 215, 364, 458, 402, 646, 991, 335, 982,
+	835, 300, 900, 703, 994, 983, 234, 888, 532, 804, 584, 305, 792, 442,
+	291, 964, 158, 370, 452, 250, 521, 166, 948, 812, 794, 272, 699, 205,
+	183, 507, 301, 920, 781, 233, 824, 137, 489, 833, 887, 966, 856, 78,
+	830, 153, 359, 696, 526, 216, 66, 701, 403, 891, 849, 571, 308, 483,
+	164, 293, 928, 677, 320, 837, 441, 639, 564, 510, 648, 274, 336, 661,
+	878, 777, 816, 976, 493, 810, 67, 87, 91, 187, 882, 986, 80, 22, 499,
+	90, 705, 139, 136, 122, 708, 716, 886, 572, 127, 40, 721, 764, 16,
+	379, 692, 645, 456, 710, 460, 783, 97, 776, 713, 884, 115, 466, 596,
+	374, 406, 110, 568, 68, 214, 622, 470, 107, 504, 682, 31, 421, 576,
+	654, 605, 788, 799, 280, 338, 931, 873, 204, 287, 459, 755, 939, 599,
+	431, 796, 235, 42, 750, 262, 318, 393, 202, 822, 879, 820, 728, 337,
+};
+
+static void fill_bindings_random(struct strbuf *buf, int start, int end,
+				 const char *prefix)
+{
+	int i;
+
+	for (i = start; i < end; i++) {
+		print_strbuf(buf,  "%s", prefix);
+		format_devname(buf, random_numbers[i]);
+		print_strbuf(buf,  " WWID%d\n", random_numbers[i]);
+	}
+}
+
+struct random_aliases {
+	int start;
+	int end;
+	const char *prefix;
+};
+
+static void order_test(int n, struct random_aliases ra[], bool conflict_ok)
+{
+	STRBUF_ON_STACK(buf);
+	int i, j, prev, curr, tmp;
+	struct binding *bdg;
+	Bindings *bindings = &global_bindings;
+
+	for (j = 0; j < n; j++)
+		fill_bindings_random(&buf, ra[j].start, ra[j].end, ra[j].prefix);
+	__mock_bindings_file(get_strbuf_str(&buf), conflict_ok);
+
+	for (j = 0; j < n; j++) {
+		bdg = VECTOR_SLOT(bindings, 0);
+		if (ALIAS_DEBUG && j == 0)
+			printf("%d: %s\n", 0, bdg->alias);
+		prev = scan_devname(bdg->alias, ra[j].prefix);
+		i = 1;
+		vector_foreach_slot_after(bindings, bdg, i) {
+			if (ALIAS_DEBUG && j == 0)
+				printf("%d: %s\n", i, bdg->alias);
+			tmp = scan_devname(bdg->alias, ra[j].prefix);
+			if (tmp == -1)
+				continue;
+			curr = tmp;
+			if (prev > 0) {
+				if (curr <= prev)
+					printf("ERROR: %d (%s) %d >= %d\n",
+					       i, bdg->alias, prev, curr);
+				assert_true(curr > prev);
+			}
+			prev = curr;
+		}
+	}
+}
+
+static void order_01(void **state)
+{
+	struct random_aliases ra[] = {
+		{  0, 1000, "MPATH" },
+	};
+
+	order_test(ARRAY_SIZE(ra), ra, false);
+}
+
+static void order_02(void **state)
+{
+	struct random_aliases ra[] = {
+		{   0, 500, "MPATH" },
+		{ 200, 700, "mpath" },
+	};
+	order_test(ARRAY_SIZE(ra), ra, false);
+}
+
+static void order_03(void **state)
+{
+	struct random_aliases ra[] = {
+		{  500, 1000, "MPTH" },
+		{    0,  500, "MPATH" },
+	};
+	order_test(ARRAY_SIZE(ra), ra, false);
+}
+
+static void order_04(void **state)
+{
+	struct random_aliases ra[] = {
+		{   0, 500, "mpa" },
+		{ 250, 750, "mp" },
+	};
+	order_test(ARRAY_SIZE(ra), ra, true);
+}
+
+static void order_05(void **state)
+{
+	struct random_aliases ra[] = {
+		{  0, 100, "A" },
+		{  0, 100, "B" },
+		{  0, 100, "C" },
+		{  0, 100, "D" },
+	};
+	order_test(ARRAY_SIZE(ra), ra, false);
+}
+
+static void order_06(void **state)
+{
+	struct random_aliases ra[] = {
+		{  0, 100, "" },
+		{  0, 100, "a" },
+		{  0, 100, "aa" },
+		{  0, 100, "ab" },
+		{  0, 100, "aaa" },
+	};
+	order_test(ARRAY_SIZE(ra), ra, true);
+}
+
+static int test_bindings_order()
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test_teardown(order_01, teardown_bindings),
+		cmocka_unit_test_teardown(order_02, teardown_bindings),
+		cmocka_unit_test_teardown(order_03, teardown_bindings),
+		cmocka_unit_test_teardown(order_04, teardown_bindings),
+		cmocka_unit_test_teardown(order_05, teardown_bindings),
+		cmocka_unit_test_teardown(order_06, teardown_bindings),
+	};
+
+	return cmocka_run_group_tests(tests, NULL, NULL);
+}
+
 int main(void)
 {
 	int ret = 0;
@@ -1755,6 +1960,7 @@ int main(void)
 	ret += test_rlookup_binding();
 	ret += test_allocate_binding();
 	ret += test_get_user_friendly_alias();
+	ret += test_bindings_order();
 
 	return ret;
 }
-- 
2.42.0

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [dm-devel] [PATCH v3 27/38] multipathd: watch bindings file with inotify + timestamp
  2023-09-14 14:51 [dm-devel] [PATCH v3 00/38] multipath-tools: user-friendly names rework mwilck
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings mwilck
@ 2023-09-14 14:51 ` mwilck
  2023-09-14 22:28   ` Benjamin Marzinski
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations mwilck
  2 siblings, 1 reply; 7+ messages in thread
From: mwilck @ 2023-09-14 14:51 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Since "libmultipath: keep bindings in memory", we don't re-read the
bindings file after every modification. Add a notification mechanism
that makes multipathd aware of changes to the bindings file. Because
multipathd itself will change the bindings file, it must compare
timestamps in order to avoid reading the file repeatedly.

Because select_alias() can be called from multiple thread contexts (uxlsnr,
uevent handler), we need to add locking for the bindings file. The
timestamp must also be protected by a lock, because it can't be read
or written atomically.

Note: The notification mechanism expects the bindings file to be
atomically replaced by rename(2). Changes must be made in a temporary file and
applied using rename(2), as in update_bindings_file(). The inotify
mechanism deliberately does not listen to close-after-write events
that would be generated by editing the bindings file directly. This

Note also: new bindings will only be read from add_map_with_path(),
i.e. either during reconfigure(), or when a new map is created during
runtime. Existing maps will not be renamed if the binding file changes,
unless the user runs "multipathd reconfigure". This is not a change
wrt the previous code, but it should be mentioned anyway.

Signed-off-by: Martin Wilck <mwilck@suse.com>

libmultipath: protect global_bindings with a mutex

Signed-off-by: Martin Wilck <mwilck@suse.com>

libmultipath: check timestamp of bindings file before reading it

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c              | 252 +++++++++++++++++++++++++-----
 libmultipath/alias.h              |   3 +-
 libmultipath/libmultipath.version |   5 +
 multipathd/uxlsnr.c               |  36 ++++-
 tests/alias.c                     |   3 +
 5 files changed, 254 insertions(+), 45 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 66e34e3..964b8a7 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -10,6 +10,7 @@
 #include <stdio.h>
 #include <stdbool.h>
 #include <assert.h>
+#include <sys/inotify.h>
 
 #include "debug.h"
 #include "util.h"
@@ -22,6 +23,7 @@
 #include "config.h"
 #include "devmapper.h"
 #include "strbuf.h"
+#include "time-util.h"
 
 /*
  * significant parts of this file were taken from iscsi-bindings.c of the
@@ -50,6 +52,12 @@
 "# alias wwid\n" \
 "#\n"
 
+/* uatomic access only */
+static int bindings_file_changed = 1;
+
+static pthread_mutex_t timestamp_mutex = PTHREAD_MUTEX_INITIALIZER;
+static struct timespec bindings_last_updated;
+
 struct binding {
 	char *alias;
 	char *wwid;
@@ -60,6 +68,9 @@ struct binding {
  * an abstract type.
  */
 typedef struct _vector Bindings;
+
+/* Protect global_bindings */
+static pthread_mutex_t bindings_mutex = PTHREAD_MUTEX_INITIALIZER;
 static Bindings global_bindings = { .allocated = 0 };
 
 enum {
@@ -78,6 +89,27 @@ static void _free_binding(struct binding *bdg)
 	free(bdg);
 }
 
+static void free_bindings(Bindings *bindings)
+{
+	struct binding *bdg;
+	int i;
+
+	vector_foreach_slot(bindings, bdg, i)
+		_free_binding(bdg);
+	vector_reset(bindings);
+}
+
+static void set_global_bindings(Bindings *bindings)
+{
+	Bindings old_bindings;
+
+	pthread_mutex_lock(&bindings_mutex);
+	old_bindings = global_bindings;
+	global_bindings = *bindings;
+	pthread_mutex_unlock(&bindings_mutex);
+	free_bindings(&old_bindings);
+}
+
 static const struct binding *get_binding_for_alias(const Bindings *bindings,
 						   const char *alias)
 {
@@ -199,7 +231,8 @@ static int delete_binding(Bindings *bindings, const char *wwid)
 	return BINDING_DELETED;
 }
 
-static int write_bindings_file(const Bindings *bindings, int fd)
+static int write_bindings_file(const Bindings *bindings, int fd,
+			       struct timespec *ts)
 {
 	struct binding *bnd;
 	STRBUF_ON_STACK(content);
@@ -227,9 +260,56 @@ static int write_bindings_file(const Bindings *bindings, int fd)
 		}
 		len -= n;
 	}
+	fsync(fd);
+	if (ts) {
+		struct stat st;
+
+		if (fstat(fd, &st) == 0)
+			*ts = st.st_mtim;
+		else
+			clock_gettime(CLOCK_REALTIME_COARSE, ts);
+	}
 	return 0;
 }
 
+void handle_bindings_file_inotify(const struct inotify_event *event)
+{
+	struct config *conf;
+	const char *base;
+	bool changed = false;
+	struct stat st;
+	struct timespec ts = { 0, 0 };
+	int ret;
+
+	if (!(event->mask & IN_MOVED_TO))
+		return;
+
+	conf = get_multipath_config();
+	base = strrchr(conf->bindings_file, '/');
+	changed = base && base > conf->bindings_file &&
+		!strcmp(base + 1, event->name);
+	ret = stat(conf->bindings_file, &st);
+	put_multipath_config(conf);
+
+	if (!changed)
+		return;
+
+	pthread_mutex_lock(&timestamp_mutex);
+	if (ret == 0) {
+		ts = st.st_mtim;
+		changed = timespeccmp(&ts, &bindings_last_updated) > 0;
+	}
+	pthread_mutex_unlock(&timestamp_mutex);
+
+	if (changed) {
+		uatomic_xchg(&bindings_file_changed, 1);
+		condlog(3, "%s: bindings file must be re-read, new timestamp: %ld.%06ld",
+			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
+	} else
+		condlog(3, "%s: bindings file is up-to-date, timestamp: %ld.%06ld",
+			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
+}
+
 static int update_bindings_file(const Bindings *bindings,
 				const char *bindings_file)
 {
@@ -237,6 +317,7 @@ static int update_bindings_file(const Bindings *bindings,
 	int fd = -1;
 	char tempname[PATH_MAX];
 	mode_t old_umask;
+	struct timespec ts;
 
 	if (safe_sprintf(tempname, "%s.XXXXXX", bindings_file))
 		return -1;
@@ -248,7 +329,7 @@ static int update_bindings_file(const Bindings *bindings,
 	}
 	umask(old_umask);
 	pthread_cleanup_push(cleanup_fd_ptr, &fd);
-	rc = write_bindings_file(bindings, fd);
+	rc = write_bindings_file(bindings, fd, &ts);
 	pthread_cleanup_pop(1);
 	if (rc == -1) {
 		condlog(1, "failed to write new bindings file");
@@ -257,8 +338,12 @@ static int update_bindings_file(const Bindings *bindings,
 	}
 	if ((rc = rename(tempname, bindings_file)) == -1)
 		condlog(0, "%s: rename: %m", __func__);
-	else
+	else {
+		pthread_mutex_lock(&timestamp_mutex);
+		bindings_last_updated = ts;
+		pthread_mutex_unlock(&timestamp_mutex);
 		condlog(1, "updated bindings file %s", bindings_file);
+	}
 	return rc;
 }
 
@@ -387,6 +472,7 @@ int get_free_id(const Bindings *bindings, const char *prefix, const char *map_ww
 	return id;
 }
 
+/* Called with binding_mutex held */
 static char *
 allocate_binding(const char *filename, const char *wwid, int id, const char *prefix)
 {
@@ -423,6 +509,30 @@ allocate_binding(const char *filename, const char *wwid, int id, const char *pre
 	return alias;
 }
 
+enum {
+	BINDINGS_FILE_UP2DATE,
+	BINDINGS_FILE_READ,
+	BINDINGS_FILE_ERROR,
+	BINDINGS_FILE_BAD,
+};
+
+static int _read_bindings_file(const struct config *conf, Bindings *bindings,
+			       bool force);
+
+static void read_bindings_file(void)
+{
+	struct config *conf;
+	Bindings bindings = {.allocated = 0, };
+	int rc;
+
+	conf = get_multipath_config();
+	pthread_cleanup_push(put_multipath_config, conf);
+	rc = _read_bindings_file(conf, &bindings, false);
+	pthread_cleanup_pop(1);
+	if (rc == BINDINGS_FILE_READ)
+		set_global_bindings(&bindings);
+}
+
 /*
  * get_user_friendly_alias() action table
  *
@@ -463,6 +573,11 @@ char *get_user_friendly_alias(const char *wwid, const char *file, const char *al
 	bool new_binding = false;
 	const struct binding *bdg;
 
+	read_bindings_file();
+
+	pthread_mutex_lock(&bindings_mutex);
+	pthread_cleanup_push(cleanup_mutex, &bindings_mutex);
+
 	if (!*alias_old)
 		goto new_alias;
 
@@ -514,40 +629,40 @@ new_alias:
 			alias, wwid);
 
 out:
+	/* unlock bindings_mutex */
+	pthread_cleanup_pop(1);
 	return alias;
 }
 
 int get_user_friendly_wwid(const char *alias, char *buff)
 {
 	const struct binding *bdg;
+	int rc = -1;
 
 	if (!alias || *alias == '\0') {
 		condlog(3, "Cannot find binding for empty alias");
 		return -1;
 	}
 
+	read_bindings_file();
+
+	pthread_mutex_lock(&bindings_mutex);
+	pthread_cleanup_push(cleanup_mutex, &bindings_mutex);
 	bdg = get_binding_for_alias(&global_bindings, alias);
-	if (!bdg) {
+	if (bdg) {
+		strlcpy(buff, bdg->wwid, WWID_SIZE);
+		rc = 0;
+	} else
 		*buff = '\0';
-		return -1;
-	}
-	strlcpy(buff, bdg->wwid, WWID_SIZE);
-	return 0;
-}
-
-static void free_bindings(Bindings *bindings)
-{
-	struct binding *bdg;
-	int i;
-
-	vector_foreach_slot(bindings, bdg, i)
-		_free_binding(bdg);
-	vector_reset(bindings);
+	pthread_cleanup_pop(1);
+	return rc;
 }
 
 void cleanup_bindings(void)
 {
+	pthread_mutex_lock(&bindings_mutex);
 	free_bindings(&global_bindings);
+	pthread_mutex_unlock(&bindings_mutex);
 }
 
 enum {
@@ -595,7 +710,20 @@ static int _check_bindings_file(const struct config *conf, FILE *file,
 	char *line = NULL;
 	size_t line_len = 0;
 	ssize_t n;
+	char header[sizeof(BINDINGS_FILE_HEADER)];
 
+	header[sizeof(BINDINGS_FILE_HEADER) - 1] = '\0';
+	if (fread(header, sizeof(BINDINGS_FILE_HEADER) - 1, 1, file) < 1) {
+		condlog(2, "%s: failed to read header from %s", __func__,
+			conf->bindings_file);
+		fseek(file, 0, SEEK_SET);
+		rc = -1;
+	} else if (strcmp(header, BINDINGS_FILE_HEADER)) {
+		condlog(2, "%s: invalid header in %s", __func__,
+			conf->bindings_file);
+		fseek(file, 0, SEEK_SET);
+		rc = -1;
+	}
 	pthread_cleanup_push(cleanup_free_ptr, &line);
 	while ((n = getline(&line, &line_len, file)) >= 0) {
 		char *alias, *wwid;
@@ -643,6 +771,68 @@ static int mp_alias_compar(const void *p1, const void *p2)
 			    &((*(struct mpentry * const *)p2)->alias));
 }
 
+static int _read_bindings_file(const struct config *conf, Bindings *bindings,
+			       bool force)
+{
+	int can_write;
+	int rc = 0, ret, fd;
+	FILE *file;
+	struct stat st;
+	int has_changed = uatomic_xchg(&bindings_file_changed, 0);
+
+	if (!force) {
+		if (!has_changed) {
+			condlog(4, "%s: bindings are unchanged", __func__);
+			return BINDINGS_FILE_UP2DATE;
+		}
+	}
+
+	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
+	if (fd == -1)
+		return BINDINGS_FILE_ERROR;
+
+	file = fdopen(fd, "r");
+	if (file != NULL) {
+		condlog(3, "%s: reading %s", __func__, conf->bindings_file);
+
+		pthread_cleanup_push(cleanup_fclose, file);
+		ret = _check_bindings_file(conf, file, bindings);
+		if (ret == 0) {
+			struct timespec ts;
+
+			rc = BINDINGS_FILE_READ;
+			ret = fstat(fd, &st);
+			if (ret == 0)
+				ts = st.st_mtim;
+			else {
+				condlog(1, "%s: fstat failed (%m), using current time", __func__);
+				clock_gettime(CLOCK_REALTIME_COARSE, &ts);
+			}
+			pthread_mutex_lock(&timestamp_mutex);
+			bindings_last_updated = ts;
+			pthread_mutex_unlock(&timestamp_mutex);
+		} else if (ret == -1 && can_write && !conf->bindings_read_only) {
+			ret = update_bindings_file(bindings, conf->bindings_file);
+			if (ret == 0)
+				rc = BINDINGS_FILE_READ;
+			else
+				rc = BINDINGS_FILE_BAD;
+		} else {
+			condlog(0, "ERROR: bad settings in read-only bindings file %s",
+				conf->bindings_file);
+			rc = BINDINGS_FILE_BAD;
+		}
+		pthread_cleanup_pop(1);
+	} else {
+		condlog(1, "failed to fdopen %s: %m",
+			conf->bindings_file);
+		close(fd);
+		rc = BINDINGS_FILE_ERROR;
+	}
+
+	return rc;
+}
+
 /*
  * check_alias_settings(): test for inconsistent alias configuration
  *
@@ -661,8 +851,7 @@ static int mp_alias_compar(const void *p1, const void *p2)
  */
 int check_alias_settings(const struct config *conf)
 {
-	int can_write;
-	int rc = 0, i, fd;
+	int i, rc;
 	Bindings bindings = {.allocated = 0, };
 	vector mptable = NULL;
 	struct mpentry *mpe;
@@ -695,27 +884,12 @@ int check_alias_settings(const struct config *conf)
 	pthread_cleanup_pop(1);
 	pthread_cleanup_pop(1);
 
-	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
-	if (fd != -1) {
-		FILE *file = fdopen(fd, "r");
+	rc = _read_bindings_file(conf, &bindings, true);
 
-		if (file != NULL) {
-			pthread_cleanup_push(cleanup_fclose, file);
-			rc = _check_bindings_file(conf, file, &bindings);
-			pthread_cleanup_pop(1);
-			if (rc == -1 && can_write && !conf->bindings_read_only)
-				rc = update_bindings_file(&bindings, conf->bindings_file);
-			else if (rc == -1)
-				condlog(0, "ERROR: bad settings in read-only bindings file %s",
-					conf->bindings_file);
-		} else {
-			condlog(1, "failed to fdopen %s: %m",
-				conf->bindings_file);
-			close(fd);
-		}
+	if (rc == BINDINGS_FILE_READ) {
+		set_global_bindings(&bindings);
+		rc = 0;
 	}
 
-	cleanup_bindings();
-	global_bindings = bindings;
 	return rc;
 }
diff --git a/libmultipath/alias.h b/libmultipath/alias.h
index 5ef6720..ca8911f 100644
--- a/libmultipath/alias.h
+++ b/libmultipath/alias.h
@@ -10,5 +10,6 @@ char *get_user_friendly_alias(const char *wwid, const char *file,
 struct config;
 int check_alias_settings(const struct config *);
 void cleanup_bindings(void);
-
+struct inotify_event;
+void handle_bindings_file_inotify(const struct inotify_event *event);
 #endif /* _ALIAS_H */
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index ddd302f..57e50c1 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -238,3 +238,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_20.1.0 {
+global:
+	handle_bindings_file_inotify;
+};
diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 02e89fb..d1f8f23 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -41,6 +41,7 @@
 #include "cli.h"
 #include "uxlsnr.h"
 #include "strbuf.h"
+#include "alias.h"
 
 /* state of client connection */
 enum {
@@ -190,6 +191,7 @@ void wakeup_cleanup(void *arg)
 struct watch_descriptors {
 	int conf_wd;
 	int dir_wd;
+	int mp_wd; /* /etc/multipath; for bindings file */
 };
 
 /* failing to set the watch descriptor is o.k. we just miss a warning
@@ -200,6 +202,8 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
 	struct config *conf;
 	int dir_reset = 0;
 	int conf_reset = 0;
+	int mp_reset = 0;
+	char *bindings_file __attribute__((cleanup(cleanup_charp))) = NULL;
 
 	if (notify_fd == -1)
 		return;
@@ -214,7 +218,10 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
 			conf_reset = 1;
 		if (wds->dir_wd == -1)
 			dir_reset = 1;
+		if (wds->mp_wd == -1)
+			mp_reset = 1;
 	}
+	bindings_file = strdup(conf->bindings_file);
 	put_multipath_config(conf);
 
 	if (dir_reset) {
@@ -235,7 +242,18 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
 		if (wds->conf_wd == -1)
 			condlog(3, "didn't set up notifications on /etc/multipath.conf: %m");
 	}
-	return;
+	if (mp_reset && bindings_file) {
+		char *slash = strrchr(bindings_file, '/');
+
+		if (slash && slash > bindings_file) {
+			*slash = '\0';
+			wds->mp_wd = inotify_add_watch(notify_fd, bindings_file,
+						       IN_MOVED_TO|IN_ONLYDIR);
+			if (wds->mp_wd == -1)
+				condlog(3, "didn't set up notifications on %s: %m",
+					bindings_file);
+		}
+	}
 }
 
 static void handle_inotify(int fd, struct watch_descriptors *wds)
@@ -256,12 +274,13 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 					inotify_rm_watch(fd, wds->conf_wd);
 				if (wds->dir_wd != -1)
 					inotify_rm_watch(fd, wds->dir_wd);
-				wds->conf_wd = wds->dir_wd = -1;
+				if (wds->mp_wd != -1)
+					inotify_rm_watch(fd, wds->mp_wd);
+				wds->conf_wd = wds->dir_wd = wds->mp_wd = -1;
 			}
 			break;
 		}
 
-		got_notify = 1;
 		for (ptr = buff; ptr < buff + len;
 		     ptr += sizeof(struct inotify_event) + event->len) {
 			event = (const struct inotify_event *) ptr;
@@ -273,7 +292,13 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
 					wds->conf_wd = inotify_add_watch(notify_fd, DEFAULT_CONFIGFILE, IN_CLOSE_WRITE);
 				else if (wds->dir_wd == event->wd)
 					wds->dir_wd = -1;
+				else if (wds->mp_wd == event->wd)
+					wds->mp_wd = -1;
 			}
+			if (wds->mp_wd != -1 && wds->mp_wd == event->wd)
+				handle_bindings_file_inotify(event);
+			else
+				got_notify = 1;
 		}
 	}
 	if (got_notify)
@@ -599,7 +624,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 	int max_pfds = MIN_POLLS + POLLFDS_BASE;
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
 	unsigned int sequence_nr = 0;
-	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
+	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1, .mp_wd = -1, };
 	struct vectors *vecs = trigger_data;
 
 	condlog(3, "uxsock: startup listener");
@@ -666,7 +691,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
 
 		reset_watch(notify_fd, &wds, &sequence_nr);
 		polls[POLLFD_NOTIFY].fd = notify_fd;
-		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
+		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1
+					&& wds.mp_wd == -1))
 			polls[POLLFD_NOTIFY].events = 0;
 		else
 			polls[POLLFD_NOTIFY].events = POLLIN;
diff --git a/tests/alias.c b/tests/alias.c
index 4d0adba..d41d396 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -1954,6 +1954,9 @@ int main(void)
 	int ret = 0;
 	init_test_verbosity(3);
 
+	/* avoid open_file() call in _read_bindings_file */
+	bindings_file_changed = 0;
+
 	ret += test_format_devname();
 	ret += test_scan_devname();
 	ret += test_lookup_binding();
-- 
2.42.0

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations
  2023-09-14 14:51 [dm-devel] [PATCH v3 00/38] multipath-tools: user-friendly names rework mwilck
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings mwilck
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 27/38] multipathd: watch bindings file with inotify + timestamp mwilck
@ 2023-09-14 14:51 ` mwilck
  2023-09-14 22:31   ` Benjamin Marzinski
  2 siblings, 1 reply; 7+ messages in thread
From: mwilck @ 2023-09-14 14:51 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

The use of uatomic_xchg() in alias.c causes a -Warray-bounds error
on distributions using gcc 12, such as Fedora 37. This is a similar
error to 2534c4f ("libmultipath: avoid -Warray-bounds error with gcc
12 and musl libc"). This happens only with liburcu 0.13 and earlier,
and only with certain gcc versions. See liburcu commit 835b9ab
("Fix: x86 and s390 uatomic: __hp() macro warning with gcc 11").

Enhance the fix for 2534c4f by a adding a workaround for uatomic_xchg(),
and introduce the macro URCU_VERSION (originally only used for multipathd)
globally.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile.inc         |  2 +-
 create-config.mk     |  5 +++++
 libmultipath/alias.c |  5 +++--
 libmultipath/lock.h  | 23 ++++++++++++++---------
 multipathd/Makefile  |  2 --
 5 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/Makefile.inc b/Makefile.inc
index 6e384e6..04bfa56 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -95,7 +95,7 @@ OPTFLAGS	:= -O2 -g $(STACKPROT) --param=ssp-buffer-size=4
 WARNFLAGS	:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) -Werror=implicit-int \
 		  -Werror=implicit-function-declaration -Werror=format-security \
 		  $(WNOCLOBBERED) -Werror=cast-qual $(ERROR_DISCARDED_QUALIFIERS) $(W_URCU_TYPE_LIMITS)
-CPPFLAGS	:= $(FORTIFY_OPT) $(CPPFLAGS) \
+CPPFLAGS	:= $(FORTIFY_OPT) $(CPPFLAGS) $(D_URCU_VERSION) \
 		   -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(plugindir)\" \
 		   -DRUNTIME_DIR=\"$(runtimedir)\" -DCONFIG_DIR=\"$(configdir)\" \
 		   -DDEFAULT_CONFIGFILE=\"$(configfile)\" -DSTATE_DIR=\"$(statedir)\" \
diff --git a/create-config.mk b/create-config.mk
index d125597..4d318b9 100644
--- a/create-config.mk
+++ b/create-config.mk
@@ -73,6 +73,10 @@ TEST_URCU_TYPE_LIMITS = $(shell \
 		$(CC) -c -Werror=type-limits -o /dev/null -xc - 2>/dev/null  \
 	|| echo -Wno-type-limits )
 
+URCU_VERSION = $(shell \
+	$(PKG_CONFIG) --modversion liburcu 2>/dev/null | \
+			awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + $$2) + $$3); }')
+
 DEFINES :=
 
 ifneq ($(call check_func,dm_task_no_flush,$(devmapper_incdir)/libdevmapper.h),0)
@@ -168,6 +172,7 @@ $(TOPDIR)/config.mk:	$(multipathdir)/autoconfig.h
 	@echo creating $@
 	@echo "FPIN_SUPPORT := $(FPIN_SUPPORT)" >$@
 	@echo "FORTIFY_OPT := $(FORTIFY_OPT)" >>$@
+	@echo "D_URCU_VERSION := $(call URCU_VERSION)" >>$@
 	@echo "SYSTEMD := $(SYSTEMD)" >>$@
 	@echo "ANA_SUPPORT := $(ANA_SUPPORT)" >>$@
 	@echo "STACKPROT := $(call TEST_CC_OPTION,-fstack-protector-strong,-fstack-protector)" >>$@
diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index e5d3f15..74431f3 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -24,6 +24,7 @@
 #include "devmapper.h"
 #include "strbuf.h"
 #include "time-util.h"
+#include "lock.h"
 
 /*
  * significant parts of this file were taken from iscsi-bindings.c of the
@@ -300,7 +301,7 @@ void handle_bindings_file_inotify(const struct inotify_event *event)
 	pthread_mutex_unlock(&timestamp_mutex);
 
 	if (changed) {
-		uatomic_xchg(&bindings_file_changed, 1);
+		uatomic_xchg_int(&bindings_file_changed, 1);
 		condlog(3, "%s: bindings file must be re-read, new timestamp: %ld.%06ld",
 			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
 	} else
@@ -775,7 +776,7 @@ static int _read_bindings_file(const struct config *conf, Bindings *bindings,
 	int rc = 0, ret, fd;
 	FILE *file;
 	struct stat st;
-	int has_changed = uatomic_xchg(&bindings_file_changed, 0);
+	int has_changed = uatomic_xchg_int(&bindings_file_changed, 0);
 
 	if (!force) {
 		if (!has_changed) {
diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 9814be7..ac80d1d 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -13,17 +13,22 @@ struct mutex_lock {
 	int waiters; /* uatomic access only */
 };
 
-#if !defined(__GLIBC__) && defined(__GNUC__) && __GNUC__ == 12
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Warray-bounds"
-#endif
-
 static inline void init_lock(struct mutex_lock *a)
 {
 	pthread_mutex_init(&a->mutex, NULL);
 	uatomic_set(&a->waiters, 0);
 }
 
+#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Warray-bounds"
+#endif
+
+static inline int uatomic_xchg_int(int *ptr, int val)
+{
+	return uatomic_xchg(ptr, val);
+}
+
 static inline void lock(struct mutex_lock *a)
 {
 	uatomic_inc(&a->waiters);
@@ -31,6 +36,10 @@ static inline void lock(struct mutex_lock *a)
 	uatomic_dec(&a->waiters);
 }
 
+#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
+#pragma GCC diagnostic pop
+#endif
+
 static inline int trylock(struct mutex_lock *a)
 {
 	return pthread_mutex_trylock(&a->mutex);
@@ -51,10 +60,6 @@ static inline bool lock_has_waiters(struct mutex_lock *a)
 	return (uatomic_read(&a->waiters) > 0);
 }
 
-#if !defined(__GLIBC__) && defined(__GNUC__) && __GNUC__ == 12
-#pragma GCC diagnostic pop
-#endif
-
 #define lock_cleanup_pop(a) pthread_cleanup_pop(1)
 
 void cleanup_lock (void * data);
diff --git a/multipathd/Makefile b/multipathd/Makefile
index cdba3db..0ba6ecb 100644
--- a/multipathd/Makefile
+++ b/multipathd/Makefile
@@ -5,8 +5,6 @@ CLI      := multipathc
 MANPAGES := multipathd.8
 
 CPPFLAGS += -I$(multipathdir) -I$(mpathutildir) -I$(mpathpersistdir) -I$(mpathcmddir) -I$(thirdpartydir) \
-	$(shell $(PKG_CONFIG) --modversion liburcu 2>/dev/null | \
-		awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + $$2) + $$3); }') \
 	-DBINDIR='"$(bindir)"' $(SYSTEMD_CPPFLAGS)
 
 #
-- 
2.42.0

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings mwilck
@ 2023-09-14 21:25   ` Benjamin Marzinski
  0 siblings, 0 replies; 7+ messages in thread
From: Benjamin Marzinski @ 2023-09-14 21:25 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 14, 2023 at 04:51:29PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> As the assignment of free aliases now relies on the bindings being
> properly sorted, add some unit tests to make sure the sorting algorithm
> works.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  tests/alias.c | 212 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 209 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/alias.c b/tests/alias.c
> index dff5f93..4d0adba 100644
> --- a/tests/alias.c
> +++ b/tests/alias.c
> @@ -13,6 +13,9 @@
>  #include "globals.c"
>  #include "../libmultipath/alias.c"
>  
> +/* For verbose printing of all aliases in the ordering tests */
> +#define ALIAS_DEBUG 0
> +
>  #if INT_MAX == 0x7fffffff
>  /* user_friendly_name for map #INT_MAX */
>  #define MPATH_ID_INT_MAX "fxshrxw"
> @@ -439,11 +442,12 @@ static void mock_self_alias(const char *alias, const char *wwid)
>  		expect_condlog(3, USED_STR(alias, wwid));		\
>  	} while(0)
>  
> -static void __mock_bindings_file(const char *content)
> +static void __mock_bindings_file(const char *content, bool conflict_ok)
>  {
>  	char *cnt __attribute__((cleanup(cleanup_charp))) = NULL;
>  	char *token, *savep = NULL;
>  	int i;
> +	uintmax_t values[] = { BINDING_ADDED, BINDING_CONFLICT };
>  
>  	cnt = strdup(content);
>  	assert_ptr_not_equal(cnt, NULL);
> @@ -459,12 +463,12 @@ static void __mock_bindings_file(const char *content)
>  			continue;
>  
>  		rc = add_binding(&global_bindings, alias, wwid);
> -		assert_int_equal(rc, BINDING_ADDED);
> +		assert_in_set(rc, values, conflict_ok ? 2 : 1);
>  	}
>  }
>  
>  static void mock_bindings_file(const char *content) {
> -	return __mock_bindings_file(content);
> +	return __mock_bindings_file(content, false);
>  }
>  
>  static int teardown_bindings(void **state)
> @@ -1744,6 +1748,207 @@ static int test_get_user_friendly_alias()
>  	return cmocka_run_group_tests(tests, NULL, NULL);
>  }
>  
> +/* Numbers 1-1000, randomly shuffled */
> +static int random_numbers[1000] = {
> +	694, 977, 224, 178, 841, 818, 914, 549, 831, 942, 263, 834, 919, 800,
> +	111, 517, 719, 297, 988, 98, 332, 516, 754, 772, 495, 488, 331, 529,
> +	142, 747, 848, 618, 375, 624, 74, 753, 782, 944, 623, 468, 862, 997,
> +	417, 258, 298, 774, 673, 904, 883, 766, 867, 400, 11, 950, 14, 784,
> +	655, 155, 396, 9, 743, 93, 651, 245, 968, 306, 785, 581, 880, 486,
> +	168, 631, 203, 4, 663, 294, 702, 762, 619, 684, 48, 181, 21, 443, 643,
> +	863, 1000, 327, 26, 126, 382, 765, 586, 76, 49, 925, 319, 865, 797,
> +	876, 693, 334, 433, 243, 419, 901, 854, 326, 985, 347, 874, 527, 282,
> +	290, 380, 167, 95, 3, 257, 936, 60, 426, 227, 345, 577, 492, 467, 580,
> +	967, 422, 823, 718, 610, 64, 700, 412, 163, 288, 506, 828, 432, 51,
> +	356, 348, 539, 478, 17, 945, 602, 123, 450, 660, 429, 113, 310, 358,
> +	512, 758, 508, 19, 542, 304, 286, 446, 918, 723, 333, 603, 731, 978,
> +	230, 697, 109, 872, 175, 853, 947, 965, 121, 222, 101, 811, 117, 601,
> +	191, 752, 384, 415, 938, 278, 915, 715, 240, 552, 912, 838, 150, 840,
> +	627, 29, 636, 464, 861, 481, 992, 249, 934, 82, 368, 724, 807, 593,
> +	157, 147, 199, 637, 41, 62, 902, 505, 621, 342, 174, 260, 729, 961,
> +	219, 311, 629, 789, 81, 739, 860, 712, 223, 165, 741, 981, 485, 363,
> +	346, 709, 125, 369, 279, 634, 399, 162, 193, 769, 149, 314, 868, 612,
> +	524, 675, 341, 343, 476, 606, 388, 613, 850, 264, 903, 451, 908, 779,
> +	453, 148, 497, 46, 132, 43, 885, 955, 269, 395, 72, 128, 767, 989,
> +	929, 423, 742, 55, 13, 79, 924, 182, 295, 563, 668, 169, 974, 154,
> +	970, 54, 674, 52, 437, 570, 550, 531, 554, 793, 678, 218, 367, 105,
> +	197, 315, 958, 892, 86, 47, 284, 37, 561, 522, 198, 689, 817, 573,
> +	877, 201, 803, 501, 881, 546, 530, 523, 780, 579, 953, 135, 23, 620,
> +	84, 698, 303, 656, 357, 323, 494, 58, 131, 913, 995, 120, 70, 1, 195,
> +	365, 210, 25, 898, 173, 307, 239, 77, 418, 952, 963, 92, 455, 425, 12,
> +	536, 161, 328, 933, 401, 251, 735, 725, 362, 322, 557, 681, 302, 53,
> +	786, 801, 391, 946, 748, 133, 717, 851, 7, 372, 993, 387, 906, 373,
> +	667, 33, 670, 389, 209, 611, 896, 652, 69, 999, 344, 845, 633, 36,
> +	487, 192, 180, 45, 640, 427, 707, 805, 188, 152, 905, 217, 30, 252,
> +	386, 665, 299, 541, 410, 787, 5, 857, 751, 392, 44, 595, 146, 745,
> +	641, 957, 866, 773, 806, 815, 659, 102, 704, 430, 106, 296, 129, 847,
> +	130, 990, 669, 236, 225, 680, 159, 213, 438, 189, 447, 600, 232, 594,
> +	32, 56, 390, 647, 855, 428, 330, 714, 738, 706, 666, 461, 469, 482,
> +	558, 814, 559, 177, 575, 538, 309, 383, 261, 156, 420, 761, 630, 893,
> +	10, 116, 940, 844, 71, 377, 662, 312, 520, 244, 143, 759, 119, 186,
> +	592, 909, 864, 376, 768, 254, 265, 394, 511, 760, 574, 6, 436, 514,
> +	59, 226, 644, 956, 578, 825, 548, 145, 736, 597, 378, 821, 987, 897,
> +	354, 144, 722, 895, 589, 503, 826, 498, 543, 617, 763, 231, 808, 528,
> +	89, 479, 607, 737, 170, 404, 371, 65, 103, 340, 283, 141, 313, 858,
> +	289, 124, 971, 687, 954, 732, 39, 926, 176, 100, 267, 519, 890, 535,
> +	276, 448, 27, 457, 899, 385, 184, 275, 770, 544, 614, 449, 160, 658,
> +	259, 973, 108, 604, 24, 207, 562, 757, 744, 324, 444, 962, 591, 480,
> +	398, 409, 998, 253, 325, 445, 979, 8, 35, 118, 73, 683, 208, 85, 190,
> +	791, 408, 871, 657, 179, 18, 556, 496, 475, 20, 894, 484, 775, 889,
> +	463, 241, 730, 57, 907, 551, 859, 943, 185, 416, 870, 590, 435, 471,
> +	932, 268, 381, 626, 502, 565, 273, 534, 672, 778, 292, 473, 566, 104,
> +	172, 285, 832, 411, 329, 628, 397, 472, 271, 910, 711, 690, 969, 585,
> +	809, 941, 923, 555, 228, 685, 242, 94, 96, 211, 140, 61, 922, 795,
> +	869, 34, 255, 38, 984, 676, 15, 560, 632, 434, 921, 355, 582, 351,
> +	212, 200, 819, 960, 649, 852, 75, 771, 361, 996, 238, 316, 720, 671,
> +	462, 112, 569, 171, 664, 625, 588, 405, 553, 270, 533, 353, 842, 114,
> +	972, 83, 937, 63, 194, 237, 537, 980, 802, 916, 959, 688, 839, 350,
> +	917, 650, 545, 615, 151, 352, 686, 726, 266, 509, 439, 491, 935, 608,
> +	518, 653, 339, 609, 277, 635, 836, 88, 407, 440, 642, 927, 229, 727,
> +	360, 477, 846, 413, 454, 616, 28, 598, 567, 540, 790, 424, 247, 317,
> +	746, 911, 798, 321, 547, 248, 734, 829, 220, 138, 756, 500, 691, 196,
> +	740, 930, 843, 733, 221, 827, 50, 813, 949, 525, 349, 474, 134, 875,
> +	695, 513, 414, 515, 638, 99, 366, 490, 975, 246, 465, 206, 281, 583,
> +	256, 587, 749, 2, 951, 679, 215, 364, 458, 402, 646, 991, 335, 982,
> +	835, 300, 900, 703, 994, 983, 234, 888, 532, 804, 584, 305, 792, 442,
> +	291, 964, 158, 370, 452, 250, 521, 166, 948, 812, 794, 272, 699, 205,
> +	183, 507, 301, 920, 781, 233, 824, 137, 489, 833, 887, 966, 856, 78,
> +	830, 153, 359, 696, 526, 216, 66, 701, 403, 891, 849, 571, 308, 483,
> +	164, 293, 928, 677, 320, 837, 441, 639, 564, 510, 648, 274, 336, 661,
> +	878, 777, 816, 976, 493, 810, 67, 87, 91, 187, 882, 986, 80, 22, 499,
> +	90, 705, 139, 136, 122, 708, 716, 886, 572, 127, 40, 721, 764, 16,
> +	379, 692, 645, 456, 710, 460, 783, 97, 776, 713, 884, 115, 466, 596,
> +	374, 406, 110, 568, 68, 214, 622, 470, 107, 504, 682, 31, 421, 576,
> +	654, 605, 788, 799, 280, 338, 931, 873, 204, 287, 459, 755, 939, 599,
> +	431, 796, 235, 42, 750, 262, 318, 393, 202, 822, 879, 820, 728, 337,
> +};
> +
> +static void fill_bindings_random(struct strbuf *buf, int start, int end,
> +				 const char *prefix)
> +{
> +	int i;
> +
> +	for (i = start; i < end; i++) {
> +		print_strbuf(buf,  "%s", prefix);
> +		format_devname(buf, random_numbers[i]);
> +		print_strbuf(buf,  " WWID%d\n", random_numbers[i]);
> +	}
> +}
> +
> +struct random_aliases {
> +	int start;
> +	int end;
> +	const char *prefix;
> +};
> +
> +static void order_test(int n, struct random_aliases ra[], bool conflict_ok)
> +{
> +	STRBUF_ON_STACK(buf);
> +	int i, j, prev, curr, tmp;
> +	struct binding *bdg;
> +	Bindings *bindings = &global_bindings;
> +
> +	for (j = 0; j < n; j++)
> +		fill_bindings_random(&buf, ra[j].start, ra[j].end, ra[j].prefix);
> +	__mock_bindings_file(get_strbuf_str(&buf), conflict_ok);
> +
> +	for (j = 0; j < n; j++) {
> +		bdg = VECTOR_SLOT(bindings, 0);
> +		if (ALIAS_DEBUG && j == 0)
> +			printf("%d: %s\n", 0, bdg->alias);
> +		prev = scan_devname(bdg->alias, ra[j].prefix);
> +		i = 1;
> +		vector_foreach_slot_after(bindings, bdg, i) {
> +			if (ALIAS_DEBUG && j == 0)
> +				printf("%d: %s\n", i, bdg->alias);
> +			tmp = scan_devname(bdg->alias, ra[j].prefix);
> +			if (tmp == -1)
> +				continue;
> +			curr = tmp;
> +			if (prev > 0) {
> +				if (curr <= prev)
> +					printf("ERROR: %d (%s) %d >= %d\n",
> +					       i, bdg->alias, prev, curr);
> +				assert_true(curr > prev);
> +			}
> +			prev = curr;
> +		}
> +	}
> +}
> +
> +static void order_01(void **state)
> +{
> +	struct random_aliases ra[] = {
> +		{  0, 1000, "MPATH" },
> +	};
> +
> +	order_test(ARRAY_SIZE(ra), ra, false);
> +}
> +
> +static void order_02(void **state)
> +{
> +	struct random_aliases ra[] = {
> +		{   0, 500, "MPATH" },
> +		{ 200, 700, "mpath" },
> +	};
> +	order_test(ARRAY_SIZE(ra), ra, false);
> +}
> +
> +static void order_03(void **state)
> +{
> +	struct random_aliases ra[] = {
> +		{  500, 1000, "MPTH" },
> +		{    0,  500, "MPATH" },
> +	};
> +	order_test(ARRAY_SIZE(ra), ra, false);
> +}
> +
> +static void order_04(void **state)
> +{
> +	struct random_aliases ra[] = {
> +		{   0, 500, "mpa" },
> +		{ 250, 750, "mp" },
> +	};
> +	order_test(ARRAY_SIZE(ra), ra, true);
> +}
> +
> +static void order_05(void **state)
> +{
> +	struct random_aliases ra[] = {
> +		{  0, 100, "A" },
> +		{  0, 100, "B" },
> +		{  0, 100, "C" },
> +		{  0, 100, "D" },
> +	};
> +	order_test(ARRAY_SIZE(ra), ra, false);
> +}
> +
> +static void order_06(void **state)
> +{
> +	struct random_aliases ra[] = {
> +		{  0, 100, "" },
> +		{  0, 100, "a" },
> +		{  0, 100, "aa" },
> +		{  0, 100, "ab" },
> +		{  0, 100, "aaa" },
> +	};
> +	order_test(ARRAY_SIZE(ra), ra, true);
> +}
> +
> +static int test_bindings_order()
> +{
> +	const struct CMUnitTest tests[] = {
> +		cmocka_unit_test_teardown(order_01, teardown_bindings),
> +		cmocka_unit_test_teardown(order_02, teardown_bindings),
> +		cmocka_unit_test_teardown(order_03, teardown_bindings),
> +		cmocka_unit_test_teardown(order_04, teardown_bindings),
> +		cmocka_unit_test_teardown(order_05, teardown_bindings),
> +		cmocka_unit_test_teardown(order_06, teardown_bindings),
> +	};
> +
> +	return cmocka_run_group_tests(tests, NULL, NULL);
> +}
> +
>  int main(void)
>  {
>  	int ret = 0;
> @@ -1755,6 +1960,7 @@ int main(void)
>  	ret += test_rlookup_binding();
>  	ret += test_allocate_binding();
>  	ret += test_get_user_friendly_alias();
> +	ret += test_bindings_order();
>  
>  	return ret;
>  }
> -- 
> 2.42.0
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dm-devel] [PATCH v3 27/38] multipathd: watch bindings file with inotify + timestamp
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 27/38] multipathd: watch bindings file with inotify + timestamp mwilck
@ 2023-09-14 22:28   ` Benjamin Marzinski
  0 siblings, 0 replies; 7+ messages in thread
From: Benjamin Marzinski @ 2023-09-14 22:28 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 14, 2023 at 04:51:30PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Since "libmultipath: keep bindings in memory", we don't re-read the
> bindings file after every modification. Add a notification mechanism
> that makes multipathd aware of changes to the bindings file. Because
> multipathd itself will change the bindings file, it must compare
> timestamps in order to avoid reading the file repeatedly.
> 
> Because select_alias() can be called from multiple thread contexts (uxlsnr,
> uevent handler), we need to add locking for the bindings file. The
> timestamp must also be protected by a lock, because it can't be read
> or written atomically.
> 
> Note: The notification mechanism expects the bindings file to be
> atomically replaced by rename(2). Changes must be made in a temporary file and
> applied using rename(2), as in update_bindings_file(). The inotify
> mechanism deliberately does not listen to close-after-write events
> that would be generated by editing the bindings file directly. This
> 
> Note also: new bindings will only be read from add_map_with_path(),
> i.e. either during reconfigure(), or when a new map is created during
> runtime. Existing maps will not be renamed if the binding file changes,
> unless the user runs "multipathd reconfigure". This is not a change
> wrt the previous code, but it should be mentioned anyway.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> 
> libmultipath: protect global_bindings with a mutex
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> 
> libmultipath: check timestamp of bindings file before reading it
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c              | 252 +++++++++++++++++++++++++-----
>  libmultipath/alias.h              |   3 +-
>  libmultipath/libmultipath.version |   5 +
>  multipathd/uxlsnr.c               |  36 ++++-
>  tests/alias.c                     |   3 +
>  5 files changed, 254 insertions(+), 45 deletions(-)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 66e34e3..964b8a7 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -10,6 +10,7 @@
>  #include <stdio.h>
>  #include <stdbool.h>
>  #include <assert.h>
> +#include <sys/inotify.h>
>  
>  #include "debug.h"
>  #include "util.h"
> @@ -22,6 +23,7 @@
>  #include "config.h"
>  #include "devmapper.h"
>  #include "strbuf.h"
> +#include "time-util.h"
>  
>  /*
>   * significant parts of this file were taken from iscsi-bindings.c of the
> @@ -50,6 +52,12 @@
>  "# alias wwid\n" \
>  "#\n"
>  
> +/* uatomic access only */
> +static int bindings_file_changed = 1;
> +
> +static pthread_mutex_t timestamp_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static struct timespec bindings_last_updated;
> +
>  struct binding {
>  	char *alias;
>  	char *wwid;
> @@ -60,6 +68,9 @@ struct binding {
>   * an abstract type.
>   */
>  typedef struct _vector Bindings;
> +
> +/* Protect global_bindings */
> +static pthread_mutex_t bindings_mutex = PTHREAD_MUTEX_INITIALIZER;
>  static Bindings global_bindings = { .allocated = 0 };
>  
>  enum {
> @@ -78,6 +89,27 @@ static void _free_binding(struct binding *bdg)
>  	free(bdg);
>  }
>  
> +static void free_bindings(Bindings *bindings)
> +{
> +	struct binding *bdg;
> +	int i;
> +
> +	vector_foreach_slot(bindings, bdg, i)
> +		_free_binding(bdg);
> +	vector_reset(bindings);
> +}
> +
> +static void set_global_bindings(Bindings *bindings)
> +{
> +	Bindings old_bindings;
> +
> +	pthread_mutex_lock(&bindings_mutex);
> +	old_bindings = global_bindings;
> +	global_bindings = *bindings;
> +	pthread_mutex_unlock(&bindings_mutex);
> +	free_bindings(&old_bindings);
> +}
> +
>  static const struct binding *get_binding_for_alias(const Bindings *bindings,
>  						   const char *alias)
>  {
> @@ -199,7 +231,8 @@ static int delete_binding(Bindings *bindings, const char *wwid)
>  	return BINDING_DELETED;
>  }
>  
> -static int write_bindings_file(const Bindings *bindings, int fd)
> +static int write_bindings_file(const Bindings *bindings, int fd,
> +			       struct timespec *ts)
>  {
>  	struct binding *bnd;
>  	STRBUF_ON_STACK(content);
> @@ -227,9 +260,56 @@ static int write_bindings_file(const Bindings *bindings, int fd)
>  		}
>  		len -= n;
>  	}
> +	fsync(fd);
> +	if (ts) {
> +		struct stat st;
> +
> +		if (fstat(fd, &st) == 0)
> +			*ts = st.st_mtim;
> +		else
> +			clock_gettime(CLOCK_REALTIME_COARSE, ts);
> +	}
>  	return 0;
>  }
>  
> +void handle_bindings_file_inotify(const struct inotify_event *event)
> +{
> +	struct config *conf;
> +	const char *base;
> +	bool changed = false;
> +	struct stat st;
> +	struct timespec ts = { 0, 0 };
> +	int ret;
> +
> +	if (!(event->mask & IN_MOVED_TO))
> +		return;
> +
> +	conf = get_multipath_config();
> +	base = strrchr(conf->bindings_file, '/');
> +	changed = base && base > conf->bindings_file &&
> +		!strcmp(base + 1, event->name);
> +	ret = stat(conf->bindings_file, &st);
> +	put_multipath_config(conf);
> +
> +	if (!changed)
> +		return;
> +
> +	pthread_mutex_lock(&timestamp_mutex);
> +	if (ret == 0) {
> +		ts = st.st_mtim;
> +		changed = timespeccmp(&ts, &bindings_last_updated) > 0;
> +	}
> +	pthread_mutex_unlock(&timestamp_mutex);
> +
> +	if (changed) {
> +		uatomic_xchg(&bindings_file_changed, 1);
> +		condlog(3, "%s: bindings file must be re-read, new timestamp: %ld.%06ld",
> +			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
> +	} else
> +		condlog(3, "%s: bindings file is up-to-date, timestamp: %ld.%06ld",
> +			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
> +}
> +
>  static int update_bindings_file(const Bindings *bindings,
>  				const char *bindings_file)
>  {
> @@ -237,6 +317,7 @@ static int update_bindings_file(const Bindings *bindings,
>  	int fd = -1;
>  	char tempname[PATH_MAX];
>  	mode_t old_umask;
> +	struct timespec ts;
>  
>  	if (safe_sprintf(tempname, "%s.XXXXXX", bindings_file))
>  		return -1;
> @@ -248,7 +329,7 @@ static int update_bindings_file(const Bindings *bindings,
>  	}
>  	umask(old_umask);
>  	pthread_cleanup_push(cleanup_fd_ptr, &fd);
> -	rc = write_bindings_file(bindings, fd);
> +	rc = write_bindings_file(bindings, fd, &ts);
>  	pthread_cleanup_pop(1);
>  	if (rc == -1) {
>  		condlog(1, "failed to write new bindings file");
> @@ -257,8 +338,12 @@ static int update_bindings_file(const Bindings *bindings,
>  	}
>  	if ((rc = rename(tempname, bindings_file)) == -1)
>  		condlog(0, "%s: rename: %m", __func__);
> -	else
> +	else {
> +		pthread_mutex_lock(&timestamp_mutex);
> +		bindings_last_updated = ts;
> +		pthread_mutex_unlock(&timestamp_mutex);
>  		condlog(1, "updated bindings file %s", bindings_file);
> +	}
>  	return rc;
>  }
>  
> @@ -387,6 +472,7 @@ int get_free_id(const Bindings *bindings, const char *prefix, const char *map_ww
>  	return id;
>  }
>  
> +/* Called with binding_mutex held */
>  static char *
>  allocate_binding(const char *filename, const char *wwid, int id, const char *prefix)
>  {
> @@ -423,6 +509,30 @@ allocate_binding(const char *filename, const char *wwid, int id, const char *pre
>  	return alias;
>  }
>  
> +enum {
> +	BINDINGS_FILE_UP2DATE,
> +	BINDINGS_FILE_READ,
> +	BINDINGS_FILE_ERROR,
> +	BINDINGS_FILE_BAD,
> +};
> +
> +static int _read_bindings_file(const struct config *conf, Bindings *bindings,
> +			       bool force);
> +
> +static void read_bindings_file(void)
> +{
> +	struct config *conf;
> +	Bindings bindings = {.allocated = 0, };
> +	int rc;
> +
> +	conf = get_multipath_config();
> +	pthread_cleanup_push(put_multipath_config, conf);
> +	rc = _read_bindings_file(conf, &bindings, false);
> +	pthread_cleanup_pop(1);
> +	if (rc == BINDINGS_FILE_READ)
> +		set_global_bindings(&bindings);
> +}
> +
>  /*
>   * get_user_friendly_alias() action table
>   *
> @@ -463,6 +573,11 @@ char *get_user_friendly_alias(const char *wwid, const char *file, const char *al
>  	bool new_binding = false;
>  	const struct binding *bdg;
>  
> +	read_bindings_file();
> +
> +	pthread_mutex_lock(&bindings_mutex);
> +	pthread_cleanup_push(cleanup_mutex, &bindings_mutex);
> +
>  	if (!*alias_old)
>  		goto new_alias;
>  
> @@ -514,40 +629,40 @@ new_alias:
>  			alias, wwid);
>  
>  out:
> +	/* unlock bindings_mutex */
> +	pthread_cleanup_pop(1);
>  	return alias;
>  }
>  
>  int get_user_friendly_wwid(const char *alias, char *buff)
>  {
>  	const struct binding *bdg;
> +	int rc = -1;
>  
>  	if (!alias || *alias == '\0') {
>  		condlog(3, "Cannot find binding for empty alias");
>  		return -1;
>  	}
>  
> +	read_bindings_file();
> +
> +	pthread_mutex_lock(&bindings_mutex);
> +	pthread_cleanup_push(cleanup_mutex, &bindings_mutex);
>  	bdg = get_binding_for_alias(&global_bindings, alias);
> -	if (!bdg) {
> +	if (bdg) {
> +		strlcpy(buff, bdg->wwid, WWID_SIZE);
> +		rc = 0;
> +	} else
>  		*buff = '\0';
> -		return -1;
> -	}
> -	strlcpy(buff, bdg->wwid, WWID_SIZE);
> -	return 0;
> -}
> -
> -static void free_bindings(Bindings *bindings)
> -{
> -	struct binding *bdg;
> -	int i;
> -
> -	vector_foreach_slot(bindings, bdg, i)
> -		_free_binding(bdg);
> -	vector_reset(bindings);
> +	pthread_cleanup_pop(1);
> +	return rc;
>  }
>  
>  void cleanup_bindings(void)
>  {
> +	pthread_mutex_lock(&bindings_mutex);
>  	free_bindings(&global_bindings);
> +	pthread_mutex_unlock(&bindings_mutex);
>  }
>  
>  enum {
> @@ -595,7 +710,20 @@ static int _check_bindings_file(const struct config *conf, FILE *file,
>  	char *line = NULL;
>  	size_t line_len = 0;
>  	ssize_t n;
> +	char header[sizeof(BINDINGS_FILE_HEADER)];
>  
> +	header[sizeof(BINDINGS_FILE_HEADER) - 1] = '\0';
> +	if (fread(header, sizeof(BINDINGS_FILE_HEADER) - 1, 1, file) < 1) {
> +		condlog(2, "%s: failed to read header from %s", __func__,
> +			conf->bindings_file);
> +		fseek(file, 0, SEEK_SET);
> +		rc = -1;
> +	} else if (strcmp(header, BINDINGS_FILE_HEADER)) {
> +		condlog(2, "%s: invalid header in %s", __func__,
> +			conf->bindings_file);
> +		fseek(file, 0, SEEK_SET);
> +		rc = -1;
> +	}
>  	pthread_cleanup_push(cleanup_free_ptr, &line);
>  	while ((n = getline(&line, &line_len, file)) >= 0) {
>  		char *alias, *wwid;
> @@ -643,6 +771,68 @@ static int mp_alias_compar(const void *p1, const void *p2)
>  			    &((*(struct mpentry * const *)p2)->alias));
>  }
>  
> +static int _read_bindings_file(const struct config *conf, Bindings *bindings,
> +			       bool force)
> +{
> +	int can_write;
> +	int rc = 0, ret, fd;
> +	FILE *file;
> +	struct stat st;
> +	int has_changed = uatomic_xchg(&bindings_file_changed, 0);
> +
> +	if (!force) {
> +		if (!has_changed) {
> +			condlog(4, "%s: bindings are unchanged", __func__);
> +			return BINDINGS_FILE_UP2DATE;
> +		}
> +	}
> +
> +	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
> +	if (fd == -1)
> +		return BINDINGS_FILE_ERROR;
> +
> +	file = fdopen(fd, "r");
> +	if (file != NULL) {
> +		condlog(3, "%s: reading %s", __func__, conf->bindings_file);
> +
> +		pthread_cleanup_push(cleanup_fclose, file);
> +		ret = _check_bindings_file(conf, file, bindings);
> +		if (ret == 0) {
> +			struct timespec ts;
> +
> +			rc = BINDINGS_FILE_READ;
> +			ret = fstat(fd, &st);
> +			if (ret == 0)
> +				ts = st.st_mtim;
> +			else {
> +				condlog(1, "%s: fstat failed (%m), using current time", __func__);
> +				clock_gettime(CLOCK_REALTIME_COARSE, &ts);
> +			}
> +			pthread_mutex_lock(&timestamp_mutex);
> +			bindings_last_updated = ts;
> +			pthread_mutex_unlock(&timestamp_mutex);
> +		} else if (ret == -1 && can_write && !conf->bindings_read_only) {
> +			ret = update_bindings_file(bindings, conf->bindings_file);
> +			if (ret == 0)
> +				rc = BINDINGS_FILE_READ;
> +			else
> +				rc = BINDINGS_FILE_BAD;
> +		} else {
> +			condlog(0, "ERROR: bad settings in read-only bindings file %s",
> +				conf->bindings_file);
> +			rc = BINDINGS_FILE_BAD;
> +		}
> +		pthread_cleanup_pop(1);
> +	} else {
> +		condlog(1, "failed to fdopen %s: %m",
> +			conf->bindings_file);
> +		close(fd);
> +		rc = BINDINGS_FILE_ERROR;
> +	}
> +
> +	return rc;
> +}
> +
>  /*
>   * check_alias_settings(): test for inconsistent alias configuration
>   *
> @@ -661,8 +851,7 @@ static int mp_alias_compar(const void *p1, const void *p2)
>   */
>  int check_alias_settings(const struct config *conf)
>  {
> -	int can_write;
> -	int rc = 0, i, fd;
> +	int i, rc;
>  	Bindings bindings = {.allocated = 0, };
>  	vector mptable = NULL;
>  	struct mpentry *mpe;
> @@ -695,27 +884,12 @@ int check_alias_settings(const struct config *conf)
>  	pthread_cleanup_pop(1);
>  	pthread_cleanup_pop(1);
>  
> -	fd = open_file(conf->bindings_file, &can_write, BINDINGS_FILE_HEADER);
> -	if (fd != -1) {
> -		FILE *file = fdopen(fd, "r");
> +	rc = _read_bindings_file(conf, &bindings, true);
>  
> -		if (file != NULL) {
> -			pthread_cleanup_push(cleanup_fclose, file);
> -			rc = _check_bindings_file(conf, file, &bindings);
> -			pthread_cleanup_pop(1);
> -			if (rc == -1 && can_write && !conf->bindings_read_only)
> -				rc = update_bindings_file(&bindings, conf->bindings_file);
> -			else if (rc == -1)
> -				condlog(0, "ERROR: bad settings in read-only bindings file %s",
> -					conf->bindings_file);
> -		} else {
> -			condlog(1, "failed to fdopen %s: %m",
> -				conf->bindings_file);
> -			close(fd);
> -		}
> +	if (rc == BINDINGS_FILE_READ) {
> +		set_global_bindings(&bindings);
> +		rc = 0;
>  	}
>  
> -	cleanup_bindings();
> -	global_bindings = bindings;
>  	return rc;
>  }
> diff --git a/libmultipath/alias.h b/libmultipath/alias.h
> index 5ef6720..ca8911f 100644
> --- a/libmultipath/alias.h
> +++ b/libmultipath/alias.h
> @@ -10,5 +10,6 @@ char *get_user_friendly_alias(const char *wwid, const char *file,
>  struct config;
>  int check_alias_settings(const struct config *);
>  void cleanup_bindings(void);
> -
> +struct inotify_event;
> +void handle_bindings_file_inotify(const struct inotify_event *event);
>  #endif /* _ALIAS_H */
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index ddd302f..57e50c1 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -238,3 +238,8 @@ global:
>  local:
>  	*;
>  };
> +
> +LIBMULTIPATH_20.1.0 {
> +global:
> +	handle_bindings_file_inotify;
> +};
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 02e89fb..d1f8f23 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -41,6 +41,7 @@
>  #include "cli.h"
>  #include "uxlsnr.h"
>  #include "strbuf.h"
> +#include "alias.h"
>  
>  /* state of client connection */
>  enum {
> @@ -190,6 +191,7 @@ void wakeup_cleanup(void *arg)
>  struct watch_descriptors {
>  	int conf_wd;
>  	int dir_wd;
> +	int mp_wd; /* /etc/multipath; for bindings file */
>  };
>  
>  /* failing to set the watch descriptor is o.k. we just miss a warning
> @@ -200,6 +202,8 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
>  	struct config *conf;
>  	int dir_reset = 0;
>  	int conf_reset = 0;
> +	int mp_reset = 0;
> +	char *bindings_file __attribute__((cleanup(cleanup_charp))) = NULL;
>  
>  	if (notify_fd == -1)
>  		return;
> @@ -214,7 +218,10 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
>  			conf_reset = 1;
>  		if (wds->dir_wd == -1)
>  			dir_reset = 1;
> +		if (wds->mp_wd == -1)
> +			mp_reset = 1;
>  	}
> +	bindings_file = strdup(conf->bindings_file);
>  	put_multipath_config(conf);
>  
>  	if (dir_reset) {
> @@ -235,7 +242,18 @@ static void reset_watch(int notify_fd, struct watch_descriptors *wds,
>  		if (wds->conf_wd == -1)
>  			condlog(3, "didn't set up notifications on /etc/multipath.conf: %m");
>  	}
> -	return;
> +	if (mp_reset && bindings_file) {
> +		char *slash = strrchr(bindings_file, '/');
> +
> +		if (slash && slash > bindings_file) {
> +			*slash = '\0';
> +			wds->mp_wd = inotify_add_watch(notify_fd, bindings_file,
> +						       IN_MOVED_TO|IN_ONLYDIR);
> +			if (wds->mp_wd == -1)
> +				condlog(3, "didn't set up notifications on %s: %m",
> +					bindings_file);
> +		}
> +	}
>  }
>  
>  static void handle_inotify(int fd, struct watch_descriptors *wds)
> @@ -256,12 +274,13 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
>  					inotify_rm_watch(fd, wds->conf_wd);
>  				if (wds->dir_wd != -1)
>  					inotify_rm_watch(fd, wds->dir_wd);
> -				wds->conf_wd = wds->dir_wd = -1;
> +				if (wds->mp_wd != -1)
> +					inotify_rm_watch(fd, wds->mp_wd);
> +				wds->conf_wd = wds->dir_wd = wds->mp_wd = -1;
>  			}
>  			break;
>  		}
>  
> -		got_notify = 1;
>  		for (ptr = buff; ptr < buff + len;
>  		     ptr += sizeof(struct inotify_event) + event->len) {
>  			event = (const struct inotify_event *) ptr;
> @@ -273,7 +292,13 @@ static void handle_inotify(int fd, struct watch_descriptors *wds)
>  					wds->conf_wd = inotify_add_watch(notify_fd, DEFAULT_CONFIGFILE, IN_CLOSE_WRITE);
>  				else if (wds->dir_wd == event->wd)
>  					wds->dir_wd = -1;
> +				else if (wds->mp_wd == event->wd)
> +					wds->mp_wd = -1;
>  			}
> +			if (wds->mp_wd != -1 && wds->mp_wd == event->wd)
> +				handle_bindings_file_inotify(event);
> +			else
> +				got_notify = 1;
>  		}
>  	}
>  	if (got_notify)
> @@ -599,7 +624,7 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  	int max_pfds = MIN_POLLS + POLLFDS_BASE;
>  	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
>  	unsigned int sequence_nr = 0;
> -	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
> +	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1, .mp_wd = -1, };
>  	struct vectors *vecs = trigger_data;
>  
>  	condlog(3, "uxsock: startup listener");
> @@ -666,7 +691,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data)
>  
>  		reset_watch(notify_fd, &wds, &sequence_nr);
>  		polls[POLLFD_NOTIFY].fd = notify_fd;
> -		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
> +		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1
> +					&& wds.mp_wd == -1))
>  			polls[POLLFD_NOTIFY].events = 0;
>  		else
>  			polls[POLLFD_NOTIFY].events = POLLIN;
> diff --git a/tests/alias.c b/tests/alias.c
> index 4d0adba..d41d396 100644
> --- a/tests/alias.c
> +++ b/tests/alias.c
> @@ -1954,6 +1954,9 @@ int main(void)
>  	int ret = 0;
>  	init_test_verbosity(3);
>  
> +	/* avoid open_file() call in _read_bindings_file */
> +	bindings_file_changed = 0;
> +
>  	ret += test_format_devname();
>  	ret += test_scan_devname();
>  	ret += test_lookup_binding();
> -- 
> 2.42.0
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations
  2023-09-14 14:51 ` [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations mwilck
@ 2023-09-14 22:31   ` Benjamin Marzinski
  0 siblings, 0 replies; 7+ messages in thread
From: Benjamin Marzinski @ 2023-09-14 22:31 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Sep 14, 2023 at 04:51:31PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The use of uatomic_xchg() in alias.c causes a -Warray-bounds error
> on distributions using gcc 12, such as Fedora 37. This is a similar
> error to 2534c4f ("libmultipath: avoid -Warray-bounds error with gcc
> 12 and musl libc"). This happens only with liburcu 0.13 and earlier,
> and only with certain gcc versions. See liburcu commit 835b9ab
> ("Fix: x86 and s390 uatomic: __hp() macro warning with gcc 11").
> 
> Enhance the fix for 2534c4f by a adding a workaround for uatomic_xchg(),
> and introduce the macro URCU_VERSION (originally only used for multipathd)
> globally.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  Makefile.inc         |  2 +-
>  create-config.mk     |  5 +++++
>  libmultipath/alias.c |  5 +++--
>  libmultipath/lock.h  | 23 ++++++++++++++---------
>  multipathd/Makefile  |  2 --
>  5 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/Makefile.inc b/Makefile.inc
> index 6e384e6..04bfa56 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -95,7 +95,7 @@ OPTFLAGS	:= -O2 -g $(STACKPROT) --param=ssp-buffer-size=4
>  WARNFLAGS	:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) -Werror=implicit-int \
>  		  -Werror=implicit-function-declaration -Werror=format-security \
>  		  $(WNOCLOBBERED) -Werror=cast-qual $(ERROR_DISCARDED_QUALIFIERS) $(W_URCU_TYPE_LIMITS)
> -CPPFLAGS	:= $(FORTIFY_OPT) $(CPPFLAGS) \
> +CPPFLAGS	:= $(FORTIFY_OPT) $(CPPFLAGS) $(D_URCU_VERSION) \
>  		   -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(plugindir)\" \
>  		   -DRUNTIME_DIR=\"$(runtimedir)\" -DCONFIG_DIR=\"$(configdir)\" \
>  		   -DDEFAULT_CONFIGFILE=\"$(configfile)\" -DSTATE_DIR=\"$(statedir)\" \
> diff --git a/create-config.mk b/create-config.mk
> index d125597..4d318b9 100644
> --- a/create-config.mk
> +++ b/create-config.mk
> @@ -73,6 +73,10 @@ TEST_URCU_TYPE_LIMITS = $(shell \
>  		$(CC) -c -Werror=type-limits -o /dev/null -xc - 2>/dev/null  \
>  	|| echo -Wno-type-limits )
>  
> +URCU_VERSION = $(shell \
> +	$(PKG_CONFIG) --modversion liburcu 2>/dev/null | \
> +			awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + $$2) + $$3); }')
> +
>  DEFINES :=
>  
>  ifneq ($(call check_func,dm_task_no_flush,$(devmapper_incdir)/libdevmapper.h),0)
> @@ -168,6 +172,7 @@ $(TOPDIR)/config.mk:	$(multipathdir)/autoconfig.h
>  	@echo creating $@
>  	@echo "FPIN_SUPPORT := $(FPIN_SUPPORT)" >$@
>  	@echo "FORTIFY_OPT := $(FORTIFY_OPT)" >>$@
> +	@echo "D_URCU_VERSION := $(call URCU_VERSION)" >>$@
>  	@echo "SYSTEMD := $(SYSTEMD)" >>$@
>  	@echo "ANA_SUPPORT := $(ANA_SUPPORT)" >>$@
>  	@echo "STACKPROT := $(call TEST_CC_OPTION,-fstack-protector-strong,-fstack-protector)" >>$@
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index e5d3f15..74431f3 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -24,6 +24,7 @@
>  #include "devmapper.h"
>  #include "strbuf.h"
>  #include "time-util.h"
> +#include "lock.h"
>  
>  /*
>   * significant parts of this file were taken from iscsi-bindings.c of the
> @@ -300,7 +301,7 @@ void handle_bindings_file_inotify(const struct inotify_event *event)
>  	pthread_mutex_unlock(&timestamp_mutex);
>  
>  	if (changed) {
> -		uatomic_xchg(&bindings_file_changed, 1);
> +		uatomic_xchg_int(&bindings_file_changed, 1);
>  		condlog(3, "%s: bindings file must be re-read, new timestamp: %ld.%06ld",
>  			__func__, (long)ts.tv_sec, (long)ts.tv_nsec / 1000);
>  	} else
> @@ -775,7 +776,7 @@ static int _read_bindings_file(const struct config *conf, Bindings *bindings,
>  	int rc = 0, ret, fd;
>  	FILE *file;
>  	struct stat st;
> -	int has_changed = uatomic_xchg(&bindings_file_changed, 0);
> +	int has_changed = uatomic_xchg_int(&bindings_file_changed, 0);
>  
>  	if (!force) {
>  		if (!has_changed) {
> diff --git a/libmultipath/lock.h b/libmultipath/lock.h
> index 9814be7..ac80d1d 100644
> --- a/libmultipath/lock.h
> +++ b/libmultipath/lock.h
> @@ -13,17 +13,22 @@ struct mutex_lock {
>  	int waiters; /* uatomic access only */
>  };
>  
> -#if !defined(__GLIBC__) && defined(__GNUC__) && __GNUC__ == 12
> -#pragma GCC diagnostic push
> -#pragma GCC diagnostic ignored "-Warray-bounds"
> -#endif
> -
>  static inline void init_lock(struct mutex_lock *a)
>  {
>  	pthread_mutex_init(&a->mutex, NULL);
>  	uatomic_set(&a->waiters, 0);
>  }
>  
> +#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Warray-bounds"
> +#endif
> +
> +static inline int uatomic_xchg_int(int *ptr, int val)
> +{
> +	return uatomic_xchg(ptr, val);
> +}
> +
>  static inline void lock(struct mutex_lock *a)
>  {
>  	uatomic_inc(&a->waiters);
> @@ -31,6 +36,10 @@ static inline void lock(struct mutex_lock *a)
>  	uatomic_dec(&a->waiters);
>  }
>  
> +#if defined(__GNUC__) && __GNUC__ == 12 && URCU_VERSION < 0xe00
> +#pragma GCC diagnostic pop
> +#endif
> +
>  static inline int trylock(struct mutex_lock *a)
>  {
>  	return pthread_mutex_trylock(&a->mutex);
> @@ -51,10 +60,6 @@ static inline bool lock_has_waiters(struct mutex_lock *a)
>  	return (uatomic_read(&a->waiters) > 0);
>  }
>  
> -#if !defined(__GLIBC__) && defined(__GNUC__) && __GNUC__ == 12
> -#pragma GCC diagnostic pop
> -#endif
> -
>  #define lock_cleanup_pop(a) pthread_cleanup_pop(1)
>  
>  void cleanup_lock (void * data);
> diff --git a/multipathd/Makefile b/multipathd/Makefile
> index cdba3db..0ba6ecb 100644
> --- a/multipathd/Makefile
> +++ b/multipathd/Makefile
> @@ -5,8 +5,6 @@ CLI      := multipathc
>  MANPAGES := multipathd.8
>  
>  CPPFLAGS += -I$(multipathdir) -I$(mpathutildir) -I$(mpathpersistdir) -I$(mpathcmddir) -I$(thirdpartydir) \
> -	$(shell $(PKG_CONFIG) --modversion liburcu 2>/dev/null | \
> -		awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + $$2) + $$3); }') \
>  	-DBINDIR='"$(bindir)"' $(SYSTEMD_CPPFLAGS)
>  
>  #
> -- 
> 2.42.0
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-09-15  6:45 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-14 14:51 [dm-devel] [PATCH v3 00/38] multipath-tools: user-friendly names rework mwilck
2023-09-14 14:51 ` [dm-devel] [PATCH v3 26/38] multipath-tools tests: add test for ordering of bindings mwilck
2023-09-14 21:25   ` Benjamin Marzinski
2023-09-14 14:51 ` [dm-devel] [PATCH v3 27/38] multipathd: watch bindings file with inotify + timestamp mwilck
2023-09-14 22:28   ` Benjamin Marzinski
2023-09-14 14:51 ` [dm-devel] [PATCH v3 38/38] libmultipath: avoid -Warray-bounds error in uatomic operations mwilck
2023-09-14 22:31   ` Benjamin Marzinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox