> -----Original Message----- > From: NeilBrown [mailto:neilb@xxxxxxx] > Sent: Thursday, December 15, 2011 12:08 PM > To: Kwolek, Adam > Cc: linux-raid@xxxxxxxxxxxxxxx; Ciechanowski, Ed; Labun, Marcin; Williams, > Dan J > Subject: Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails > > On Thu, 15 Dec 2011 10:28:08 +0000 "Kwolek, Adam" > <adam.kwolek@xxxxxxxxx> > wrote: > > > > > > > > -----Original Message----- > > > From: NeilBrown [mailto:neilb@xxxxxxx] > > > Sent: Thursday, December 15, 2011 5:01 AM > > > To: Kwolek, Adam > > > Cc: linux-raid@xxxxxxxxxxxxxxx; Ciechanowski, Ed; Labun, Marcin; > > > Williams, Dan J > > > Subject: Re: [PATCH 2/3] imsm: FIX: 'UT 09imsm-assemble' fails > > > > > > On Wed, 14 Dec 2011 16:07:12 +0100 Adam Kwolek > > > <adam.kwolek@xxxxxxxxx> wrote: > > > > > > > Problem was introduced by patch (2011-06-08): > > > > getinfo_super now clears the 'info' structure before filling it in. > > > > > > > > Field update private is not managed here and pointer associated > > > > outside is cleaned up. > > > > Add code for field update_private cleaning preservation. > > > > In places where in patch > > > > 'getinfo_super now clears the 'info' structure before filling it in.' > > > > cleaning structure was removed, cleaning update_private field was > > > > added as getinfo_super() cannot be responsible for this pointer > > > management. > > > > > > > > Signed-off-by: Adam Kwolek <adam.kwolek@xxxxxxxxx> > > > > --- > > > > > > > > Assemble.c | 2 ++ > > > > Incremental.c | 3 +++ > > > > super-intel.c | 9 +++++++++ > > > > 3 files changed, 14 insertions(+), 0 deletions(-) > > > > > > > > diff --git a/Assemble.c b/Assemble.c index fac2bad..c8b538f 100644 > > > > --- a/Assemble.c > > > > +++ b/Assemble.c > > > > @@ -422,6 +422,7 @@ int Assemble(struct supertype *st, char > *mddev, > > > > int uuid[4]; > > > > > > > > content = &info; > > > > + info.update_private = NULL > > > > tst->ss->getinfo_super(tst, content, > > > NULL); > > > > > > > > if (!parse_uuid(ident->container, > > > uuid) || @@ -485,6 +486,7 @@ > > > > int Assemble(struct supertype *st, char *mddev, > > > > } else { > > > > > > > > content = &info; > > > > + info.update_private = NULL > > > > tst->ss->getinfo_super(tst, content, NULL); > > > > > > > > if (!ident_matches(ident, content, tst, diff --git > > > a/Incremental.c > > > > b/Incremental.c index d3724a4..112a1ec 100644 > > > > --- a/Incremental.c > > > > +++ b/Incremental.c > > > > @@ -205,6 +205,7 @@ int Incremental(char *devname, int verbose, > > > > int > > > runstop, > > > > } > > > > close (dfd); dfd = -1; > > > > > > > > + info.update_private = NULL > > > > st->ss->getinfo_super(st, &info, NULL); > > > > > > > > /* 3/ Check if there is a match in mdadm.conf */ @@ -404,6 > > > > +405,7 > > > @@ > > > > int Incremental(char *devname, int verbose, int runstop, > > > > goto out_unlock; > > > > } > > > > close(dfd2); > > > > + info.update_private = NULL > > > > st2->ss->getinfo_super(st2, &info2, NULL); > > > > st2->ss->free_super(st2); > > > > if (info.array.level != info2.array.level || @@ -1382,6 > > > +1384,7 @@ > > > > static int Incremental_container(struct supertype *st, char *devname, > > > > int ra_blocked = 0; > > > > int ra_all = 0; > > > > > > > > + info.update_private = NULL > > > > st->ss->getinfo_super(st, &info, NULL); > > > > > > > > if ((runstop > 0 && info.container_enough >= 0) || diff --git > > > > a/super-intel.c b/super-intel.c index e1073ef..5e1d278 100644 > > > > --- a/super-intel.c > > > > +++ b/super-intel.c > > > > @@ -2365,8 +2365,13 @@ static void > > > > getinfo_super_imsm_volume(struct > > > supertype *st, struct mdinfo *info, > > > > char *devname; > > > > unsigned int component_size_alligment; > > > > int map_disks = info->array.raid_disks; > > > > + void *update_private_saver = info->update_private; > > > > > > > > memset(info, 0, sizeof(*info)); > > > > + /* preserve pointer cleanup, as someone elese is pointer owner > > > > + */ > > > > + info->update_private = update_private_saver; > > > > + > > > > if (prev_map) > > > > map_to_analyse = prev_map; > > > > > > > > @@ -2601,12 +2606,16 @@ static void getinfo_super_imsm(struct > > > supertype *st, struct mdinfo *info, char * > > > > int max_enough = -1; > > > > int i; > > > > struct imsm_super *mpb; > > > > + void *update_private_saver = info->update_private; > > > > > > > > if (super->current_vol >= 0) { > > > > getinfo_super_imsm_volume(st, info, map); > > > > return; > > > > } > > > > memset(info, 0, sizeof(*info)); > > > > + /* preserve pointer cleanup, as someone elese is pointer owner > > > > + */ > > > > + info->update_private = update_private_saver; > > > > > > > > /* Set raid_disks to zero so that Assemble will always pull in valid > > > > * spares > > > > > > > > > > You didn't really think I'd let that through, did you :-) > > > > > > I've written an alternate which gets rid of update_private. > > > > > > Could you and Dan please review and check it performs as required. > > > > > > Thanks, > > > NeilBrown > > > > > > > > Don't you think that condition should set rv variable in opposite way? E.g.: > > + } else { > > + if (info->uuid[0] != info->uuid[1] || > > + info->uuid[1] != info->uuid[2] || > > + info->uuid[2] != info->uuid[3]) > > + rv = 0; > > + else > > + rv = -1; > > } > > > > I think that when condition is true, this means "random" value and update > should be performed. > > If all 4 numbers are the same, then it is almost certain that uuid_set was true > the first time through and so we made them all the same. > In that case we want to succeed and used the first number as the family. > If the number are not all the same, then uuid_set was never true so the uuid > was given on the command line and we cannot handle that case so we give > an error. > > So I think the original code is correct. > > Thanks, > NeilBrown ... what do you think about adding '!' in condition 'if (uuid_set) {' It is like your comment in code directs to. With this change test looks ok, but I can miss something this time also ;). BR Adam > > > > > > > BR > > Adam > > > > > > > > > > > > > commit 8606be19835abaf888d0fbd1729dcda82a8b2815 > > > Author: NeilBrown <neilb@xxxxxxx> > > > Date: Thu Dec 15 14:59:12 2011 +1100 > > > > > > Remove update_private > > > > > > This fields doesn't work any more as ->getinfo_super clears the info > > > structure at an awkward time. So get rid of it and do it differently. > > > > > > The issue is that the metadata handler cannot tell if the uuid it has > > > was randomly generated or explicitly requested, except on the first > > > call. > > > And we don't want to accept explicit requests for IMSM. > > > So when it was auto-generated, make it look distinctive by having the > > > same int copied in all 4 positions. If someone requests a uuid like > > > that, I guess they get away with it. > > > > > > Reported-by: Adam Kwolek <adam.kwolek@xxxxxxxxx> > > > Signed-off-by: NeilBrown <neilb@xxxxxxx> > > > > > > diff --git a/Assemble.c b/Assemble.c index fac2bad..74fb6a3 100644 > > > --- a/Assemble.c > > > +++ b/Assemble.c > > > @@ -706,7 +706,6 @@ int Assemble(struct supertype *st, char *mddev, > > > bitmap_done = 0; > > > #endif > > > /* Ok, no bad inconsistancy, we can try updating etc */ > > > - content->update_private = NULL; > > > devices = malloc(num_devs * sizeof(*devices)); > > > devmap = calloc(num_devs * content->array.raid_disks, 1); > > > for (tmpdev = devlist; tmpdev; tmpdev=tmpdev->next) if (tmpdev- > > > >used == 1) { @@ -891,8 +890,6 @@ int Assemble(struct supertype *st, > > > >char > > > *mddev, > > > } > > > devcnt++; > > > } > > > - free(content->update_private); > > > - content->update_private = NULL; > > > > > > if (devcnt == 0) { > > > fprintf(stderr, Name ": no devices found for %s\n", diff --git > > > a/mdadm.h b/mdadm.h index 1351d42..4f3533f 100644 > > > --- a/mdadm.h > > > +++ b/mdadm.h > > > @@ -217,11 +217,6 @@ struct mdinfo { > > > unsigned long cache_size; /* size of raid456 stripe cache*/ > > > int mismatch_cnt; > > > char text_version[50]; > > > - void *update_private; /* for passing metadata- > > > format > > > - * specific update data > > > - * between successive calls > > > to > > > - * update_super() > > > - */ > > > > > > int container_member; /* for assembling external-metatdata arrays > > > * This is to be used internally by metadata diff -- > git > > > a/super-intel.c b/super-intel.c index e1073ef..78781c7 100644 > > > --- a/super-intel.c > > > +++ b/super-intel.c > > > @@ -2791,25 +2791,30 @@ static int update_super_imsm(struct > > > supertype *st, struct mdinfo *info, > > > > > > mpb = super->anchor; > > > > > > - if (strcmp(update, "uuid") == 0 && uuid_set && !info- > > > >update_private) > > > - rv = -1; > > > - else if (strcmp(update, "uuid") == 0 && uuid_set && info- > > > >update_private) { > > > - mpb->orig_family_num = *((__u32 *) info- > > > >update_private); > > > - rv = 0; > > > - } else if (strcmp(update, "uuid") == 0) { > > > - __u32 *new_family = malloc(sizeof(*new_family)); > > > - > > > - /* update orig_family_number with the incoming random > > > - * data, report the new effective uuid, and store the > > > - * new orig_family_num for future updates. > > > + if (strcmp(update, "uuid") == 0) { > > > + /* We take this to mean that the family_num should be > > > updated. > > > + * However that is much smaller than the uuid so we cannot > > > really > > > + * allow an explicit uuid to be given. And it is hard to reliably > > > + * know if one was. > > > + * So if !uuid_set we know the current uuid is random and > > > just used > > > + * the first 'int' and copy it to the other 3 positions. > > > + * Otherwise we require the 4 'int's to be the same as would > > > be the > > > + * case if we are using a random uuid. So an explicit uuid will > > > be > > > + * accepted as long as all for ints are the same... which > > > shouldn't > > > +hurt > > > */ > > > - if (new_family) { > > > - memcpy(&mpb->orig_family_num, info->uuid, > > > sizeof(__u32)); > > > - uuid_from_super_imsm(st, info->uuid); > > > - *new_family = mpb->orig_family_num; > > > - info->update_private = new_family; > > > + if (uuid_set) { > > > + info->uuid[1] = info->uuid[2] = info->uuid[3] = info- > > > >uuid[0]; > > > rv = 0; > > > + } else { > > > + if (info->uuid[0] != info->uuid[1] || > > > + info->uuid[1] != info->uuid[2] || > > > + info->uuid[2] != info->uuid[3]) > > > + rv = -1; > > > + else > > > + rv = 0; > > > } > > > + if (rv == 0) > > > + mpb->orig_family_num = info->uuid[0]; > > > } else if (strcmp(update, "assemble") == 0) > > > rv = 0; > > > else -- 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
