opensvc / multipath-tools

Other
59 stars 47 forks source link

multipath-tools 0.9.9 #85

Closed mwilck closed 3 months ago

mwilck commented 4 months ago

Important note

It is not recommended to use lvm2 2.03.24 and newer with multipath-tools versions earlier than 0.9.9. See "Other major changes" below.

User-Visible Changes

Other major changes

Bug fixes

Other

Shortlog

@bmarzins (17): multipathd: use condlog level for setscheduler error message multipathd: make multipathd scheduling configurable multipathd: make multipathd set priority to RLIMIT_RTPRIO multipathd: Set CPUWeight to 1000 and LimitRTPRIO to 10 libmultipath: export partmap_in_use libmultipath: change flush_on_last_del to fix a multipathd hang libmultipath: remove redundant config option from InfiniBox config libmultipath: pad dev_loss_tmo to avoid race with no_path_retry libmultipath: fix deferred_remove function arguments libmultipath: remove redundant config option from InfiniBox config libmultipath: pad dev_loss_tmo to avoid race with no_path_retry libmultipath: fix deferred_remove function arguments libmultipath: accept poorly chosen aliases in find_mp_by_str libmultipath: accept wwids in find_mp_by_str multipath-tools man pages: don't assume multipath.socket is enabled libmultipath: print all values in snprint_failback multipathd: Stop double counting map failures for no_path_retry > 0 multipath-tools man pages: add missing multipathd commands libmultipath: change the vend/prod/rev printing multipath-tools man pages: Add format wildcard descriptions

@mwilck (33): 11-dm-mpath.rules: fix misspelled DM_UDEV_DISABLE_OTHER_RULES_FLAG libmpathutil: really always use glibc basename() 11-dm-mpath.rules: explain logic for device becoming ready while suspended 11-dm-mpath.rules: don't import DM_NOSCAN from udev db 11-dm-mpath.rules: don't import ID_FS_VERSION from udev db 11-dm-mpath.rules: adapt MPATH_DEVICE_READY=0 logic to 10-dm.rules update 11-dm-mpath.rules: adapt coldplug event handling ro 10-dm.rules update 11-dm-mpath.rules: don't import properties with new 13-dm-disk.rules 11-dm-mpath.rules: replace DM_SUSPENDED by .DM_SUSPENDED 11-dm-mpath.rules: replace DM_NOSCAN by .DM_NOSCAN 11-dm-mpath.rules: simplify PATH_FAILED case 11-dm-mpath.rules: make label names more intuitive kpartx.rules: ignore DM_SUSPENDED multipath-tools tests: fix CI failures on arm/v7 with glibc 2.37 multipath-tools tests: fix CI failures with clang on Fedora Rawhide GitHub actions: fixes for spelling CI GitHub workflows: run workflows if workflow file has changed multipath-tools: add TGTDIR option libmultipath: move get_udev_for_mpp to sysfs.c libmultipath: add mp_find_path_by_devt() Revert "libmultipath: fix max_sectors_kb on adding path" libmultipath: Only set max_sectors_kb on map creation libmultipath: set max_sectors_kb in adopt_paths() libmultipath: add wildcard %k for printing max_sectors_kb multipath.conf(5): update documentation for max_sectors_kb libmultipath: use bitwise flags for map flushing API libmultipath: use bitwise flags for dm_simplecmd API libmultipath: add argument names to some prototypes libmultipath: bump version to 0.9.9 GitHub Workflows: native.yaml: run for Fedora 40 update NEWS.md multipath.conf.5.in: fix man page date More updates to NEWS.md

@NitinYewale (1): libmultipath: remove pathgroup wildcard options

@xosevp (3): multipath-tools: simplify comment in hwtable multipath-tools: unify text in multipath.conf.5 multipath-tools: update man pages dates

As usual, all patches have a Reviewed-by: trailer, except those that just concern NEWS.md, the workflows, and the version bump, and the trivial commit 9fa3770.

Update 2024-06-04

Added some more patches from @bmarzins and @NitinYewale. Mostly man pages and wildcard treatment.

yu-re-ka commented 3 months ago

The tests do not compile:


== running parser-test ==
building util.o because of util.c
util.c: In function 'test_basename_01':
util.c:174:16: error: implicit declaration of function 'basename'; did you mean 'dm_basename'? [-Werror=implicit-function-declaration]
  174 |         base = basename(path);
      |                ^~~~~~~~
      |                dm_basename
