[v2,09/14] libmpathutil: remove systemd_service_enabled()

Commit Message

Martin Wilck Oct. 26, 2023, 5:41 p.m. UTC

From: Martin Wilck <[e-mail]>

Since  c203929 ("multipathd.service: add dependency on
systemd-udevd-kernel.socket"), multipathd will start early during boot.
Moreover, we recommend using socket activation for multipathd,
and if multipathd.socket is enabled, the __mpath_connect()
check will succeed anyway.

The systemd_service_enabled() test was just "good enough" for
standard situations but never robust; it checked for multipathd.wants in
"some" directory, which might or might not be the current target,
and it would return true even of multipathd.service was masked.

Remove this test.

Signed-off-by: Martin Wilck <[e-mail]>
Reviewed-by: Benjamin Marzinski <[e-mail]>
---
 libmpathutil/libmpathutil.version | 17 +++------
 libmpathutil/util.c               | 58 -------------------------------
 libmpathutil/util.h               |  1 -
 libmultipath/valid.c              | 16 ++-------
 tests/valid.c                     | 24 ++-----------
 5 files changed, 9 insertions(+), 107 deletions(-)

Patch

diff --git a/libmpathutil/libmpathutil.version b/libmpathutil/libmpathutil.version
index 6ebb718..15ff467 100644
--- a/libmpathutil/libmpathutil.version
+++ b/libmpathutil/libmpathutil.version
@@ -93,12 +93,15 @@  local:
 };
 
 /* symbols referenced internally by libmultipath */
-LIBMPATHUTIL_1.0 {
+LIBMPATHUTIL_2.0 {
 	alloc_bitfield;
 	__append_strbuf_str;
 	append_strbuf_quoted;
 	basenamecpy;
+	cleanup_fd_ptr;
 	cleanup_free_ptr;
+	cleanup_vector_free;
+	cleanup_fclose;
 	filepresent;
 	find_keyword;
 	free_keywords;
@@ -120,21 +123,9 @@  LIBMPATHUTIL_1.0 {
 	snprint_keyword;
 	steal_strbuf_str;
 	strlcat;
-	systemd_service_enabled;
 	validate_config_strvec;
 	vector_find_or_add_slot;
 	vector_insert_slot;
 	vector_move_up;
 	vector_sort;
 };
-
-LIBMPATHUTIL_1.1 {
-global:
-	cleanup_fd_ptr;
-} LIBMPATHUTIL_1.0;
-
-LIBMPATHUTIL_1.2 {
-global:
-	cleanup_vector_free;
-	cleanup_fclose;
-} LIBMPATHUTIL_1.0;
diff --git a/libmpathutil/util.c b/libmpathutil/util.c
index 92f25a5..9d147fc 100644
--- a/libmpathutil/util.c
+++ b/libmpathutil/util.c
@@ -213,64 +213,6 @@  setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
 	}
 }
 
-int systemd_service_enabled_in(const char *dev, const char *prefix)
-{
-	static const char service[] = "multipathd.service";
-	char path[PATH_MAX], file[PATH_MAX];
-	DIR *dirfd;
-	struct dirent *d;
-	int found = 0;
-
-	if (safe_sprintf(path, "%s/systemd/system", prefix))
-		return 0;
-
-	condlog(3, "%s: checking for %s in %s", dev, service, path);
-
-	dirfd = opendir(path);
-	if (dirfd == NULL)
-		return 0;
-
-	while ((d = readdir(dirfd)) != NULL) {
-		char *p;
-		struct stat stbuf;
-
-		if ((strcmp(d->d_name,".") == 0) ||
-		    (strcmp(d->d_name,"..") == 0))
-			continue;
-
-		if (strlen(d->d_name) < 6)
-			continue;
-
-		p = d->d_name + strlen(d->d_name) - 6;
-		if (strcmp(p, ".wants"))
-			continue;
-		if (!safe_sprintf(file, "%s/%s/%s",
-				  path, d->d_name, service)
-		    && stat(file, &stbuf) == 0) {
-			condlog(3, "%s: found %s", dev, file);
-			found++;
-			break;
-		}
-	}
-	closedir(dirfd);
-
-	return found;
-}
-
-int systemd_service_enabled(const char *dev)
-{
-	int found = 0;
-
-	found = systemd_service_enabled_in(dev, "/etc");
-	if (!found)
-		found = systemd_service_enabled_in(dev, "/usr/lib");
-	if (!found)
-		found = systemd_service_enabled_in(dev, "/lib");
-	if (!found)
-		found = systemd_service_enabled_in(dev, "/run");
-	return found;
-}
-
 static int _linux_version_code;
 static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
 
diff --git a/libmpathutil/util.h b/libmpathutil/util.h
index 99a471d..de9fcfd 100644
--- a/libmpathutil/util.h
+++ b/libmpathutil/util.h
@@ -21,7 +21,6 @@  size_t strlcat(char * restrict dst, const char * restrict src, size_t size);
 dev_t parse_devt(const char *dev_t);
 char *convert_dev(char *dev, int is_path_device);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
