[PATCH 0/4] mdadm: Fix some errors for regression tests and building

[PATCH 0/4] mdadm: Fix some errors for regression tests and building

[Xiao Ni]

The first two patches are for test cases.
The thrid is a fix for building error.
The forth fixes an output problem.

Xiao Ni (4):
  Fix regular expression failure
  mdadm/tests: Don't run mknod before losetup
  Avoid array bounds check of gcc
  Print version to stdout

 Makefile      | 2 +-
 mdadm.c       | 2 +-
 tests/06name  | 4 ++--
 tests/func.sh | 1 -
 4 files changed, 4 insertions(+), 5 deletions(-)

-- 
2.32.0 (Apple Git-132)

[PATCH 1/4 v2] mdadm/tests: Fix regular expression failure

[Xiao Ni]

The test fails because of the regular expression.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/06name | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/06name b/tests/06name
index 4d5e824d3e0e..86eaab69e3a1 100644
--- a/tests/06name
+++ b/tests/06name
@@ -3,8 +3,8 @@ set -x
 # create an array with a name
 
 mdadm -CR $md0 -l0 -n2 --metadata=1 --name="Fred" $dev0 $dev1
-mdadm -E $dev0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
-mdadm -D $md0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
+mdadm -E $dev0 | grep 'Name : Fred' > /dev/null || exit 1
+mdadm -D $md0 | grep 'Name : Fred' > /dev/null || exit 1
 mdadm -S $md0
 
 mdadm -A $md0 --name="Fred" $devlist
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 1/4 v2] mdadm/tests: Fix regular expression failure

[Mariusz Tkaczyk]

On Wed, 27 Sep 2023 10:52:16 +0800
Xiao Ni <xni@redhat.com> wrote:

> The test fails because of the regular expression.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  tests/06name | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/06name b/tests/06name
> index 4d5e824d3e0e..86eaab69e3a1 100644
> --- a/tests/06name
> +++ b/tests/06name
> @@ -3,8 +3,8 @@ set -x
>  # create an array with a name
>  
>  mdadm -CR $md0 -l0 -n2 --metadata=1 --name="Fred" $dev0 $dev1
> -mdadm -E $dev0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
> -mdadm -D $md0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
> +mdadm -E $dev0 | grep 'Name : Fred' > /dev/null || exit 1
> +mdadm -D $md0 | grep 'Name : Fred' > /dev/null || exit 1
>  mdadm -S $md0
>  
>  mdadm -A $md0 --name="Fred" $devlist

Hello Xiao,
I can see that it is not sent first time. Previous version was moved by me to
the "awaiting_upstream" state on patchwork but I forgot to answer.
You don't need to send it again. I'm ignoring this one. Anyway, here my
approval:

Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

https://patchwork.kernel.org/project/linux-raid/patch/20230907085744.18967-1-xni@redhat.com/

Thanks,
Mariusz

Re: [PATCH 1/4 v2] mdadm/tests: Fix regular expression failure

[Xiao Ni]

On Thu, Sep 28, 2023 at 5:25 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Wed, 27 Sep 2023 10:52:16 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > The test fails because of the regular expression.
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  tests/06name | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/06name b/tests/06name
> > index 4d5e824d3e0e..86eaab69e3a1 100644
> > --- a/tests/06name
> > +++ b/tests/06name
> > @@ -3,8 +3,8 @@ set -x
> >  # create an array with a name
> >
> >  mdadm -CR $md0 -l0 -n2 --metadata=1 --name="Fred" $dev0 $dev1
> > -mdadm -E $dev0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
> > -mdadm -D $md0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
> > +mdadm -E $dev0 | grep 'Name : Fred' > /dev/null || exit 1
> > +mdadm -D $md0 | grep 'Name : Fred' > /dev/null || exit 1
> >  mdadm -S $md0
> >
> >  mdadm -A $md0 --name="Fred" $devlist
>
> Hello Xiao,
> I can see that it is not sent first time. Previous version was moved by me to
> the "awaiting_upstream" state on patchwork but I forgot to answer.
> You don't need to send it again. I'm ignoring this one. Anyway, here my
> approval:
>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
>
> https://patchwork.kernel.org/project/linux-raid/patch/20230907085744.18967-1-xni@redhat.com/

Thanks for the information :)

Regards
Xiao
>
> Thanks,
> Mariusz
>

Re: [PATCH 1/4 v2] mdadm/tests: Fix regular expression failure