util.c:174:14: error: assignment to 'const char *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
  174 |         base = basename(path);
      |              ^
util.c: In function 'test_basename_02':
util.c:184:14: error: assignment to 'const char *' from 'int' makes pointer from integer without a cast [-Werror=int-conversion]
  184 |         base = basename(path);
      |              ^
cc1: all warnings being treated as errors
make[1]: *** [Makefile:74: util.o] Error 1
rm parser.o.wrap uevent.o.wrap
make[1]: Leaving directory '/build/source/tests'
make: *** [Makefile:121: test] Error 2
mwilck commented 3 months ago

The tests do not compile:

They do in all environments our CI covers. What distro / libc / toolchain are you using?

mwilck commented 3 months ago

@yu-re-ka, can you please reply to my question?

yu-re-ka commented 3 months ago

NixOS / glibc 2.39 / gcc 13.2.0 full log: https://termbin.com/4qpf

github-actions[bot] commented 3 months ago

@check-spelling-bot Report

:red_circle: Please review

See the :open_file_folder: files view, the :scroll:action log, or :memo: job summary for details.

Unrecognized words (2)

gitlab lvmteam

To accept these unrecognized words as correct, you could run the following commands ... in a clone of the [git@github.com:openSUSE/multipath-tools.git](https://github.com/openSUSE/multipath-tools.git) repository on the `queue` branch ([:information_source: how do I use this?]( https://github.com/check-spelling/check-spelling/wiki/Accepting-Suggestions)): ``` sh curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/main/apply.pl' | perl - 'https://github.com/opensvc/multipath-tools/actions/runs/9370840303/attempts/1' ```
Available :books: dictionaries could cover words (expected and unrecognized) not in the :blue_book: dictionary This includes both **expected items** (241) from .github/actions/spelling/expect.txt and **unrecognized words** (2) Dictionary | Entries | Covers | Uniquely -|-|-|- [cspell:php/dict/php.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/php/dict/php.txt)|1689|13|3| [cspell:node/dict/node.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/node/dict/node.txt)|891|14|2| [cspell:python/src/python/python-lib.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/python/src/python/python-lib.txt)|2417|10|1| [cspell:golang/dict/go.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/golang/dict/go.txt)|2099|7|1| [cspell:k8s/dict/k8s.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/v20230509/dictionaries/k8s/dict/k8s.txt)|153|6|1| Consider adding them (in `.github/workflows/spelling.yml`) for `uses: check-spelling/check-spelling@main` in its `with`: ``` yml with: extra_dictionaries: cspell:php/dict/php.txt cspell:node/dict/node.txt cspell:python/src/python/python-lib.txt cspell:golang/dict/go.txt cspell:k8s/dict/k8s.txt ``` To stop checking additional dictionaries, add (in `.github/workflows/spelling.yml`) for `uses: check-spelling/check-spelling@main` in its `with`: ``` yml check_extra_dictionaries: '' ```
mwilck commented 3 months ago

@check-spelling-bot Report

has been resolved with latest push

mwilck commented 3 months ago

@yu-re-ka, forgive my ignorance, I have zero knowledge about NixOS.

Can you provide a quick recipe for me to reproduce your issue, preferably in a container?

mwilck commented 3 months ago

@yu-re-ka

It's strange because your environment uses the same base versions that I have in mine (openSUSE Tumbleweed).

The NixOS issue seems to be here:

building util.o because of util.c
util.c: In function 'test_basename_01':
util.c:174:16:error: implicit declaration of function 'basename'; did you mean 'dm_basename'? https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-function-declaration-Werror=implicit-function-declaration
  174 |         base = basename(path);

But if you look at the file tests/util.c, it has:

#define _GNU_SOURCE
#include <string.h>
#include "util.h"

and util.h includes

const char *libmp_basename(const char *filename);
#ifndef __GLIBC__
#define basename(x) libmp_basename(x)
#endif

So if you are actually compiling against glibc[^musl], you should be fine because of the string.h include, and if you are compiling against musl libc, the code from util.h should resolve basename() to libmp_basename(). This works for me for Tumbleweed and Fedora (recent glibc and gcc) and in Alpine (recent gcc and musl).

If this doesn't work, I fear I have to ask you to do some debugging on your own behalf and see what's going wrong in your environment.

[^musl]: You say you're using glibc but AFAICS NixOS is carrying MUSL related patches, so maybe it isn't using glibc after all?

mitchty commented 3 months ago

So running with these changes to the nixos-24.05 branch I get this, looks like util.h isn't getting included by my eyes.

Diff to the derivation:

diff --git a/pkgs/os-specific/linux/multipath-tools/default.nix b/pkgs/os-specific/linux/multipath-tools/default.nix
index 5ec8197451cf..39fa47418c34 100644
--- a/pkgs/os-specific/linux/multipath-tools/default.nix
+++ b/pkgs/os-specific/linux/multipath-tools/default.nix
@@ -21,20 +21,20 @@

 stdenv.mkDerivation rec {
   pname = "multipath-tools";
-  version = "0.9.6";
+  version = "0.9.9";

   src = fetchFromGitHub {
-    owner = "opensvc";
+    owner = "openSUSE";
     repo = "multipath-tools";
-    rev = "refs/tags/${version}";
-    sha256 = "sha256-X4sAMGn4oBMY3cQkVj1dMcrDF7FgMl8SbZeUnCCOY6Q=";
+    rev = "queue";
+    sha256 = "sha256-D2t1XsDa82m9JliL/i+68TZ/PKanXrtkN3w4gYrb/dM=";
   };

   postPatch = ''
     substituteInPlace create-config.mk \
       --replace /bin/echo ${coreutils}/bin/echo

-    substituteInPlace multipathd/multipathd.service \
+    substituteInPlace multipathd/multipathd.service.in \
       --replace /sbin/multipathd "$out/bin/multipathd"

     sed -i -re '
@@ -64,6 +64,7 @@ stdenv.mkDerivation rec {
   ];

   makeFlags = [
+    "V=1"
     "LIB=lib"
     "prefix=$(out)"
     "systemd_prefix=$(out)"

Built locally via nix build .#legacyPackages.x86_64-linux.multipath-tools -L

multipath-tools> == running parser-test ==
multipath-tools> building util.o because of util.c
multipath-tools> gcc -D_FORTIFY_SOURCE=3  -DURCU_VERSION=0x000e00 -D_FILE_OFFSET_BITS=64 -DBIN_DIR=\"/nix/store/dc3z48ls70h8sr9dkir71ybrvqy50rb8-multipath-tools-0.9.9/sbin\" -DMULTIPATH_DIR=\"/nix/store/dc3z48ls70h8sr9dkir71ybrvqy50rb8-multipath-tools-0.9.9/lib/multipath\" -DRUNTIME_DIR=\"/run\" -DCONFIG_DIR=\"/nix/store/dc3z48ls70h8sr9dkir71ybrvqy50rb8-multipath-tools-0.9.9/etc/multipath/conf.d\" -DDEFAULT_CONFIGFILE=\"/nix/store/dc3z48ls70h8sr9dkir71ybrvqy50rb8-multipath-tools-0.9.9/etc/multipath.conf\" -DSTATE_DIR=\"/nix/store/dc3z48ls70h8sr9dkir71ybrvqy50rb8-multipath-tools-0.9.9/etc/multipath\" -DEXTRAVERSION=\"\" -MMD -MP -I../libmultipath -I../libmpathutil -I../libmpathcmd -I../multipathd -DTESTCONFDIR=\"/build/source/tests/conf.d\" -std=gnu99  -O2 -g -fstack-protector-strong --param=ssp-buffer-size=4 -Werror -Wall -Wextra -Wformat=2 -Wformat-overflow=2 -Werror=implicit-int -Werror=implicit-function-declaration -Werror=format-security -Wno-clobbered -Wno-error=clobbered -Werror=cast-qual -Werror=discarded-qualifiers  -pipe -fPIE -DPIE -Wno-unused-parameter   -c -o util.o util.c
multipath-tools> util.c: In function 'test_basename_01':
multipath-tools> util.c:174:16: error: implicit declaration of function 'basename'; did you mean 'dm_basename'? [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-function-declaration-Werror=implicit-function-declaration8;;]
multipath-tools>   174 |         base = basename(path);
multipath-tools>       |                ^~~~~~~~
multipath-tools>       |                dm_basename
multipath-tools> util.c:174:14: error: assignment to 'const char *' from 'int' makes pointer from integer without a cast [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wint-conversion-Werror=int-conversion8;;]
multipath-tools>   174 |         base = basename(path);
multipath-tools>       |              ^
multipath-tools> util.c: In function 'test_basename_02':
multipath-tools> util.c:184:14: error: assignment to 'const char *' from 'int' makes pointer from integer without a cast [8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wint-conversion-Werror=int-conversion8;;]
multipath-tools>   184 |         base = basename(path);
multipath-tools>       |              ^
multipath-tools> cc1: all warnings being treated as errors
multipath-tools> make[1]: *** [Makefile:74: util.o] Error 1
multipath-tools> rm parser.o.wrap uevent.o.wrap
multipath-tools> make[1]: Leaving directory '/build/source/tests'
multipath-tools> make: *** [Makefile:121: test] Error 2

Not entirely sure whats going on the makefiles are a bit confusing but it looks more to me like the makefiles aren't including what is expected.

mitchty commented 3 months ago

Noting from glibc, the import we want is according to: https://sourceware.org/git/?p=glibc.git;a=blob;f=misc/libgen.h;hb=HEAD#l26

And rerunning with these changes to those test files like so (and killing the const warning as a const char seems to be being passed into basename):

diff --git a/pkgs/os-specific/linux/multipath-tools/default.nix b/pkgs/os-specific/linux/multipath-tools/default.nix
index 5ec8197451cf..50e6307b3a03 100644
--- a/pkgs/os-specific/linux/multipath-tools/default.nix
+++ b/pkgs/os-specific/linux/multipath-tools/default.nix
@@ -21,20 +21,20 @@

 stdenv.mkDerivation rec {
   pname = "multipath-tools";
-  version = "0.9.6";
+  version = "0.9.9";

   src = fetchFromGitHub {
-    owner = "opensvc";
+    owner = "openSUSE";
     repo = "multipath-tools";
-    rev = "refs/tags/${version}";
-    sha256 = "sha256-X4sAMGn4oBMY3cQkVj1dMcrDF7FgMl8SbZeUnCCOY6Q=";
+    rev = "queue";
+    sha256 = "sha256-D2t1XsDa82m9JliL/i+68TZ/PKanXrtkN3w4gYrb/dM=";
   };

   postPatch = ''
     substituteInPlace create-config.mk \
       --replace /bin/echo ${coreutils}/bin/echo

-    substituteInPlace multipathd/multipathd.service \
+    substituteInPlace multipathd/multipathd.service.in \
       --replace /sbin/multipathd "$out/bin/multipathd"

     sed -i -re '
@@ -46,6 +46,7 @@ stdenv.mkDerivation rec {
       $(find * -name Makefile\*)

     sed '1i#include <assert.h>' -i tests/{util,vpd}.c
+    sed '1i#include <libgen.h>' -i tests/{util,vpd}.c
   '';

   nativeBuildInputs = [
@@ -64,6 +65,8 @@ stdenv.mkDerivation rec {
   ];

   makeFlags = [
+    "V=1"
+    "CFLAGS=-Wno-discarded-qualifiers"
     "LIB=lib"
     "prefix=$(out)"
     "systemd_prefix=$(out)"

I get to here where util.h is definitely not getting sent to CFLAGS as an include dir:

multipath-tools> gcc  -Wl,-z,relro -Wl,-z,now -Wl,-z,defs -shared -Wl,-soname=libmultipath.so.0 \
multipath-tools>        -Wl,--version-script=libmultipath.version -o libmultipath.so.0 devmapper.o hwtable.o blacklist.o dmparser.o structs.o discovery.o propsel.o dict.o pgpolicies.o defaults.o uevent.o switchgroup.o print.o alias.o configure.o structs_vec.o sysfs.o lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o io_err_stat.o dm-generic.o generic.o nvme-lib.o libsg.o valid.o prio.o checkers.o foreign.o config.o -lpthread -ldl -ldevmapper -ludev -L../libmpathutil -lmpathutil -L../libmpathcmd -lmpathcmd -lmount -lurcu -laio -lsystemd
multipath-tools> ln -sf libmultipath.so.0 libmultipath.so
multipath-tools> make[1]: Leaving directory '/build/source/libmultipath'
multipath-tools> make[1]: Entering directory '/build/source/libmpathpersist'
multipath-tools> building mpath_persist.o because of mpath_persist.c
multipath-tools> gcc -D_FORTIFY_SOURCE=3  -DURCU_VERSION=0x000e00 -D_FILE_OFFSET_BITS=64 -DBIN_DIR=\"/nix/store/x5gck165q7r1h0dhqrfpvv1vfk9h8hw0-multipath-tools-0.9.9/sbin\" -DMULTIPATH_DIR=\"/nix/store/x5gck165q7r1h0dhqrfpvv1vfk9h8hw0-multipath-tools-0.9.9/lib/multipath\" -DRUNTIME_DIR=\"/run\" -DCONFIG_DIR=\"/nix/store/x5gck165q7r1h0dhqrfpvv1vfk9h8hw0-multipath-tools-0.9.9/etc/multipath/conf.d\" -DDEFAULT_CONFIGFILE=\"/nix/store/x5gck165q7r1h0dhqrfpvv1vfk9h8hw0-multipath-tools-0.9.9/etc/multipath.conf\" -DSTATE_DIR=\"/nix/store/x5gck165q7r1h0dhqrfpvv1vfk9h8hw0-multipath-tools-0.9.9/etc/multipath\" -DEXTRAVERSION=\"\" -MMD -MP -Wno-discarded-qualifiers -c -o mpath_persist.o mpath_persist.c
multipath-tools> mpath_persist.c:3:10: fatal error: util.h: No such file or directory
multipath-tools>     3 | #include "util.h"
multipath-tools>       |          ^~~~~~~~
multipath-tools> compilation terminated.
multipath-tools> make[1]: *** [../Makefile.inc:136: mpath_persist.o] Error 1
multipath-tools> make[1]: Leaving directory '/build/source/libmpathpersist'
multipath-tools> make: *** [Makefile:45: libmpathpersist] Error 2
mwilck commented 3 months ago

@yu-re-ka

The problem is in the nix derivation itself, not in the multipath-tools code. Because of the insertion of #include <assert.h> in line 1 in the source file, the #define _GNU_SOURCE later in the file has no effect.[^glibc]

The insertion of #include <assert.h> is obsoleted by 5e95a28 (0.9.5) and can be removed. The same holds for this downstream patch (obsoleted by e5004de, this PR).

Next time, instead of creating downstream changes or string substitution hacks, please send patches upstream that make the NixOS build succeed without such customizations. We won't reject such patches unless they break something in other distros.

[^glibc]: "These directives must come before any #include of a system header file".

mwilck commented 3 months ago

This code block in multipath-tools default.nix also seems superfluous:

    sed -i -re '
      s,^( *#define +DEFAULT_MULTIPATHDIR\>).*,\1 "'"$out/lib/multipath"'",
    ' libmultipath/defaults.h
    sed -i -e 's,\$(DESTDIR)/\(usr/\)\?,$(prefix)/,g' \
      kpartx/Makefile libmpathpersist/Makefile
    sed -i -e "s,GZIP,GZ," \
      $(find * -name Makefile\*)

The first two sed commands replace patterns which we don't use any more, and the 3rd one appears totally pointless to me.

mwilck commented 3 months ago

the 3rd one appears totally pointless to me.

Historically, it was necessary. It has been obsoleted by 02bc889.

mwilck commented 3 months ago

The first two sed commands replace patterns which we don't use any more

DEFAULT_MULTIPATHDIR has been removed in af15832 (0.9.0). $(DESTDIR)/usr hasn't been referenced since 66bea11 (0.6.2).

yu-re-ka commented 3 months ago

Thank you so much for figuring this out! I have not been involved with the multipath-tools package before except updating it to this new version / backporting the fix for the new musl basename compatibility things.

These lines are quite old (up to 8 years!). I would like to believe we are a lot more strict in how we patch around code nowadays. Anyways, it's cleaned up now.

mwilck commented 3 months ago

@yu-re-ka

while we're discussing this, what's the right way to insert absolute paths to executables e.g. in systemd unit files or udev rules in NixoS?

mwilck commented 3 months ago

@cvaroqui, as the NixOS issue is resolved, can we go forward with this PR?

yu-re-ka commented 3 months ago

while we're discussing this, what's the right way to insert absolute paths to executables e.g. in systemd unit > files or udev rules in NixoS?

this depends: is it a executable from your own application, or from another my expectation would be:

mwilck commented 3 months ago

I think we can do the former (I'm currently working on a patch series for it) but not the latter, which is just too NixOS-specific and up to the Nix developers to care about.