Re: [PATCH mdadm v1 06/14] mdadm: Fix mdadm -r remove option regresision

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Thu,  9 Jun 2022 15:11:22 -0600
Logan Gunthorpe <logang@deltatee.com> wrote:

> The commit noted below globally adds a parameter to the -r option but missed
> the fact that -r is used for another purpose: --remove.
> 
> After that commit, a command such as:
> 
>   mdadm /dev/md0 -r /dev/loop0
> 
> will do nothing seeing the device parameter will be consumed as a
> argument to the -r option; thus, there will only be one device
> seen one the command line, devs_found will only be 1 and nothing will
> happen.
> 
> This caused the 01r5integ and 01raid6integ tests to hang indefinitely
> as mdadm did not remove the failed device. With the device not removed,
> it would not be readded. Then the loop waiting for the array status to
> change would loop forever.
> 
> To fix this, revert the changes in the noted patch and create a new subopt
> type for the monitor mode with parameters required for -r.
> 
> Fixes: 546047688e1c ("mdadm: fix coredump of mdadm --monitor -r")
> Cc: Wu Guanghao <wuguanghao3@huawei.com>
> Cc: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---


>  ReadMe.c | 7 ++++---
>  mdadm.c  | 1 +
>  mdadm.h  | 1 +
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/ReadMe.c b/ReadMe.c
> index 8f873c4895da..556104f75d72 100644
> --- a/ReadMe.c
> +++ b/ReadMe.c
> @@ -81,11 +81,12 @@ char Version[] = "mdadm - v" VERSION " - " VERS_DATE
> EXTRAVERSION "\n";
>   *     found, it is started.
>   */
>  
> -char
> short_options[]="-ABCDEFGIQhVXYWZ:vqbc:i:l:p:m:r:n:x:u:c:d:z:U:N:safRSow1tye:k";
> +char
> short_options[]="-ABCDEFGIQhVXYWZ:vqbc:i:l:p:m:n:x:u:c:d:z:U:N:sarfRSow1tye:k";
> +char
> short_monitor_options[]="-ABCDEFGIQhVXYWZ:vqbc:i:l:p:m:r:n:x:u:c:d:z:U:N:safRSow1tye:k";
> char short_bitmap_options[]=
> -
> "-ABCDEFGIQhVXYWZ:vqb:c:i:l:p:m:r:n:x:u:c:d:z:U:N:sarfRSow1tye:k:";
> +
> "-ABCDEFGIQhVXYWZ:vqb:c:i:l:p:m:n:x:u:c:d:z:U:N:sarfRSow1tye:k:"; char
> short_bitmap_auto_options[]=
> -
> "-ABCDEFGIQhVXYWZ:vqb:c:i:l:p:m:r:n:x:u:c:d:z:U:N:sa:rfRSow1tye:k:";
> +
> "-ABCDEFGIQhVXYWZ:vqb:c:i:l:p:m:n:x:u:c:d:z:U:N:sa:rfRSow1tye:k:"; 
>  struct option long_options[] = {
>      {"manage",    0, 0, ManageOpt},
> diff --git a/mdadm.c b/mdadm.c
> index be40686cf91b..d0c5e6def901 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -227,6 +227,7 @@ int main(int argc, char *argv[])
>  			shortopt = short_bitmap_auto_options;
>  			break;
>  		case 'F': newmode = MONITOR;
> +			shortopt = short_monitor_options;
>  			break;
>  		case 'G': newmode = GROW;
>  			shortopt = short_bitmap_options;
> diff --git a/mdadm.h b/mdadm.h
> index 09915a0009d9..559da3f6f440 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -419,6 +419,7 @@ enum mode {
>  };
>  
>  extern char short_options[];
> +extern char short_monitor_options[];
>  extern char short_bitmap_options[];
>  extern char short_bitmap_auto_options[];
>  extern struct option long_options[];

Jes applied Nigel's revert but I cannot see it on repository yet.
I consider adding short monitor options as valuable.
Could you apply revert manually and then adopt your patch?
https://lore.kernel.org/linux-raid/5f9a4417-d044-a87e-3945-2c6b29278d8c@trained-monkey.org/#t

Thanks,
Mariusz