On 10/21/21 8:09 PM, Xiao Ni wrote: > 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? 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
