| Message ID | 20250318054638.58276-1-xni@redhat.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/1] mdadm: check posix name before setting name and devname | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_14-PR | fail | merge-conflict |
Dear Xiao, Thank you for the patch. Wasn’t a similar patch sent to the list already months ago? For the commit message subject/title I suggest: mdadm: Allow to assemble existing arrays with non-POSIX names Am 18.03.25 um 06:46 schrieb Xiao Ni: > It's good to has limitation for name when creating an array. But the arrays … to have limitations … > which were created before patch e2eb503bd797 (mdadm: Follow POSIX Portable > Character Set) can't be assembled. In this patch, it removes the posix > check for assemble mode. “In this patch” is redundant in the commit message. Maybe: So, remove the POSIX check for assemble mode. Maybe add how to reproduce this? Is there a way to create a non-POSIX name with current mdadm, or should such a file be provided for tests. > Fixes: e2eb503bd797 (mdadm: Follow POSIX Portable Character Set) > Signed-off-by: Xiao Ni <xni@redhat.com> > --- > config.c | 8 ++------ > mdadm.c | 11 +++++++++++ > 2 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/config.c b/config.c > index 8a8ae5e48c41..ef7dbc4eb29f 100644 > --- a/config.c > +++ b/config.c > @@ -208,11 +208,6 @@ static mdadm_status_t ident_check_name(const char *name, const char *prop_name, > return MDADM_STATUS_ERROR; > } > > - if (!is_name_posix_compatible(name)) { > - ident_log(prop_name, name, "Not POSIX compatible", cmdline); > - return MDADM_STATUS_ERROR; > - } > - > return MDADM_STATUS_SUCCESS; > } > > @@ -512,7 +507,8 @@ void arrayline(char *line) > > for (w = dl_next(line); w != line; w = dl_next(w)) { > if (w[0] == '/' || strchr(w, '=') == NULL) { > - _ident_set_devname(&mis, w, false); > + if (is_name_posix_compatible(w)) > + _ident_set_devname(&mis, w, false); > } else if (strncasecmp(w, "uuid=", 5) == 0) { > if (mis.uuid_set) > pr_err("only specify uuid once, %s ignored.\n", > diff --git a/mdadm.c b/mdadm.c > index 6200cd0e7f9b..9d5b0e567799 100644 > --- a/mdadm.c > +++ b/mdadm.c > @@ -732,6 +732,11 @@ int main(int argc, char *argv[]) > exit(2); > } > > + if (mode != ASSEMBLE && !is_name_posix_compatible(optarg)) { > + pr_err("%s Not POSIX compatible\n", optarg); > + exit(2); > + } > + > if (ident_set_name(&ident, optarg) != MDADM_STATUS_SUCCESS) > exit(2); > > @@ -1289,6 +1294,12 @@ int main(int argc, char *argv[]) > pr_err("an md device must be given in this mode\n"); > exit(2); > } > + > + if (mode != ASSEMBLE && !is_name_posix_compatible(devlist->devname)) { > + pr_err("%s Not POSIX compatible\n", devlist->devname); > + exit(2); > + } > + > if (ident_set_devname(&ident, devlist->devname) != MDADM_STATUS_SUCCESS) > exit(1); >
On Tue, Mar 18, 2025 at 3:38 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Xiao, > > > Thank you for the patch. Wasn’t a similar patch sent to the list already > months ago? > > For the commit message subject/title I suggest: > > mdadm: Allow to assemble existing arrays with non-POSIX names Hi Paul Thanks for reminding me about this. I forgot about this patch. I can't find the patch. Do you still have the link for it? > > Am 18.03.25 um 06:46 schrieb Xiao Ni: > > It's good to has limitation for name when creating an array. But the arrays > > … to have limitations … > > > which were created before patch e2eb503bd797 (mdadm: Follow POSIX Portable > > Character Set) can't be assembled. In this patch, it removes the posix > > check for assemble mode. > > “In this patch” is redundant in the commit message. Maybe: > > So, remove the POSIX check for assemble mode. good to me. > > Maybe add how to reproduce this? Is there a way to create a non-POSIX > name with current mdadm, or should such a file be provided for tests. For example: * build mdadm without patch e2eb503bd797 * mdadm -CR /dev/md/tbz:pv1 -l0 -n2 /dev/loop0 /dev/loop1 * mdadm -Ss * build with latest mdadm, and try to assemble it. * mdadm -A /dev/md/tbz:pv1 --name tbz:pv1 mdadm: Value "tbz:pv1" cannot be set as name. Reason: Not POSIX compatible. Regards Xiao > > > Fixes: e2eb503bd797 (mdadm: Follow POSIX Portable Character Set) > > Signed-off-by: Xiao Ni <xni@redhat.com> > > --- > > config.c | 8 ++------ > > mdadm.c | 11 +++++++++++ > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/config.c b/config.c > > index 8a8ae5e48c41..ef7dbc4eb29f 100644 > > --- a/config.c > > +++ b/config.c > > @@ -208,11 +208,6 @@ static mdadm_status_t ident_check_name(const char *name, const char *prop_name, > > return MDADM_STATUS_ERROR; > > } > > > > - if (!is_name_posix_compatible(name)) { > > - ident_log(prop_name, name, "Not POSIX compatible", cmdline); > > - return MDADM_STATUS_ERROR; > > - } > > - > > return MDADM_STATUS_SUCCESS; > > } > > > > @@ -512,7 +507,8 @@ void arrayline(char *line) > > > > for (w = dl_next(line); w != line; w = dl_next(w)) { > > if (w[0] == '/' || strchr(w, '=') == NULL) { > > - _ident_set_devname(&mis, w, false); > > + if (is_name_posix_compatible(w)) > > + _ident_set_devname(&mis, w, false); > > } else if (strncasecmp(w, "uuid=", 5) == 0) { > > if (mis.uuid_set) > > pr_err("only specify uuid once, %s ignored.\n", > > diff --git a/mdadm.c b/mdadm.c > > index 6200cd0e7f9b..9d5b0e567799 100644 > > --- a/mdadm.c > > +++ b/mdadm.c > > @@ -732,6 +732,11 @@ int main(int argc, char *argv[]) > > exit(2); > > } > > > > + if (mode != ASSEMBLE && !is_name_posix_compatible(optarg)) { > > + pr_err("%s Not POSIX compatible\n", optarg); > > + exit(2); > > + } > > + > > if (ident_set_name(&ident, optarg) != MDADM_STATUS_SUCCESS) > > exit(2); > > > > @@ -1289,6 +1294,12 @@ int main(int argc, char *argv[]) > > pr_err("an md device must be given in this mode\n"); > > exit(2); > > } > > + > > + if (mode != ASSEMBLE && !is_name_posix_compatible(devlist->devname)) { > > + pr_err("%s Not POSIX compatible\n", devlist->devname); > > + exit(2); > > + } > > + > > if (ident_set_devname(&ident, devlist->devname) != MDADM_STATUS_SUCCESS) > > exit(1); > > >
Dear Xiao, Am 18.03.25 um 09:18 schrieb Xiao Ni: > On Tue, Mar 18, 2025 at 3:38 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: >> Thank you for the patch. Wasn’t a similar patch sent to the list already >> months ago? >> >> For the commit message subject/title I suggest: >> >> mdadm: Allow to assemble existing arrays with non-POSIX names > Thanks for reminding me about this. I forgot about this patch. I can't > find the patch. Do you still have the link for it? I searched for *e2eb503bd797* in the linux-raid archive [1], I found the patch and discussion *[PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes* [2]. So, similar, and I think only the topic of already created arrays came up, your patch solves. Sorry for the confusion. >> Am 18.03.25 um 06:46 schrieb Xiao Ni: >>> It's good to has limitation for name when creating an array. But the arrays >> >> … to have limitations … >> >>> which were created before patch e2eb503bd797 (mdadm: Follow POSIX Portable >>> Character Set) can't be assembled. In this patch, it removes the posix >>> check for assemble mode. >> >> “In this patch” is redundant in the commit message. Maybe: >> >> So, remove the POSIX check for assemble mode. > > good to me. > >> Maybe add how to reproduce this? Is there a way to create a non-POSIX >> name with current mdadm, or should such a file be provided for tests. > > For example: > > * build mdadm without patch e2eb503bd797 > * mdadm -CR /dev/md/tbz:pv1 -l0 -n2 /dev/loop0 /dev/loop1 > * mdadm -Ss > * build with latest mdadm, and try to assemble it. > * mdadm -A /dev/md/tbz:pv1 --name tbz:pv1 > mdadm: Value "tbz:pv1" cannot be set as name. Reason: Not POSIX compatible. Thank you. For running tests, re-building mdadm might not be feasible. But having these instructions would be great to have in the commit message nevertheless. >>> Fixes: e2eb503bd797 (mdadm: Follow POSIX Portable Character Set) >>> Signed-off-by: Xiao Ni <xni@redhat.com> >>> --- >>> config.c | 8 ++------ >>> mdadm.c | 11 +++++++++++ >>> 2 files changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/config.c b/config.c >>> index 8a8ae5e48c41..ef7dbc4eb29f 100644 >>> --- a/config.c >>> +++ b/config.c >>> @@ -208,11 +208,6 @@ static mdadm_status_t ident_check_name(const char *name, const char *prop_name, >>> return MDADM_STATUS_ERROR; >>> } >>> >>> - if (!is_name_posix_compatible(name)) { >>> - ident_log(prop_name, name, "Not POSIX compatible", cmdline); >>> - return MDADM_STATUS_ERROR; >>> - } >>> - >>> return MDADM_STATUS_SUCCESS; >>> } >>> >>> @@ -512,7 +507,8 @@ void arrayline(char *line) >>> >>> for (w = dl_next(line); w != line; w = dl_next(w)) { >>> if (w[0] == '/' || strchr(w, '=') == NULL) { >>> - _ident_set_devname(&mis, w, false); >>> + if (is_name_posix_compatible(w)) >>> + _ident_set_devname(&mis, w, false); >>> } else if (strncasecmp(w, "uuid=", 5) == 0) { >>> if (mis.uuid_set) >>> pr_err("only specify uuid once, %s ignored.\n", >>> diff --git a/mdadm.c b/mdadm.c >>> index 6200cd0e7f9b..9d5b0e567799 100644 >>> --- a/mdadm.c >>> +++ b/mdadm.c >>> @@ -732,6 +732,11 @@ int main(int argc, char *argv[]) >>> exit(2); >>> } >>> >>> + if (mode != ASSEMBLE && !is_name_posix_compatible(optarg)) { >>> + pr_err("%s Not POSIX compatible\n", optarg); >>> + exit(2); >>> + } >>> + >>> if (ident_set_name(&ident, optarg) != MDADM_STATUS_SUCCESS) >>> exit(2); >>> >>> @@ -1289,6 +1294,12 @@ int main(int argc, char *argv[]) >>> pr_err("an md device must be given in this mode\n"); >>> exit(2); >>> } >>> + >>> + if (mode != ASSEMBLE && !is_name_posix_compatible(devlist->devname)) { >>> + pr_err("%s Not POSIX compatible\n", devlist->devname); >>> + exit(2); >>> + } >>> + >>> if (ident_set_devname(&ident, devlist->devname) != MDADM_STATUS_SUCCESS) >>> exit(1); Kind regards, Paul [1]: https://lore.kernel.org/linux-raid/?q=e2eb503bd797 [2]: https://lore.kernel.org/linux-raid/20241015173553.276546-1-loberman@redhat.com/
On Tue, Mar 18, 2025 at 4:24 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > Dear Xiao, > > > > Am 18.03.25 um 09:18 schrieb Xiao Ni: > > On Tue, Mar 18, 2025 at 3:38 PM Paul Menzel <pmenzel@molgen.mpg.de> wrote: > > >> Thank you for the patch. Wasn’t a similar patch sent to the list already > >> months ago? > >> > >> For the commit message subject/title I suggest: > >> > >> mdadm: Allow to assemble existing arrays with non-POSIX names > > > Thanks for reminding me about this. I forgot about this patch. I can't > > find the patch. Do you still have the link for it? > > I searched for *e2eb503bd797* in the linux-raid archive [1], I found the > patch and discussion *[PATCH] Add the ":" to the allowed_symbols list to > work with the latest POSIX changes* [2]. So, similar, and I think only > the topic of already created arrays came up, your patch solves. Sorry > for the confusion. Thanks for finding it. It doesn't resolve the problem. So this patch tries to resolve the problem. > > >> Am 18.03.25 um 06:46 schrieb Xiao Ni: > >>> It's good to has limitation for name when creating an array. But the arrays > >> > >> … to have limitations … > >> > >>> which were created before patch e2eb503bd797 (mdadm: Follow POSIX Portable > >>> Character Set) can't be assembled. In this patch, it removes the posix > >>> check for assemble mode. > >> > >> “In this patch” is redundant in the commit message. Maybe: > >> > >> So, remove the POSIX check for assemble mode. > > > > good to me. > > > >> Maybe add how to reproduce this? Is there a way to create a non-POSIX > >> name with current mdadm, or should such a file be provided for tests. > > > > For example: > > > > * build mdadm without patch e2eb503bd797 > > * mdadm -CR /dev/md/tbz:pv1 -l0 -n2 /dev/loop0 /dev/loop1 > > * mdadm -Ss > > * build with latest mdadm, and try to assemble it. > > * mdadm -A /dev/md/tbz:pv1 --name tbz:pv1 > > mdadm: Value "tbz:pv1" cannot be set as name. Reason: Not POSIX compatible. > > Thank you. For running tests, re-building mdadm might not be feasible. > But having these instructions would be great to have in the commit > message nevertheless. It's right. I'll add them into the commit message. Regards Xiao > > >>> Fixes: e2eb503bd797 (mdadm: Follow POSIX Portable Character Set) > >>> Signed-off-by: Xiao Ni <xni@redhat.com> > >>> --- > >>> config.c | 8 ++------ > >>> mdadm.c | 11 +++++++++++ > >>> 2 files changed, 13 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/config.c b/config.c > >>> index 8a8ae5e48c41..ef7dbc4eb29f 100644 > >>> --- a/config.c > >>> +++ b/config.c > >>> @@ -208,11 +208,6 @@ static mdadm_status_t ident_check_name(const char *name, const char *prop_name, > >>> return MDADM_STATUS_ERROR; > >>> } > >>> > >>> - if (!is_name_posix_compatible(name)) { > >>> - ident_log(prop_name, name, "Not POSIX compatible", cmdline); > >>> - return MDADM_STATUS_ERROR; > >>> - } > >>> - > >>> return MDADM_STATUS_SUCCESS; > >>> } > >>> > >>> @@ -512,7 +507,8 @@ void arrayline(char *line) > >>> > >>> for (w = dl_next(line); w != line; w = dl_next(w)) { > >>> if (w[0] == '/' || strchr(w, '=') == NULL) { > >>> - _ident_set_devname(&mis, w, false); > >>> + if (is_name_posix_compatible(w)) > >>> + _ident_set_devname(&mis, w, false); > >>> } else if (strncasecmp(w, "uuid=", 5) == 0) { > >>> if (mis.uuid_set) > >>> pr_err("only specify uuid once, %s ignored.\n", > >>> diff --git a/mdadm.c b/mdadm.c > >>> index 6200cd0e7f9b..9d5b0e567799 100644 > >>> --- a/mdadm.c > >>> +++ b/mdadm.c > >>> @@ -732,6 +732,11 @@ int main(int argc, char *argv[]) > >>> exit(2); > >>> } > >>> > >>> + if (mode != ASSEMBLE && !is_name_posix_compatible(optarg)) { > >>> + pr_err("%s Not POSIX compatible\n", optarg); > >>> + exit(2); > >>> + } > >>> + > >>> if (ident_set_name(&ident, optarg) != MDADM_STATUS_SUCCESS) > >>> exit(2); > >>> > >>> @@ -1289,6 +1294,12 @@ int main(int argc, char *argv[]) > >>> pr_err("an md device must be given in this mode\n"); > >>> exit(2); > >>> } > >>> + > >>> + if (mode != ASSEMBLE && !is_name_posix_compatible(devlist->devname)) { > >>> + pr_err("%s Not POSIX compatible\n", devlist->devname); > >>> + exit(2); > >>> + } > >>> + > >>> if (ident_set_devname(&ident, devlist->devname) != MDADM_STATUS_SUCCESS) > >>> exit(1); > > Kind regards, > > Paul > > > [1]: https://lore.kernel.org/linux-raid/?q=e2eb503bd797 > [2]: > https://lore.kernel.org/linux-raid/20241015173553.276546-1-loberman@redhat.com/ >
diff --git a/config.c b/config.c index 8a8ae5e48c41..ef7dbc4eb29f 100644 --- a/config.c +++ b/config.c @@ -208,11 +208,6 @@ static mdadm_status_t ident_check_name(const char *name, const char *prop_name, return MDADM_STATUS_ERROR; } - if (!is_name_posix_compatible(name)) { - ident_log(prop_name, name, "Not POSIX compatible", cmdline); - return MDADM_STATUS_ERROR; - } - return MDADM_STATUS_SUCCESS; } @@ -512,7 +507,8 @@ void arrayline(char *line) for (w = dl_next(line); w != line; w = dl_next(w)) { if (w[0] == '/' || strchr(w, '=') == NULL) { - _ident_set_devname(&mis, w, false); + if (is_name_posix_compatible(w)) + _ident_set_devname(&mis, w, false); } else if (strncasecmp(w, "uuid=", 5) == 0) { if (mis.uuid_set) pr_err("only specify uuid once, %s ignored.\n", diff --git a/mdadm.c b/mdadm.c index 6200cd0e7f9b..9d5b0e567799 100644 --- a/mdadm.c +++ b/mdadm.c @@ -732,6 +732,11 @@ int main(int argc, char *argv[]) exit(2); } + if (mode != ASSEMBLE && !is_name_posix_compatible(optarg)) { + pr_err("%s Not POSIX compatible\n", optarg); + exit(2); + } + if (ident_set_name(&ident, optarg) != MDADM_STATUS_SUCCESS) exit(2); @@ -1289,6 +1294,12 @@ int main(int argc, char *argv[]) pr_err("an md device must be given in this mode\n"); exit(2); } + + if (mode != ASSEMBLE && !is_name_posix_compatible(devlist->devname)) { + pr_err("%s Not POSIX compatible\n", devlist->devname); + exit(2); + } + if (ident_set_devname(&ident, devlist->devname) != MDADM_STATUS_SUCCESS) exit(1);
It's good to has limitation for name when creating an array. But the arrays which were created before patch e2eb503bd797 (mdadm: Follow POSIX Portable Character Set) can't be assembled. In this patch, it removes the posix check for assemble mode. Fixes: e2eb503bd797 (mdadm: Follow POSIX Portable Character Set) Signed-off-by: Xiao Ni <xni@redhat.com> --- config.c | 8 ++------ mdadm.c | 11 +++++++++++ 2 files changed, 13 insertions(+), 6 deletions(-)