On Fri, 22 Jul 2011 16:09:10 +0900 Namhyung Kim <namhyung@xxxxxxxxx> wrote:
> NeilBrown <neilb@xxxxxxx> writes:
>
> > Prior to commit ab69ae12ceef7 the code in handle_stripe5 and
> > handle_stripe6 to "Finish reconstruct operations initiated by the
> > expansion process" was identical.
> > That commit added an identical stanza of code to each function, but in
> > different places. That was careless.
> >
> > The raid5 code was correct, so move that out into handle_stripe and
> > remove raid6 version.
> >
> > Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
> Reviewed-by: Namhyung Kim <namhyung@xxxxxxxxx>
>
> and nitpick below..
>
>
> > static void handle_stripe(struct stripe_head *sh)
> > {
> > struct stripe_head_state s;
> > + int done;
> > raid5_conf_t *conf = sh->raid_conf;
> > + int i;
>
> Can be declared in a line.
>
True.... There are differing opinions on whether that is a good idea or not.
I tend to like to keep each variable in it's own declaration, though closely
related variables might get put together.
I have moved 'int i' up one line so the addition is in just one place, but
I've kept it separate from 'done'.
> > + /* Finish reconstruct operations initiated by the expansion process */
> > + if (sh->reconstruct_state == reconstruct_state_result) {
> > + struct stripe_head *sh2
> > + = get_active_stripe(conf, sh->sector, 1, 1, 1);
>
> Wouldn't it be better to use more descriptive name rather than sh2,
> something like sh_prev, sh_src or whatever?
good point. 'sh_src' it is.
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
