>-----Original Message----- >From: dan.j.williams@xxxxxxxxx [mailto:dan.j.williams@xxxxxxxxx] On Behalf >Of Dan Williams >Sent: Thursday, April 15, 2010 12:15 AM >To: Hawrylewicz Czarnowski, Przemyslaw >Cc: Neil Brown; linux-raid@xxxxxxxxxxxxxxx; Ciechanowski, Ed >Subject: Re: [PATCH] imsm: assign UUID of spare device based on ARRAY >*device* keyword in mdadm configuration file. > >On Thu, Apr 8, 2010 at 7:48 AM, Hawrylewicz Czarnowski, Przemyslaw ><przemyslaw.hawrylewicz.czarnowski@xxxxxxxxx> wrote: >> If config has list of devices associated with container and it comprises >given spare drive, UUID is set apporopriately to this container. >> It allows to assign spare with proper container according >> to content in devices parameter in configuration file. >> > >I guess it would be nice if a user could direct spares via the >configuration file, but what gets honored in the future when this >value conflicts with a container group id in the metadata? Especially You are right, but at this moment it is *always* first container. This proposal gives a way how user can point out the right one. >when the actual device associated with a given name /dev/sda changes >from one boot to the next, potentially even across controllers, or the >admin moves the drive and forgets to update the configuration file. but it will need to rewrite this part of config to respect eg. /dev/disk/by-id patterns which are constant, which is far beyond this patch. >As much as possible I would like everything to be determined from the >platform / metadata and discourage using the 'devices' line in the >configuration file. As I have written above, the solution available now generates more opportunities to make a mistake. > >So, I'm on the fence with this one. > >A couple technical comments below: > >> Signed-off-by: Przemyslaw Czarnowski ><przemyslaw.hawrylewicz.czarnowski@xxxxxxxxx> >> --- >> super-intel.c | 48 +++++++++++++++++++++++++++++++++++++++++++----- [cut] >> + >> /* check the config file to see if we can return a real uuid for this >spare */ >> -static void fixup_container_spare_uuid(struct mdinfo *inf) >> +static void fixup_container_spare_uuid(struct mdinfo *inf, struct dl >*disk) >> { >> - struct mddev_ident_s *array_list; >> + struct mddev_ident_s *array_list, *first_match = NULL; >> >> if (inf->array.level != LEVEL_CONTAINER || >> memcmp(inf->uuid, uuid_match_any, sizeof(int[4])) != 0) >> @@ -1520,12 +1529,41 @@ static void fixup_container_spare_uuid(struct >mdinfo *inf) >> _sst = NULL; >> >> if (_sst) { >> - memcpy(inf->uuid, array_list->uuid, >sizeof(int[4])); >> + char *devices = array_list->devices; >> + >> + if (!first_match) >> + first_match = array_list; >> + >> + while (disk && devices && *devices) { >> + char path[PATH_MAX]; >> + char *p = devices; >> + int fd; >> + >> + devices = strchr(devices, ','); >> + if (!devices) >> + devices = p + strlen(p); >> + if (devices-p < PATH_MAX) { >> + strncpy(path, p, devices >- p); >> + if ((fd = open(path, >O_RDONLY)) >= 0) { >> + if >(matchfd2devnum(fd, disk->major, disk->minor)) { >> + >free(_sst); >> + >close(fd); >> + goto >match; >> + } >> + close(fd); >> + } >> + if (*devices == ',') >> + devices++; >> + } >> + } > >This routine is already available: match_oneof() in config.c This one is more specific, but you right, it may be generalized and reused. > >> free(_sst); >> - break; >> } >> } >> } >> +match:; > >Don't need a semicolon after a label. It had left from the previous version of code when compiler complained that there is no semicolon. > >-- >Dan -- 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