[Jes Sorensen]

On 10/7/23 09:35, Xiao Ni wrote:
> On Thu, Sep 28, 2023 at 5:25 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
>>
>> On Wed, 27 Sep 2023 10:52:16 +0800
>> Xiao Ni <xni@redhat.com> wrote:
>>
>>> The test fails because of the regular expression.
>>>
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>> ---
>>>  tests/06name | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/06name b/tests/06name
>>> index 4d5e824d3e0e..86eaab69e3a1 100644
>>> --- a/tests/06name
>>> +++ b/tests/06name
>>> @@ -3,8 +3,8 @@ set -x
>>>  # create an array with a name
>>>
>>>  mdadm -CR $md0 -l0 -n2 --metadata=1 --name="Fred" $dev0 $dev1
>>> -mdadm -E $dev0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
>>> -mdadm -D $md0 | grep 'Name : [^:]*:Fred ' > /dev/null || exit 1
>>> +mdadm -E $dev0 | grep 'Name : Fred' > /dev/null || exit 1
>>> +mdadm -D $md0 | grep 'Name : Fred' > /dev/null || exit 1
>>>  mdadm -S $md0
>>>
>>>  mdadm -A $md0 --name="Fred" $devlist
>>
>> Hello Xiao,
>> I can see that it is not sent first time. Previous version was moved by me to
>> the "awaiting_upstream" state on patchwork but I forgot to answer.
>> You don't need to send it again. I'm ignoring this one. Anyway, here my
>> approval:
>>
>> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
>>
>> https://patchwork.kernel.org/project/linux-raid/patch/20230907085744.18967-1-xni@redhat.com/
> 
> Thanks for the information :)
>

Applied!

Thanks,
Jes

[PATCH 2/4 v2] mdadm/tests: Don't run mknod before losetup

[Xiao Ni]

Sometimes it can fail:
losetup: /var/tmp/mdtest0: failed to set up loop device: No such device or address
/dev/loop0 and /var/tmp/mdtest0 are already created before losetup.

Because losetup can create device node by itself. So remove mknod.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 tests/func.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/func.sh b/tests/func.sh
index 9710a53b8a73..5053b0121f1d 100644
--- a/tests/func.sh
+++ b/tests/func.sh
@@ -170,7 +170,6 @@ do_setup() {
 				dd if=/dev/zero of=$targetdir/mdtest$d count=$sz bs=1K > /dev/null 2>&1
 			# make sure udev doesn't touch
 			mdadm --zero $targetdir/mdtest$d 2> /dev/null
-			[ -b /dev/loop$d ] || mknod /dev/loop$d b 7 $d
 			if [ $d -eq 7 ]
 			then
 				losetup /dev/loop$d $targetdir/mdtest6 # for multipath use
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 2/4 v2] mdadm/tests: Don't run mknod before losetup

[Mariusz Tkaczyk]

On Wed, 27 Sep 2023 10:52:17 +0800
Xiao Ni <xni@redhat.com> wrote:

> Sometimes it can fail:
> losetup: /var/tmp/mdtest0: failed to set up loop device: No such device or
> address /dev/loop0 and /var/tmp/mdtest0 are already created before losetup.
> 
> Because losetup can create device node by itself. So remove mknod.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  tests/func.sh | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tests/func.sh b/tests/func.sh
> index 9710a53b8a73..5053b0121f1d 100644
> --- a/tests/func.sh
> +++ b/tests/func.sh
> @@ -170,7 +170,6 @@ do_setup() {
>  				dd if=/dev/zero of=$targetdir/mdtest$d
> count=$sz bs=1K > /dev/null 2>&1 # make sure udev doesn't touch
>  			mdadm --zero $targetdir/mdtest$d 2> /dev/null
> -			[ -b /dev/loop$d ] || mknod /dev/loop$d b 7 $d
>  			if [ $d -eq 7 ]
>  			then
>  				losetup /dev/loop$d $targetdir/mdtest6 # for
> multipath use

Hello,
Same as in previous case, it is waiting for Jes:
https://patchwork.kernel.org/project/linux-raid/patch/20230908084435.30674-1-xni@redhat.com/

I'm ignoring this one.
Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Thanks,
Mariusz

Re: [PATCH 2/4 v2] mdadm/tests: Don't run mknod before losetup

[Jes Sorensen]

On 9/28/23 05:27, Mariusz Tkaczyk wrote:
> On Wed, 27 Sep 2023 10:52:17 +0800
> Xiao Ni <xni@redhat.com> wrote:
> 
>> Sometimes it can fail:
>> losetup: /var/tmp/mdtest0: failed to set up loop device: No such device or
>> address /dev/loop0 and /var/tmp/mdtest0 are already created before losetup.
>>
>> Because losetup can create device node by itself. So remove mknod.
>>
>> Signed-off-by: Xiao Ni <xni@redhat.com>
>> ---

Applied!

Thanks,
Jes

[PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Xiao Ni]

With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports error:
super-ddf.c:1988:58: error: array subscript -1 is below array bounds of
‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]
The subscrit is defined as int type. And it can be smaller than 0.
To avoid this error, add -Wno-array-bounds flag in Makefile.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 5eac1a4e9690..47da883a9fb9 100644
--- a/Makefile
+++ b/Makefile
@@ -50,7 +50,7 @@ ifeq ($(origin CC),default)
 CC := $(CROSS_COMPILE)gcc
 endif
 CXFLAGS ?= -ggdb
-CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter
+CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter -Wno-array-bounds
 ifdef WARN_UNUSED
 CWFLAGS += -Wp,-D_FORTIFY_SOURCE=2 -O3
 endif
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Paul Menzel]

Dear Xiao,


Thank you for your patch.

Am 27.09.23 um 04:52 schrieb Xiao Ni:
> With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports error:
> super-ddf.c:1988:58: error: array subscript -1 is below array bounds of
> ‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]

I wouldn’t wrap pasted lines.

> The subscrit is defined as int type. And it can be smaller than 0.

subscript?

> To avoid this error, add -Wno-array-bounds flag in Makefile.

Wouldn’t it be better to fix the error and not work around it by 
disabling the warning?


Kind regards,

Paul


> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>   Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 5eac1a4e9690..47da883a9fb9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -50,7 +50,7 @@ ifeq ($(origin CC),default)
>   CC := $(CROSS_COMPILE)gcc
>   endif
>   CXFLAGS ?= -ggdb
> -CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter
> +CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter -Wno-array-bounds
>   ifdef WARN_UNUSED
>   CWFLAGS += -Wp,-D_FORTIFY_SOURCE=2 -O3
>   endif

Re: [PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Xiao Ni]

On Wed, Sep 27, 2023 at 12:36 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
>
> Dear Xiao,
>
>
> Thank you for your patch.
>
> Am 27.09.23 um 04:52 schrieb Xiao Ni:
> > With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports error:
> > super-ddf.c:1988:58: error: array subscript -1 is below array bounds of
> > ‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]
>
> I wouldn’t wrap pasted lines.

Hi Paul

I thought it exceeds the max character limit which is used for kernel
patch rule. I'll change it back.

>
> > The subscrit is defined as int type. And it can be smaller than 0.
>
> subscript?

yes
>
> > To avoid this error, add -Wno-array-bounds flag in Makefile.
>
> Wouldn’t it be better to fix the error and not work around it by
> disabling the warning?

I want to do this first. But the logic bellow indeed checks if the
subscript is lower than 0.
It's the reason I add -Wno-array-bounds.

                info->data_offset = be64_to_cpu(ddf->phys->
                                                  entries[info->disk.raid_disk].
                                                  config_size);
                info->component_size = ddf->dlist->size - info->data_offset;
                if (info->disk.raid_disk >= 0)
                        pde = ddf->phys->entries + info->disk.raid_disk;

Do you have any suggestions for this?

Best Regards
Xiao

>
>
> Kind regards,
>
> Paul
>
>
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >   Makefile | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 5eac1a4e9690..47da883a9fb9 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -50,7 +50,7 @@ ifeq ($(origin CC),default)
> >   CC := $(CROSS_COMPILE)gcc
> >   endif
> >   CXFLAGS ?= -ggdb
> > -CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter
> > +CWFLAGS = -Wall -Werror -Wstrict-prototypes -Wextra -Wno-unused-parameter -Wno-array-bounds
> >   ifdef WARN_UNUSED
> >   CWFLAGS += -Wp,-D_FORTIFY_SOURCE=2 -O3
> >   endif
>

Re: [PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Mariusz Tkaczyk]

On Wed, 27 Sep 2023 10:52:18 +0800
Xiao Ni <xni@redhat.com> wrote:

