On Mon, 5 Jul 2010 10:35:26 +0100 "Czarnowska, Anna" <anna.czarnowska@xxxxxxxxx> wrote: > To avoid repeated retrieving of parent container or volumes list. > > Simplifies finding unused disk in a container. > > Function check_disk_is_free will be used for spare sharing. > > > > Signed-off-by: Marcin Labun <marcin.labun@xxxxxxxxx<mailto:marcin.labun@xxxxxxxxx>> > > --- > > Monitor.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++---------- > > 1 files changed, 102 insertions(+), 21 deletions(-) > > > > diff --git a/Monitor.c b/Monitor.c > > index 8e82797..e16af2d 100644 > > --- a/Monitor.c > > +++ b/Monitor.c > > @@ -38,6 +38,24 @@ static void alert(char *event, char *dev, char *disc, char *mailaddr, char *mail > > * At least it isn't MD_SB_DISKS. > > */ > > #define MaxDisks 384 > > +struct state { > > + char *devname; > > + int devnum; /* to sync with mdstat info */ > > + long utime; > > + int err; > > + char *spare_group; > > + int active, working, failed, spare, raid, total; > > + int expected_spares; > > + int devstate[MaxDisks]; > > + int devid[MaxDisks]; > > + int percent; > > + char *metadata_version; > > + struct state *next; > > + struct state *volumes; > > + struct state *parent; > > + struct state *missing; I'm having trouble figuring out what next/volumes/parent/missing are meant to mean exactly, and there is a lack of any comments explaining it. I suspect that is probably makes sense, but I need some help understanding exactly what you are doing here.... > > @@ -281,11 +298,9 @@ int Monitor(mddev_dev_t devlist, > > st->spare == array.spare_disks && > > (mse == NULL || ( > > mse->percent == st->percent > > - ))) { > > - close(fd); > > + ))) > > st->err = 0; > > - continue; > > - } > That if statement no just sets st->err=0, which happens again a few lines later, so it seems the if statement is now pointless.... But it was there for a reason - I think to short-circuit a collections of tests that won't be needed if the 'if' succeeds. Maybe you could explain why you want to remove the 'if' now. > > + if (missing_parent_list) > > + st->missing = missing_parent_list; > > + else > > + st->missing = NULL; This 'if' is exactly equivalent to st->missing = missing_parent_list; ! > struct mdstat_ent *mse; > > for (mse=mdstat; mse; mse=mse->next) > > if (mse->devnum != INT_MAX && > > - mse->level && > > - (strcmp(mse->level, "raid0")!=0 && > > - strcmp(mse->level, "linear")!=0) > > - ) { > > + (!mse->level || /* retrieve containers */ > > + (strcmp(mse->level, "raid0") != 0 && > > + strcmp(mse->level, "linear") != 0))) { You are messing up the indenting rather badly. Please preserve the formatting style of the original code. > > + if (st->metadata_version) > > + free(st->metadata_version); You don't need the test, as free(NULL) is defined to do noting. > > > > +/* check if disk is used in donor array (native) or any volume in donor > > +container (external)*/ int check_disk_is_free(struct state *donor, int > > +disk_idx, int ext) { Some how the comment and the function declaration got wrapped together. Not nice. 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
