On 2/8/22 04:14, Mariusz Tkaczyk wrote: > On Mon, 7 Feb 2022 17:15:19 -0500 > Nigel Croxon <ncroxon@xxxxxxxxxx> wrote: > >> The change from commit mdadm: fix coredump of mdadm >> --monitor -r broke the printing of the return message when >> passing -r to mdadm --manage, the removal of a device from >> an array. >> >> If the current code reverts this commit, both issues are >> still fixed. >> >> The original problem reported that the fix tried to address >> was: The --monitor -r option requires a parameter, >> otherwise a null pointer will be manipulated when >> converting to integer data, and a core dump will appear. >> >> The original problem was really fixed with: >> 60815698c0a Refactor parse_num and use it to parse optarg. >> Which added a check for NULL in 'optarg' before moving it >> to the 'increments' variable. >> >> New issue: When trying to remove a device using the short >> argument -r, instead of the long argument --remove, the >> output is empty. The problem started when commit >> 546047688e1c was added. >> >> Steps to Reproduce: >> 1. create/assemble /dev/md0 device >> 2. mdadm --manage /dev/md0 -r /dev/vdxx >> >> Actual results: >> Nothing, empty output, nothing happens, the device is still >> connected to the array. >> >> The output should have stated "mdadm: hot remove failed >> for /dev/vdxx: Device or resource busy", if the device was >> still active. Or it should remove the device and print >> a message: >> >> # mdadm --set-faulty /dev/md0 /dev/vdd >> mdadm: set /dev/vdd faulty in /dev/md0 >> # mdadm --manage /dev/md0 -r /dev/vdd >> mdadm: hot removed /dev/vdd from /dev/md0 >> >> >> The following commit should be reverted as it breaks >> mdadm --manage -r. >> >> commit 546047688e1c64638f462147c755b58119cabdc8 >> Author: Wu Guanghao <wuguanghao3@xxxxxxxxxx> >> Date: Mon Aug 16 15:24:51 2021 +0800 >> mdadm: fix coredump of mdadm --monitor -r >> >> -Nigel >> >> Signed-off-by: Nigel Croxon <ncroxon@xxxxxxxxxx> >> >> > Hi, > Thanks for the reminder. Revert is obviously correct. Jes could you > merge it? > > In systemd world mdmonitor is rarely started from cmdline. If > "increment" option is not settable via config then it is probably > unused. I consider this option as deprecated and I suspect that no one > is using it now. Can we remove it completely? > > If you disagree with that, then we should add support for it in config > file to make it usable. Additionally, I suggest to change "-r" for > "increments" to short opt always used with parameter. Nigel, Send me a patch and I'll apply it. Reminder, keep the author of the patch you want to revert in CC. Thanks, Jes