> With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports error:
> super-ddf.c:1988:58: error: array subscript -1 is below array bounds of
> ‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]
> The subscrit is defined as int type. And it can be smaller than 0.

If it can be smaller that 0 then it is something we need to fix.
I think that it comes from here:
		info->disk.raid_disk = find_phys(ddf, ddf->dlist->disk.refnum);
		info->data_offset = be64_to_cpu(ddf->phys->
						  entries[info->disk.raid_disk].
						  config_size);

find_phys can return -1.
It is handled few lines bellow. I don't see reason why we cannot handle it here
too.

		if (info->disk.raid_disk >= 0)
			pde = ddf->phys->entries + info->disk.raid_disk;

I think that it will be fair to abort because metadata seems to be corrupted.
We are referring to info->disk.raid_disk from many places. We cannot return
error because it is void, we can just return.

> To avoid this error, add -Wno-array-bounds flag in Makefile.

If you want do it this way please provide strong justification. We are
disabling check in all code to hide particular case. It will not prevent us
from similar mistakes during development in the future.

Thanks,
Mariusz

Re: [PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Xiao Ni]

On Thu, Sep 28, 2023 at 5:42 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Wed, 27 Sep 2023 10:52:18 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports error:
> > super-ddf.c:1988:58: error: array subscript -1 is below array bounds of
> > ‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]
> > The subscrit is defined as int type. And it can be smaller than 0.
>
> If it can be smaller that 0 then it is something we need to fix.
> I think that it comes from here:
>                 info->disk.raid_disk = find_phys(ddf, ddf->dlist->disk.refnum);
>                 info->data_offset = be64_to_cpu(ddf->phys->
>                                                   entries[info->disk.raid_disk].
>                                                   config_size);
>
> find_phys can return -1.
> It is handled few lines bellow. I don't see reason why we cannot handle it here
> too.
>
>                 if (info->disk.raid_disk >= 0)
>                         pde = ddf->phys->entries + info->disk.raid_disk;
>
> I think that it will be fair to abort because metadata seems to be corrupted.
> We are referring to info->disk.raid_disk from many places. We cannot return
> error because it is void, we can just return.

Hi Mariusz

You mean something like this?