-int systemd_service_enabled(const char *dev);
 int get_linux_version_code(void);
 int safe_write(int fd, const void *buf, size_t count);
 void set_max_fds(rlim_t max_fds);
diff --git a/libmultipath/valid.c b/libmultipath/valid.c
index d4dae3e..f223778 100644
--- a/libmultipath/valid.c
+++ b/libmultipath/valid.c
@@ -314,23 +314,11 @@  is_path_valid(const char *name, struct config *conf, struct path *pp,
 		return PATH_IS_VALID_NO_CHECK;
 	}
 
-	/*
-	 * "multipath -u" may be run before the daemon is started. In this
-	 * case, systemd might own the socket but might delay multipathd
-	 * startup until some other unit (udev settle!)  has finished
-	 * starting. With many LUNs, the listen backlog may be exceeded, which
-	 * would cause connect() to block. This causes udev workers calling
-	 * "multipath -u" to hang, and thus creates a deadlock, until "udev
-	 * settle" times out.  To avoid this, call connect() in non-blocking
-	 * mode here, and take EAGAIN as indication for a filled-up systemd
-	 * backlog.
-	 */
-
 	if (check_multipathd) {
 		fd = __mpath_connect(1);
 		if (fd < 0) {
-			if (errno != EAGAIN && !systemd_service_enabled(name)) {
-				condlog(3, "multipathd not running or enabled");
+			if (errno != EAGAIN) {
+				condlog(3, "multipathd not running");
 				return PATH_IS_NOT_VALID;
 			}
 		} else
diff --git a/tests/valid.c b/tests/valid.c
index 7032932..18a5a7b 100644
--- a/tests/valid.c
+++ b/tests/valid.c
@@ -62,11 +62,6 @@  int __wrap___mpath_connect(int nonblocking)
 	return -1;
 }
 
-int __wrap_systemd_service_enabled(const char *dev)
-{
-	return (int)mock_type(bool);
-}
-
 /* There's no point in checking the return value here */
 int __wrap_mpath_disconnect(int fd)
 {
@@ -216,7 +211,6 @@  enum {
 enum {
 	CHECK_MPATHD_RUNNING,
 	CHECK_MPATHD_EAGAIN,
-	CHECK_MPATHD_ENABLED,
 	CHECK_MPATHD_SKIP,
 };
 
@@ -232,11 +226,8 @@  static void setup_passing(char *name, char *wwid, unsigned int check_multipathd,
 	else if (check_multipathd == CHECK_MPATHD_EAGAIN) {
 		will_return(__wrap___mpath_connect, false);
 		will_return(__wrap___mpath_connect, EAGAIN);
-	} else if (check_multipathd == CHECK_MPATHD_ENABLED) {
-		will_return(__wrap___mpath_connect, false);
-		will_return(__wrap___mpath_connect, ECONNREFUSED);
-		will_return(__wrap_systemd_service_enabled, true);
 	}
+
 	/* nothing for CHECK_MPATHD_SKIP */
 	if (stage == STAGE_CHECK_MULTIPATHD)
 		return;
@@ -342,19 +333,10 @@  static void test_check_multipathd(void **state)
 	will_return(__wrap_sysfs_is_multipathed, false);
 	will_return(__wrap___mpath_connect, false);
 	will_return(__wrap___mpath_connect, ECONNREFUSED);
-	will_return(__wrap_systemd_service_enabled, false);
+
 	assert_int_equal(is_path_valid(name, &conf, &pp, true),
 			 PATH_IS_NOT_VALID);
 	assert_string_equal(pp.dev, name);
-	/* test pass because service is enabled. fail getting udev */
-	memset(&pp, 0, sizeof(pp));
-	setup_passing(name, NULL, CHECK_MPATHD_ENABLED, STAGE_CHECK_MULTIPATHD);
-	will_return(__wrap_udev_device_new_from_subsystem_sysname, false);
-	will_return(__wrap_udev_device_new_from_subsystem_sysname,
-		    name);
-	assert_int_equal(is_path_valid(name, &conf, &pp, true),
-			 PATH_IS_ERROR);
-	assert_string_equal(pp.dev, name);
 	/* test pass because connect returned EAGAIN. fail getting udev */
 	setup_passing(name, NULL, CHECK_MPATHD_EAGAIN, STAGE_CHECK_MULTIPATHD);
 	will_return(__wrap_udev_device_new_from_subsystem_sysname, false);
@@ -533,7 +515,7 @@  static void test_check_uuid_present(void **state)
 
 	memset(&pp, 0, sizeof(pp));
 	conf.find_multipaths = FIND_MULTIPATHS_STRICT;
-	setup_passing(name, wwid, CHECK_MPATHD_ENABLED, STAGE_CHECK_WWIDS);
+	setup_passing(name, wwid, CHECK_MPATHD_RUNNING, STAGE_CHECK_WWIDS);
 	will_return(__wrap_dm_map_present_by_uuid, 1);
 	will_return(__wrap_dm_map_present_by_uuid, wwid);
 	assert_int_equal(is_path_valid(name, &conf, &pp, true),