On Fri, 22 Jul 2011 16:32:03 +0900 Namhyung Kim <namhyung@xxxxxxxxx> wrote:
> NeilBrown <neilb@xxxxxxx> writes:
>
> > The difference between the RAID5 and RAID6 code here is easily
> > resolved using conf->max_degraded.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
> Reviewed-by: Namhyung Kim <namhyung@xxxxxxxxx>
>
> and a coding style issue.
>
> > + if (s.failed <= conf->max_degraded && !conf->mddev->ro)
> > + for (i = 0; i < s.failed; i++) {
> > + struct r5dev *dev = &sh->dev[s.failed_num[i]];
> > + if (test_bit(R5_ReadError, &dev->flags)
> > + && !test_bit(R5_LOCKED, &dev->flags)
> > + && test_bit(R5_UPTODATE, &dev->flags)
> > + ) {
>
> This line looks weird and should be combined to previous line.
>
> And I slightly prefer that the logical operators are put on the
> same line with previous conditional, so that next one can be on
> the same column and more readable, but ...
>
I agree that it is nice if they line up. But I also think the "&&" is
important and want it to stand out. In this case the '!' would stop them
from lining up very nicely, so I'll leave it as it is.
The thing I like about having ") {" on a separate line is that I can later
add a condition by just adding a line - it makes the patch easier to read.
Not a bit thing maybe and I don't always structure conditions like this, but
I think I'll leave it this time.
Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
