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

[Jes Sorensen <jes@trained-monkey.org>]

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