diff --git a/super-ddf.c b/super-ddf.c
index 7213284e0a59..b6e514042055 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -1984,6 +1984,9 @@ static void getinfo_super_ddf(struct supertype
*st, struct mdinfo *info, char *m
                info->disk.number = be32_to_cpu(ddf->dlist->disk.refnum);
                info->disk.raid_disk = find_phys(ddf, ddf->dlist->disk.refnum);

+               if (info->disk.raid_disk < 0)
+                       return;
+
                info->data_offset = be64_to_cpu(ddf->phys->
                                                  entries[info->disk.raid_disk].
                                                  config_size);

>
> > To avoid this error, add -Wno-array-bounds flag in Makefile.
>
> If you want do it this way please provide strong justification. We are
> disabling check in all code to hide particular case. It will not prevent us
> from similar mistakes during development in the future.

As Paul and you suggested, I'll not choose this way

Regards
Xiao
>
> Thanks,
> Mariusz
>

Re: [PATCH 3/4] mdadm: Avoid array bounds check of gcc

[Mariusz Tkaczyk]

On Sat, 7 Oct 2023 21:26:22 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Thu, Sep 28, 2023 at 5:42 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Wed, 27 Sep 2023 10:52:18 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > With gcc version 13.2.1 20230918 (Red Hat 13.2.1-3) (GCC), it reports
> > > error: super-ddf.c:1988:58: error: array subscript -1 is below array
> > > bounds of ‘struct phys_disk_entry[0]’ [-Werror=array-bounds=]
> > > The subscrit is defined as int type. And it can be smaller than 0.  
> >
> > If it can be smaller that 0 then it is something we need to fix.
> > I think that it comes from here:
> >                 info->disk.raid_disk = find_phys(ddf,
> > ddf->dlist->disk.refnum); info->data_offset = be64_to_cpu(ddf->phys->
> >                                                   entries[info->disk.raid_disk].
> >                                                   config_size);
> >
> > find_phys can return -1.
> > It is handled few lines bellow. I don't see reason why we cannot handle it
> > here too.
> >
> >                 if (info->disk.raid_disk >= 0)
> >                         pde = ddf->phys->entries + info->disk.raid_disk;
> >
> > I think that it will be fair to abort because metadata seems to be
> > corrupted. We are referring to info->disk.raid_disk from many places. We
> > cannot return error because it is void, we can just return.  
> 
> Hi Mariusz
> 
> You mean something like this?
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index 7213284e0a59..b6e514042055 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -1984,6 +1984,9 @@ static void getinfo_super_ddf(struct supertype
> *st, struct mdinfo *info, char *m
>                 info->disk.number = be32_to_cpu(ddf->dlist->disk.refnum);
>                 info->disk.raid_disk = find_phys(ddf,
> ddf->dlist->disk.refnum);
> 
> +               if (info->disk.raid_disk < 0)
> +                       return;
> +
>                 info->data_offset = be64_to_cpu(ddf->phys->
>                                                   entries[info->disk.raid_disk].
>                                                   config_size);
> 

Yes, LGTM!

Mariusz

[PATCH 4/4] mdadm: Print version to stdout

[Xiao Ni]

The version information is not error information. Print it
to stdout.

Signed-off-by: Xiao Ni <xni@redhat.com>
---
 mdadm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mdadm.c b/mdadm.c
index 076b45e030b3..0b8854baf1aa 100644
--- a/mdadm.c
+++ b/mdadm.c
@@ -128,7 +128,7 @@ int main(int argc, char *argv[])
 			continue;
 
 		case 'V':
-			fputs(Version, stderr);
+			fputs(Version, stdout);
 			exit(0);
 
 		case 'v': c.verbose++;
-- 
2.32.0 (Apple Git-132)

Re: [PATCH 4/4] mdadm: Print version to stdout

[Mariusz Tkaczyk]

On Wed, 27 Sep 2023 10:52:19 +0800
Xiao Ni <xni@redhat.com> wrote:

> The version information is not error information. Print it
> to stdout.
> 
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  mdadm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mdadm.c b/mdadm.c
> index 076b45e030b3..0b8854baf1aa 100644
> --- a/mdadm.c
> +++ b/mdadm.c
> @@ -128,7 +128,7 @@ int main(int argc, char *argv[])
>  			continue;
>  
>  		case 'V':
> -			fputs(Version, stderr);
> +			fputs(Version, stdout);
>  			exit(0);
>  
>  		case 'v': c.verbose++;

I agree with this change but...
This one is risky for users. I can realize that some users may check that
from stderr because it is how we implemented it many years ago.

I remember that I removed calls to mdam --help from dracut in the past:
https://github.com/mtkaczyk/dracut/commit/d3d37003dcecdf01f6ae0f4764d74cd035aade73#diff-f2466410e3aff8aeba95038d29b1652581c97d8d7d9feb4011d7b8bc103de1b0L64

And I can see that it does redirection "2>&1". I think that in general this kind
of problem is handled this way, so overall I ready to take the risk of changing
it to stdout by default.

Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>

Thanks,
Mariusz

Re: [PATCH 4/4] mdadm: Print version to stdout

[Xiao Ni]

On Thu, Sep 28, 2023 at 5:53 PM Mariusz Tkaczyk
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> On Wed, 27 Sep 2023 10:52:19 +0800
> Xiao Ni <xni@redhat.com> wrote:
>
> > The version information is not error information. Print it
> > to stdout.
> >
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > ---
> >  mdadm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mdadm.c b/mdadm.c
> > index 076b45e030b3..0b8854baf1aa 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -128,7 +128,7 @@ int main(int argc, char *argv[])
> >                       continue;
> >
> >               case 'V':
> > -                     fputs(Version, stderr);
> > +                     fputs(Version, stdout);
> >                       exit(0);
> >
> >               case 'v': c.verbose++;
>
> I agree with this change but...
> This one is risky for users. I can realize that some users may check that
> from stderr because it is how we implemented it many years ago.
>
> I remember that I removed calls to mdam --help from dracut in the past:
> https://github.com/mtkaczyk/dracut/commit/d3d37003dcecdf01f6ae0f4764d74cd035aade73#diff-f2466410e3aff8aeba95038d29b1652581c97d8d7d9feb4011d7b8bc103de1b0L64
>
> And I can see that it does redirection "2>&1". I think that in general this kind
> of problem is handled this way, so overall I ready to take the risk of changing
> it to stdout by default.
>
> Reviewed-by: Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>
>
> Thanks,
> Mariusz
>

Hi Mariusz

Sorry for being late to respond. I just came back from holiday. Thanks
for the suggestion. It's right that it's risky to do so. I'll drop it
in the next version.

Regards
Xiao