On Thu, Oct 21, 2021 at 5:13 PM Tkaczyk, Mariusz <mariusz.tkaczyk@xxxxxxxxxxxxxxx> 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
