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

On 10/21/21 8:09 PM, Xiao Ni wrote:

On Thu, Oct 21, 2021 at 5:13 PM Tkaczyk, Mariusz
[off-list ref] wrote:

quoted

Hi Xiao,
On 20.10.2021 16:38, Xiao Ni wrote:

quoted

+             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.

quoted

quoted

+                 (!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.

quoted

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

quoted

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