On Tue, Oct 24, 2023 at 06:49:37PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> 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 <mwilck@xxxxxxxx>
I'd be lying if I said I had no worries at all about this. Removing this
check means that if someone isn't using socket activation, and
multipathd is temporarily down, and a new device appears, it will always
be marked as not claimed by multipath. This could cause the device to be
grabbed by LVM, and not multipathed. This is probably just may paranoia
talking, since the chance of this happening in the real world seems
pretty low. So,
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> 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(-)
>
> 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),
> --
> 2.42.0
