[PATCH] mdadm: fix msg when removing a device using the short arg -r

[PATCH] mdadm: fix msg when removing a device using the short arg -r

[Nigel Croxon]

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@huawei.com>
Date:   Mon Aug 16 15:24:51 2021 +0800
mdadm: fix coredump of mdadm --monitor -r

-Nigel

Signed-off-by: Nigel Croxon <ncroxon@redhat.com>

Re: [PATCH] mdadm: fix msg when removing a device using the short arg -r

[Mariusz Tkaczyk]

On Mon,  7 Feb 2022 17:15:19 -0500
Nigel Croxon <ncroxon@redhat.com> 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@huawei.com>
> Date:   Mon Aug 16 15:24:51 2021 +0800
> mdadm: fix coredump of mdadm --monitor -r
> 
> -Nigel
> 
> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
> 
> 
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.

Thanks,
Mariusz

Re: [PATCH] mdadm: fix msg when removing a device using the short arg -r

[Jes Sorensen]

On 2/8/22 04:14, Mariusz Tkaczyk wrote:
> On Mon,  7 Feb 2022 17:15:19 -0500
> Nigel Croxon <ncroxon@redhat.com> 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@huawei.com>
>> Date:   Mon Aug 16 15:24:51 2021 +0800
>> mdadm: fix coredump of mdadm --monitor -r
>>
>> -Nigel
>>
>> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
>>
>>
> 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

Re: [PATCH] mdadm: fix msg when removing a device using the short arg -r

[Mariusz Tkaczyk]

On Tue, 8 Feb 2022 10:14:50 +0100
Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:

> On Mon,  7 Feb 2022 17:15:19 -0500
> Nigel Croxon <ncroxon@redhat.com> 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@huawei.com>
> > Date:   Mon Aug 16 15:24:51 2021 +0800
> > mdadm: fix coredump of mdadm --monitor -r
> > 
> > -Nigel
> > 
> > Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
> > 
> > 
> 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.
> 

Hi Coly,
Could you take a look?

Thanks,
Mariusz

Re: [PATCH] mdadm: fix msg when removing a device using the short arg -r

[Coly Li]

On 3/22/22 4:07 PM, Mariusz Tkaczyk wrote:
> On Tue, 8 Feb 2022 10:14:50 +0100
> Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com> wrote:
>
>> On Mon,  7 Feb 2022 17:15:19 -0500
>> Nigel Croxon <ncroxon@redhat.com> 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@huawei.com>
>>> Date:   Mon Aug 16 15:24:51 2021 +0800
>>> mdadm: fix coredump of mdadm --monitor -r
>>>
>>> -Nigel
>>>
>>> Signed-off-by: Nigel Croxon <ncroxon@redhat.com>
>>>
>>>
>> 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.
>>
> Hi Coly,
> Could you take a look?


Copied.


Coly Li