[PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

The test case is:
1. create one imsm container
2. create a raid5 device from the container
3. unplug two disks
4. mdadm --detail /dev/md126
[root@rhel85 ~]# mdadm -D /dev/md126
/dev/md126:
         Container : ��, member 0

The Detail function first gets container name by function
map_dev_preferred. Then it tries to find which disks are
available. In patch db5377883fef(It should be FAILED..)
uses map_dev_preferred to find which disks are under /dev.

But now, the major/minor information comes from kernel space.
map_dev_preferred malloc memory and init a device list when
first be called by Detail. It can't find the device in the
list by the major/minor. It free the memory and reinit the
list.

The container name now points to an area tha has been freed.
So the containt is a mess.

This patch replaces map_dev_preferred with devid2kname. If
the name is NULL, it means the disk is unplug.

Fixes: db5377883fef (It should be FAILED when raid has)
Signed-off-by: Xiao Ni <xni@redhat.com>
Reported-by: Fine Fan <ffan@redhat.com>
---
 Detail.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/Detail.c b/Detail.c
index d3af0ab..2164de3 100644
--- a/Detail.c
+++ b/Detail.c
@@ -351,11 +351,13 @@ int Detail(char *dev, struct context *c)
 	avail = xcalloc(array.raid_disks, 1);
 
 	for (d = 0; d < array.raid_disks; d++) {
-		char *dv, *dv_rep;
-		dv = map_dev_preferred(disks[d*2].major,
-				disks[d*2].minor, 0, c->prefer);
-		dv_rep = map_dev_preferred(disks[d*2+1].major,
-				disks[d*2+1].minor, 0, c->prefer);
+		char *dv, *dv_rep = NULL;
+
+		if (!disks[d*2].major && !disks[d*2].minor)
+			continue;
+
+		dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
+		dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
 
 		if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
 		    (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
-- 
2.7.5

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Tkaczyk, Mariusz]

Hi Xiao,
Thanks for taking care of this.

On 15.10.2021 11:25, Xiao Ni wrote:
> The test case is:
> 1. create one imsm container
> 2. create a raid5 device from the container
> 3. unplug two disks
> 4. mdadm --detail /dev/md126
> [root@rhel85 ~]# mdadm -D /dev/md126
> /dev/md126:
>           Container : ��, member 0
> 
> The Detail function first gets container name by function
> map_dev_preferred. Then it tries to find which disks are
> available. In patch db5377883fef(It should be FAILED..)
> uses map_dev_preferred to find which disks are under /dev.
> 
> But now, the major/minor information comes from kernel space.
> map_dev_preferred malloc memory and init a device list when
> first be called by Detail. It can't find the device in the
> list by the major/minor. It free the memory and reinit the
> list.
>  > The container name now points to an area tha has been freed.
> So the containt is a mess.
> 

Container name is collected with 'create' flag set, so it's
name is additionally copied to static memory to prevent
overwrites. Could you verify?


So summarizing: the previously returned value might be lost
in next call.

I looked into code, map_dev_preffered() mainly is used for
determining short-life buffers, which are used only to the next
call (next call overwrites previous result, expect case you fixed).

IMO map_dev_preffered() cannot be trusted now. I see some options:
1 - allocate additional memory and save return value there (caller
needs to free it later).
2 - describe limitations in description of the function to avoid
incorrect usages in the future.

> This patch replaces map_dev_preferred with devid2kname. If
> the name is NULL, it means the disk is unplug.
> 
Your patch fixes only one place. Please go forward and analyze all
map_dev_preffered() calls (which looks safe to me). Maybe this
function can be replaced at all and we can drop this code in
flavor of devid2kname() or other.

> Fixes: db5377883fef (It should be FAILED when raid has)
> Signed-off-by: Xiao Ni <xni@redhat.com>
> Reported-by: Fine Fan <ffan@redhat.com>
> ---
>   Detail.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/Detail.c b/Detail.c
> index d3af0ab..2164de3 100644
> --- a/Detail.c
> +++ b/Detail.c
> @@ -351,11 +351,13 @@ int Detail(char *dev, struct context *c)
>   	avail = xcalloc(array.raid_disks, 1);
>   
>   	for (d = 0; d < array.raid_disks; d++) {
> -		char *dv, *dv_rep;
> -		dv = map_dev_preferred(disks[d*2].major,
> -				disks[d*2].minor, 0, c->prefer);
> -		dv_rep = map_dev_preferred(disks[d*2+1].major,
> -				disks[d*2+1].minor, 0, c->prefer);
> +		char *dv, *dv_rep = NULL;
> +
> +		if (!disks[d*2].major && !disks[d*2].minor)
> +			continue; > +
> +		dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
> +		dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
>   
>   		if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
>   		    (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
> 

Yeah, I know that it is used in Detail this way, but please  determine
way to replace this ugly [d*2] and [d*2+1].

This whole block should be moved from Detail() code to separate
function, which determines if device or replacement is in sync.

Thanks,
Mariusz

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Tkaczyk, Mariusz]

>> +        dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
>> +        dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));

devid2kname() returns static memory. If both drive and replacement
are available then dv value will be overwritten. Not sure if it is
possible.

Mariusz

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

On Mon, Oct 18, 2021 at 2:56 PM Tkaczyk, Mariusz
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Xiao,
> Thanks for taking care of this.
>
> On 15.10.2021 11:25, Xiao Ni wrote:
> > The test case is:
> > 1. create one imsm container
> > 2. create a raid5 device from the container
> > 3. unplug two disks
> > 4. mdadm --detail /dev/md126
> > [root@rhel85 ~]# mdadm -D /dev/md126
> > /dev/md126:
> >           Container : ��, member 0
> >
> > The Detail function first gets container name by function
> > map_dev_preferred. Then it tries to find which disks are
> > available. In patch db5377883fef(It should be FAILED..)
> > uses map_dev_preferred to find which disks are under /dev.
> >
> > But now, the major/minor information comes from kernel space.
> > map_dev_preferred malloc memory and init a device list when
> > first be called by Detail. It can't find the device in the
> > list by the major/minor. It free the memory and reinit the
> > list.
> >  > The container name now points to an area tha has been freed.
> > So the containt is a mess.
> >
>
> Container name is collected with 'create' flag set, so it's
> name is additionally copied to static memory to prevent
> overwrites. Could you verify?

Hi Mariusz

The chapter above you mentioned is talking about the creation process?
The container name mentioned from the patch is a temporary variable in
Detail function.

You want to say we can use the container name from the "static memory" in
Detail function, so we don't get the container name again? And where is the
static memory?

>
> So summarizing: the previously returned value might be lost
> in next call.

If you say the value returned from map_dev_preffered, you are right,
the buffer might be free and re-alloc. So the value might be lost
in the next call.

>
> I looked into code, map_dev_preffered() mainly is used for
> determining short-life buffers, which are used only to the next
> call (next call overwrites previous result, expect case you fixed).

right

>
> IMO map_dev_preffered() cannot be trusted now. I see some options:
> 1 - allocate additional memory and save return value there (caller
> needs to free it later).
> 2 - describe limitations in description of the function to avoid
> incorrect usages in the future.

I'll add one description first.

>
> > This patch replaces map_dev_preferred with devid2kname. If
> > the name is NULL, it means the disk is unplug.
> >
> Your patch fixes only one place. Please go forward and analyze all
> map_dev_preffered() calls (which looks safe to me). Maybe this
> function can be replaced at all and we can drop this code in
> flavor of devid2kname() or other.

At first, I just wanted to fix this bug. We can check all the places which
are using map_dev_preffered. But it looks like a big project. It needs to
understand all codes related to this function. It needs more time. Do you
agree with fixing this bug first, then we can try to fix the hidden bugs.

>
> > Fixes: db5377883fef (It should be FAILED when raid has)
> > Signed-off-by: Xiao Ni <xni@redhat.com>
> > Reported-by: Fine Fan <ffan@redhat.com>
> > ---
> >   Detail.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/Detail.c b/Detail.c
> > index d3af0ab..2164de3 100644
> > --- a/Detail.c
> > +++ b/Detail.c
> > @@ -351,11 +351,13 @@ int Detail(char *dev, struct context *c)
> >       avail = xcalloc(array.raid_disks, 1);
> >
> >       for (d = 0; d < array.raid_disks; d++) {
> > -             char *dv, *dv_rep;
> > -             dv = map_dev_preferred(disks[d*2].major,
> > -                             disks[d*2].minor, 0, c->prefer);
> > -             dv_rep = map_dev_preferred(disks[d*2+1].major,
> > -                             disks[d*2+1].minor, 0, c->prefer);
> > +             char *dv, *dv_rep = NULL;
> > +
> > +             if (!disks[d*2].major && !disks[d*2].minor)
> > +                     continue; > +
> > +             dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
> > +             dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
> >
> >               if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
> >                   (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
> >
>
> Yeah, I know that it is used in Detail this way, but please  determine
> way to replace this ugly [d*2] and [d*2+1].

Yes, it takes me much time to understand how to calculate avail[]. We
can focus on
fixing this bug first, then we prepare some patches for improving the codes that
related to the function map_dev_preferred and the [d*2] format codes.
It might be
a big change.

>
> This whole block should be moved from Detail() code to separate
> function, which determines if device or replacement is in sync.

A good suggestion. Put it into the change I mentioned above, is it ok?

Regards
Xiao

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Tkaczyk, Mariusz]

On 18.10.2021 11:58, Xiao Ni wrote:
>>> The test case is:
>>> 1. create one imsm container
>>> 2. create a raid5 device from the container
>>> 3. unplug two disks
>>> 4. mdadm --detail /dev/md126
>>> [root@rhel85 ~]# mdadm -D /dev/md126
>>> /dev/md126:
>>>            Container : ��, member 0
>>>
>>> The Detail function first gets container name by function
>>> map_dev_preferred. Then it tries to find which disks are
>>> available. In patch db5377883fef(It should be FAILED..)
>>> uses map_dev_preferred to find which disks are under /dev.
>>>
>>> But now, the major/minor information comes from kernel space.
>>> map_dev_preferred malloc memory and init a device list when
>>> first be called by Detail. It can't find the device in the
>>> list by the major/minor. It free the memory and reinit the
>>> list.
>>>   > The container name now points to an area tha has been freed.
>>> So the containt is a mess.
>>>
>>
>> Container name is collected with 'create' flag set, so it's
>> name is additionally copied to static memory to prevent
>> overwrites. Could you verify?
> 
> Hi Mariusz
> 
> The chapter above you mentioned is talking about the creation process?
> The container name mentioned from the patch is a temporary variable in
> Detail function.
> 
> You want to say we can use the container name from the "static memory" in
> Detail function, so we don't get the container name again? And where is the
> static memory?
> 

There is a code:
	if (create && !regular && !preferred) {
		static char buf[30]; <- this variable will survive retry.
		snprintf(buf, sizeof(buf), "%d:%d", major, minor);
		regular = buf;
	}
but seems that it is not a case for this scenario. I suspected that
this was used because when gathering container name:

		container = map_dev_preferred(major(devid), minor(devid),
					      1, c->prefer);

'create' is explicitly set to 1. That is why I expect to have 'container'
declared in static area. Make sense?


>>> This patch replaces map_dev_preferred with devid2kname. If
>>> the name is NULL, it means the disk is unplug.
>>>
>> Your patch fixes only one place. Please go forward and analyze all
>> map_dev_preffered() calls (which looks safe to me). Maybe this
>> function can be replaced at all and we can drop this code in
>> flavor of devid2kname() or other.
> 
> At first, I just wanted to fix this bug. We can check all the places which
> are using map_dev_preffered. But it looks like a big project. It needs to
> understand all codes related to this function. It needs more time. Do you
> agree with fixing this bug first, then we can try to fix the hidden bugs.
> 
Sure, we are in rc2, so bigger changes should be merged after release.
It need to wait for now. Seems to be good future improvement.

>>
>>> Fixes: db5377883fef (It should be FAILED when raid has)
>>> Signed-off-by: Xiao Ni <xni@redhat.com>
>>> Reported-by: Fine Fan <ffan@redhat.com>
>>> ---
>>>    Detail.c | 12 +++++++-----
>>>    1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Detail.c b/Detail.c
>>> index d3af0ab..2164de3 100644
>>> --- a/Detail.c
>>> +++ b/Detail.c
>>> @@ -351,11 +351,13 @@ int Detail(char *dev, struct context *c)
>>>        avail = xcalloc(array.raid_disks, 1);
>>>
>>>        for (d = 0; d < array.raid_disks; d++) {
>>> -             char *dv, *dv_rep;
>>> -             dv = map_dev_preferred(disks[d*2].major,
>>> -                             disks[d*2].minor, 0, c->prefer);
>>> -             dv_rep = map_dev_preferred(disks[d*2+1].major,
>>> -                             disks[d*2+1].minor, 0, c->prefer);
>>> +             char *dv, *dv_rep = NULL;
>>> +
>>> +             if (!disks[d*2].major && !disks[d*2].minor)
>>> +                     continue; > +
>>> +             dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
>>> +             dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
>>>
>>>                if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
>>>                    (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
>>>
>>
>> Yeah, I know that it is used in Detail this way, but please  determine
>> way to replace this ugly [d*2] and [d*2+1].
> 
> Yes, it takes me much time to understand how to calculate avail[]. We
> can focus on
> fixing this bug first, then we prepare some patches for improving the codes that
> related to the function map_dev_preferred and the [d*2] format codes.
> It might be
> a big change.
> 
Agree.

>>
>> This whole block should be moved from Detail() code to separate
>> function, which determines if device or replacement is in sync.
> 
> A good suggestion. Put it into the change I mentioned above, is it ok?
> 
Agree. So, will you take care about all improvements later (after release)?

Thanks,
Mariusz

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

On Mon, Oct 18, 2021 at 3:15 PM Tkaczyk, Mariusz
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> >> +        dv = devid2kname(makedev(disks[d*2].major, disks[d*2].minor));
> >> +        dv_rep = devid2kname(makedev(disks[d*2+1].major, disks[d*2+1].minor));
>
> devid2kname() returns static memory. If both drive and replacement
> are available then dv value will be overwritten. Not sure if it is
> possible.
>

I think it has the possibility. It only wants to check if the disk exists. Maybe
it's better to write a new api or check if the device exists directly
here. I'll try to avoid
the problem you mentioned.

Best Regards
Xiao

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

On Mon, Oct 18, 2021 at 7:52 PM Tkaczyk, Mariusz
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> There is a code:
>         if (create && !regular && !preferred) {
>                 static char buf[30]; <- this variable will survive retry.
>                 snprintf(buf, sizeof(buf), "%d:%d", major, minor);
>                 regular = buf;
>         }
> but seems that it is not a case for this scenario. I suspected that
> this was used because when gathering container name:
>
>                 container = map_dev_preferred(major(devid), minor(devid),
>                                               1, c->prefer);
>
> 'create' is explicitly set to 1. That is why I expect to have 'container'
> declared in static area. Make sense?
>

I c.

>
> >>
> >> This whole block should be moved from Detail() code to separate
> >> function, which determines if device or replacement is in sync.
> >
> > A good suggestion. Put it into the change I mentioned above, is it ok?
> >
> Agree. So, will you take care about all improvements later (after release)?
>

I plan to do this after you talk about them. If you want to fix them, I can help
to review too. I'll ping you when I'm ready to do this to check if you
start doing
it.

Best Regards
Xiao

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Jes Sorensen]

On 10/18/21 9:05 AM, Xiao Ni wrote:
> On Mon, Oct 18, 2021 at 7:52 PM Tkaczyk, Mariusz
> <mariusz.tkaczyk@linux.intel.com> wrote:
>>
>> There is a code:
>>         if (create && !regular && !preferred) {
>>                 static char buf[30]; <- this variable will survive retry.
>>                 snprintf(buf, sizeof(buf), "%d:%d", major, minor);
>>                 regular = buf;
>>         }
>> but seems that it is not a case for this scenario. I suspected that
>> this was used because when gathering container name:
>>
>>                 container = map_dev_preferred(major(devid), minor(devid),
>>                                               1, c->prefer);
>>
>> 'create' is explicitly set to 1. That is why I expect to have 'container'
>> declared in static area. Make sense?
>>
> 
> I c.
> 
>>
>>>>
>>>> This whole block should be moved from Detail() code to separate
>>>> function, which determines if device or replacement is in sync.
>>>
>>> A good suggestion. Put it into the change I mentioned above, is it ok?
>>>
>> Agree. So, will you take care about all improvements later (after release)?
>>
> 
> I plan to do this after you talk about them. If you want to fix them, I can help
> to review too. I'll ping you when I'm ready to do this to check if you
> start doing
> it.

Xiao, Mariusz,

I'll ignore this one for now, based on your discussion. Please yell if
you disagree.

Thanks,
Jes

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Jes Sorensen]

On 10/21/21 8:09 PM, Xiao Ni wrote:
> On Thu, Oct 21, 2021 at 5:13 PM Tkaczyk, Mariusz
> <mariusz.tkaczyk@linux.intel.com> wrote:
>>
>> Hi Xiao,
>> On 20.10.2021 16:38, Xiao Ni wrote:
>>> +             char dv[32], dv_rep[32];
>>> +
>>> +             sprintf(dv, "/sys/dev/block/%d:%d",
>>> +                             disks[d*2].major, disks[d*2].minor);
>>> +             sprintf(dv_rep, "/sys/dev/block/%d:%d",
>>> +                             disks[d*2+1].major, disks[d*2+1].minor);Please use snprintf and PATH_MAX instead 32.
>>> +
>>> +             if ((!access(dv, R_OK) &&
>>> +                 (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
>> IMO not correct style, please verify with checkpatch.
>> should be: [d * 2]
> 
> Hi Mariusz
> 
> I ran checkpatch before sending this patch. The checkpatch I used is
> from Song's git
> (https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git). It only
> reports one warning
> like this:
> 
> WARNING: Unknown commit id 'db5377883fef', maybe rebased or not pulled?
> #34:
> Fixes: db5377883fef (It should be FAILED when raid has)
> 
> total: 0 errors, 1 warnings, 25 lines checked
> 
> It's right. Because the commit is from mdadm git. Do we use different
> checkpatch?

checkpatch is awful in general, but I agree with Mariusz, adding the
spaces is a lot prettier.

> 
>>> +                 (!access(dv_rep, R_OK) &&
>>> +                 (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
>>
>> Could you define function for that?
>> something like (you can add access() verification if needed):
>> is_dev_alive(mdu_disk_info_t *disk)
>> {
>>         char *devnm = devid2kname(makedev..);
>>         if (devnm)
>>                 return true;
>>         return false;
>> }
> 
> Sure, it sounds better. I'll do this in the next version.
>>
>> using true/false will require to add #include <stdbool.h>.
>> Jes suggests to use meaningful return values. This is only
>> suggestion so you can ignore it and use 0 and 1.
> 
> <stdbool.h> is a c++ header and it needs libstdc++-devel, I don't want
> to include one package only for using true/false.

stdbool.h is provided by GCC directly on Fedora 33 it's
/usr/lib/gcc/x86_64-redhat-linux/10/include/stdbool.h

>> and then check:
>> if (is_dev_alive([d * 2]) & disks[d * 2].state & (1<<MD_DISK_SYNC) ||
>>     (is_dev_alive([d * 2 + 1]) & disks[d * 2 + 1].state & (1<<MD_DISK_SYNC))
>>
>> What do you think?
> 
> It's good for me.

I think using bool for this makes sense when the helper is called
'is_dev_alive()' (as much as I dislike the bool type).

Cheers,
Jes

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

On Fri, Oct 22, 2021 at 8:09 AM Xiao Ni <xni@redhat.com> wrote:
> >
> > using true/false will require to add #include <stdbool.h>.
> > Jes suggests to use meaningful return values. This is only
> > suggestion so you can ignore it and use 0 and 1.
>
> <stdbool.h> is a c++ header and it needs libstdc++-devel, I don't want
> to include one package only for using true/false.
>
Hi all

Do you think the method is better?

typedef int bool
#define true 1
#define false 0

So we don't need to inlcuce <stdboo.h> and we can use true/false too.
If we include <stdbool.h>, it needs to add the corresponding package in the spec
file too to resolve dependency problems.

Regards
Xiao

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

On Thu, Oct 21, 2021 at 5:13 PM Tkaczyk, Mariusz
<mariusz.tkaczyk@linux.intel.com> wrote:
>
> Hi Xiao,
> On 20.10.2021 16:38, Xiao Ni wrote:
> > +             char dv[32], dv_rep[32];
> > +
> > +             sprintf(dv, "/sys/dev/block/%d:%d",
> > +                             disks[d*2].major, disks[d*2].minor);
> > +             sprintf(dv_rep, "/sys/dev/block/%d:%d",
> > +                             disks[d*2+1].major, disks[d*2+1].minor);Please use snprintf and PATH_MAX instead 32.
> > +
> > +             if ((!access(dv, R_OK) &&
> > +                 (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
> IMO not correct style, please verify with checkpatch.
> should be: [d * 2]

Hi Mariusz

I ran checkpatch before sending this patch. The checkpatch I used is
from Song's git
(https://git.kernel.org/pub/scm/linux/kernel/git/song/md.git). It only
reports one warning
like this:

WARNING: Unknown commit id 'db5377883fef', maybe rebased or not pulled?
#34:
Fixes: db5377883fef (It should be FAILED when raid has)

total: 0 errors, 1 warnings, 25 lines checked

It's right. Because the commit is from mdadm git. Do we use different
checkpatch?

> > +                 (!access(dv_rep, R_OK) &&
> > +                 (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
>
> Could you define function for that?
> something like (you can add access() verification if needed):
> is_dev_alive(mdu_disk_info_t *disk)
> {
>         char *devnm = devid2kname(makedev..);
>         if (devnm)
>                 return true;
>         return false;
> }

Sure, it sounds better. I'll do this in the next version.
>
> using true/false will require to add #include <stdbool.h>.
> Jes suggests to use meaningful return values. This is only
> suggestion so you can ignore it and use 0 and 1.

<stdbool.h> is a c++ header and it needs libstdc++-devel, I don't want
to include one package only for using true/false.

>
> and then check:
> if (is_dev_alive([d * 2]) & disks[d * 2].state & (1<<MD_DISK_SYNC) ||
>     (is_dev_alive([d * 2 + 1]) & disks[d * 2 + 1].state & (1<<MD_DISK_SYNC))
>
> What do you think?

It's good for me.

Best Regards
Xiao

Re: [PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Tkaczyk, Mariusz]

Hi Xiao,
On 20.10.2021 16:38, Xiao Ni wrote:
> +		char dv[32], dv_rep[32];
> +
> +		sprintf(dv, "/sys/dev/block/%d:%d",
> +				disks[d*2].major, disks[d*2].minor);
> +		sprintf(dv_rep, "/sys/dev/block/%d:%d",
> +				disks[d*2+1].major, disks[d*2+1].minor);Please use snprintf and PATH_MAX instead 32.
> +
> +		if ((!access(dv, R_OK) &&
> +		    (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
IMO not correct style, please verify with checkpatch.
should be: [d * 2]
> +		    (!access(dv_rep, R_OK) &&
> +		    (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {

Could you define function for that?
something like (you can add access() verification if needed):
is_dev_alive(mdu_disk_info_t *disk)
{
	char *devnm = devid2kname(makedev..);
	if (devnm)
		return true;
	return false;
}

using true/false will require to add #include <stdbool.h>.
Jes suggests to use meaningful return values. This is only
suggestion so you can ignore it and use 0 and 1.

and then check:
if (is_dev_alive([d * 2]) & disks[d * 2].state & (1<<MD_DISK_SYNC) ||
    (is_dev_alive([d * 2 + 1]) & disks[d * 2 + 1].state & (1<<MD_DISK_SYNC))

What do you think?
Thanks,
Mariusz

[PATCH 1/1] mdadm/Detail: Can't show container name correctly when unpluging disks

[Xiao Ni]

The test case is:
1. create one imsm container
2. create a raid5 device from the container
3. unplug two disks
4. mdadm --detail /dev/md126
[root@rhel85 ~]# mdadm -D /dev/md126
/dev/md126:
         Container : ��, member 0

The Detail function first gets container name by function
map_dev_preferred. Then it tries to find which disks are
available. In patch db5377883fef(It should be FAILED..)
uses map_dev_preferred to find which disks are under /dev.

But now, the major/minor information comes from kernel space.
map_dev_preferred malloc memory and init a device list when
first be called by Detail. It can't find the device in the
list by the major/minor. It free the memory and reinit the
list.

The container name now points to an area tha has been freed.
So the containt is a mess.

This patch replaces map_dev_preferred with access.

Fixes: db5377883fef (It should be FAILED when raid has)
Signed-off-by: Xiao Ni <xni@redhat.com>
Reported-by: Fine Fan <ffan@redhat.com>
---
v2: use access rather than devid2kname
---
 Detail.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/Detail.c b/Detail.c
index d3af0ab..df59378 100644
--- a/Detail.c
+++ b/Detail.c
@@ -351,14 +351,17 @@ int Detail(char *dev, struct context *c)
 	avail = xcalloc(array.raid_disks, 1);
 
 	for (d = 0; d < array.raid_disks; d++) {
-		char *dv, *dv_rep;
-		dv = map_dev_preferred(disks[d*2].major,
-				disks[d*2].minor, 0, c->prefer);
-		dv_rep = map_dev_preferred(disks[d*2+1].major,
-				disks[d*2+1].minor, 0, c->prefer);
-
-		if ((dv && (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
-		    (dv_rep && (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
+		char dv[32], dv_rep[32];
+
+		sprintf(dv, "/sys/dev/block/%d:%d",
+				disks[d*2].major, disks[d*2].minor);
+		sprintf(dv_rep, "/sys/dev/block/%d:%d",
+				disks[d*2+1].major, disks[d*2+1].minor);
+
+		if ((!access(dv, R_OK) &&
+		    (disks[d*2].state & (1<<MD_DISK_SYNC))) ||
+		    (!access(dv_rep, R_OK) &&
+		    (disks[d*2+1].state & (1<<MD_DISK_SYNC)))) {
 			avail_disks ++;
 			avail[d] = 1;
 		} else
-- 
2.7.5