| Message ID | 20241015173553.276546-1-loberman@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Add the ":" to the allowed_symbols list to work with the latest POSIX changes | expand |
| Context | Check | Description |
|---|---|---|
| mdraidci/vmtest-md-6_12-PR | fail | merge-conflict |
| mdraidci/vmtest-md-6_13-PR | fail | merge-conflict |
On 10/15/24 1:35 PM, Laurence Oberman wrote: > Signed-off-by: Laurence Oberman <loberman@redhat.com> > --- > lib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib.c b/lib.c > index f36ae03a..cb4b6a0f 100644 > --- a/lib.c > +++ b/lib.c > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char * const name) > { > assert(name); > > - char allowed_symbols[] = "-_."; > + char allowed_symbols[] = "-_.:"; > const char *n = name; > > if (!is_string_lq(name, NAME_MAX)) Reviewed-by: Nigel Croxon <ncroxon@redhat.com>
What are "the latest POSIX changes"?
On Tue, 15 Oct 2024 20:59:44 -0700 Christoph Hellwig <hch@infradead.org> wrote: > What are "the latest POSIX changes"? > > This is mdadm change, it is not kernel. I limited subset of supported symbols in md device name to make it more portable and compatible with udev. If you are interested, here are details: https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=e2eb503bd797908f515b58428b274f1ba6a05349 Thanks, Mariusz
On Wed, Oct 16, 2024 at 09:50:43AM +0200, Mariusz Tkaczyk wrote: > On Tue, 15 Oct 2024 20:59:44 -0700 > Christoph Hellwig <hch@infradead.org> wrote: > > > What are "the latest POSIX changes"? > > > > > > This is mdadm change, it is not kernel. Yes, I've seen that. > I limited subset of supported symbols in > md device name to make it more portable and compatible with udev. So please state that. No matter if kernel or userspace meaningful commit messages are helpful. And I don't know any "POSIX changes" about "allowed characters". > > If you are interested, here are details: > https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=e2eb503bd797908f515b58428b274f1ba6a05349 That is a significant better commit log, and it would be helpful if this patch was as careful about wording and also referenced the above commit.
On Tue, 15 Oct 2024 13:35:24 -0400 Laurence Oberman <loberman@redhat.com> wrote: Hello Laurence, Thanks for the patch. ":" is used internally by native metadata name, we have "hostname:name". We are searching for it specifically, for that reason I think that I cannot accept it. Name must stay simple. If you want this again, I need to full set of mdadm tests that is covering every scenario and is confirming that we are able to determine hostname (if exists) and name in every case correctly. There are some workaround in code for that, I can see that we are not appending homehost if ":" is in the name. It is not user friendly. I prefer to not allow ":" to keep in simpler unless you have really good reason to have it back - there is no reason in commit message. > Signed-off-by: Laurence Oberman <loberman@redhat.com> > --- > lib.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib.c b/lib.c > index f36ae03a..cb4b6a0f 100644 > --- a/lib.c > +++ b/lib.c > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char * const name) > { > assert(name); > > - char allowed_symbols[] = "-_."; > + char allowed_symbols[] = "-_.:"; Because the function has been made to follow POSIX, this cannot be simply added here. Please at least explain that in description. It is not POSIX compatible with this change. > const char *n = name; > > if (!is_string_lq(name, NAME_MAX)) Thanks, Mariusz
On Wed, 2024-10-16 at 10:14 +0200, Mariusz Tkaczyk wrote: > On Tue, 15 Oct 2024 13:35:24 -0400 > Laurence Oberman <loberman@redhat.com> wrote: > > Hello Laurence, > Thanks for the patch. > > ":" is used internally by native metadata name, we have > "hostname:name". We are > searching for it specifically, for that reason I think that I cannot > accept it. > Name must stay simple. > > If you want this again, I need to full set of mdadm tests that is > covering every > scenario and is confirming that we are able to determine hostname (if > exists) > and name in every case correctly. > > There are some workaround in code for that, I can see that we are not > appending > homehost if ":" is in the name. It is not user friendly. I prefer to > not > allow ":" to keep in simpler unless you have really good reason to > have it back > - there is no reason in commit message. > > > > Signed-off-by: Laurence Oberman <loberman@redhat.com> > > --- > > lib.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib.c b/lib.c > > index f36ae03a..cb4b6a0f 100644 > > --- a/lib.c > > +++ b/lib.c > > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char * > > const name) > > { > > assert(name); > > > > - char allowed_symbols[] = "-_."; > > + char allowed_symbols[] = "-_.:"; > > Because the function has been made to follow POSIX, this cannot be > simply added > here. Please at least explain that in description. > > It is not POSIX compatible with this change. > > > > const char *n = name; > > > > if (!is_string_lq(name, NAME_MAX)) > > Thanks, > Mariusz > Hello Apologies Christoph and Mariusz, this could have definitely done with more of an explanation. We have customers complaining about a regression in mdadm since these changes happened. They have 1000's of raid devices that can no longer be started because they have ":" in the name. Example mdadm: Value "tbz:pv1" cannot be set as devname. Reason: Not POSIX compatible. Should we add a --legacy flag where the original behavior is still an option, what are your thoughts ? Regards Laurence
On Wed, Oct 16, 2024 at 9:11 PM Laurence Oberman <loberman@redhat.com> wrote: > > On Wed, 2024-10-16 at 10:14 +0200, Mariusz Tkaczyk wrote: > > On Tue, 15 Oct 2024 13:35:24 -0400 > > Laurence Oberman <loberman@redhat.com> wrote: > > > > Hello Laurence, > > Thanks for the patch. > > > > ":" is used internally by native metadata name, we have > > "hostname:name". We are > > searching for it specifically, for that reason I think that I cannot > > accept it. > > Name must stay simple. > > > > If you want this again, I need to full set of mdadm tests that is > > covering every > > scenario and is confirming that we are able to determine hostname (if > > exists) > > and name in every case correctly. > > > > There are some workaround in code for that, I can see that we are not > > appending > > homehost if ":" is in the name. It is not user friendly. I prefer to > > not > > allow ":" to keep in simpler unless you have really good reason to > > have it back > > - there is no reason in commit message. > > > > > > > Signed-off-by: Laurence Oberman <loberman@redhat.com> > > > --- > > > lib.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib.c b/lib.c > > > index f36ae03a..cb4b6a0f 100644 > > > --- a/lib.c > > > +++ b/lib.c > > > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char * > > > const name) > > > { > > > assert(name); > > > > > > - char allowed_symbols[] = "-_."; > > > + char allowed_symbols[] = "-_.:"; > > > > Because the function has been made to follow POSIX, this cannot be > > simply added > > here. Please at least explain that in description. > > > > It is not POSIX compatible with this change. > > > > > > > const char *n = name; > > > > > > if (!is_string_lq(name, NAME_MAX)) > > > > Thanks, > > Mariusz > > > > Hello > Apologies Christoph and Mariusz, this could have definitely done with > more of an explanation. > > We have customers complaining about a regression in mdadm since these > changes happened. They have 1000's of raid devices that can no longer > be started because they have ":" in the name. Example > mdadm: Value "tbz:pv1" cannot be set as devname. Reason: Not POSIX > compatible. > > Should we add a --legacy flag where the original behavior is still an > option, what are your thoughts ? > > Regards > Laurence > > Hi Laurence and Mariusz Can we allow assemble an array whose devname has ":"? So the users can assemble the arrays which were created before patch e2eb503bd797 (mdadm: Follow POSIX Portable Character Set). For creating an array, we can still follow the POSIX policy. So it can keep the naming rule simpler. Best Regards Xiao
On Thu, 17 Oct 2024 17:24:59 +0800 Xiao Ni <xni@redhat.com> wrote: > On Wed, Oct 16, 2024 at 9:11 PM Laurence Oberman <loberman@redhat.com> wrote: > > > > On Wed, 2024-10-16 at 10:14 +0200, Mariusz Tkaczyk wrote: > > > On Tue, 15 Oct 2024 13:35:24 -0400 > > > Laurence Oberman <loberman@redhat.com> wrote: > > > > > > Hello Laurence, > > > Thanks for the patch. > > > > > > ":" is used internally by native metadata name, we have > > > "hostname:name". We are > > > searching for it specifically, for that reason I think that I cannot > > > accept it. > > > Name must stay simple. > > > > > > If you want this again, I need to full set of mdadm tests that is > > > covering every > > > scenario and is confirming that we are able to determine hostname (if > > > exists) > > > and name in every case correctly. > > > > > > There are some workaround in code for that, I can see that we are not > > > appending > > > homehost if ":" is in the name. It is not user friendly. I prefer to > > > not > > > allow ":" to keep in simpler unless you have really good reason to > > > have it back > > > - there is no reason in commit message. > > > > > > > > > > Signed-off-by: Laurence Oberman <loberman@redhat.com> > > > > --- > > > > lib.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/lib.c b/lib.c > > > > index f36ae03a..cb4b6a0f 100644 > > > > --- a/lib.c > > > > +++ b/lib.c > > > > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char * > > > > const name) > > > > { > > > > assert(name); > > > > > > > > - char allowed_symbols[] = "-_."; > > > > + char allowed_symbols[] = "-_.:"; > > > > > > Because the function has been made to follow POSIX, this cannot be > > > simply added > > > here. Please at least explain that in description. > > > > > > It is not POSIX compatible with this change. > > > > > > > > > > const char *n = name; > > > > > > > > if (!is_string_lq(name, NAME_MAX)) > > > > > > Thanks, > > > Mariusz > > > > > > > Hello > > Apologies Christoph and Mariusz, this could have definitely done with > > more of an explanation. > > > > We have customers complaining about a regression in mdadm since these > > changes happened. They have 1000's of raid devices that can no longer > > be started because they have ":" in the name. Example > > mdadm: Value "tbz:pv1" cannot be set as devname. Reason: Not POSIX > > compatible. > > > > Should we add a --legacy flag where the original behavior is still an > > option, what are your thoughts ? > > > > Regards > > Laurence > > > > > > Hi Laurence and Mariusz > > Can we allow assemble an array whose devname has ":"? So the users can > assemble the arrays which were created before patch e2eb503bd797 > (mdadm: Follow POSIX Portable Character Set). > > For creating an array, we can still follow the POSIX policy. So it can > keep the naming rule simpler. > Thanks Xiao, Please give me some time to reproduce and understand this case. I will find some time on monday, maybe tomorrow. I hope that works for you guys. Mariusz
On Thu, 2024-10-17 at 13:26 +0200, Mariusz Tkaczyk wrote: > On Thu, 17 Oct 2024 17:24:59 +0800 > Xiao Ni <xni@redhat.com> wrote: > > > On Wed, Oct 16, 2024 at 9:11 PM Laurence Oberman > > <loberman@redhat.com> wrote: > > > > > > On Wed, 2024-10-16 at 10:14 +0200, Mariusz Tkaczyk wrote: > > > > On Tue, 15 Oct 2024 13:35:24 -0400 > > > > Laurence Oberman <loberman@redhat.com> wrote: > > > > > > > > Hello Laurence, > > > > Thanks for the patch. > > > > > > > > ":" is used internally by native metadata name, we have > > > > "hostname:name". We are > > > > searching for it specifically, for that reason I think that I > > > > cannot > > > > accept it. > > > > Name must stay simple. > > > > > > > > If you want this again, I need to full set of mdadm tests that > > > > is > > > > covering every > > > > scenario and is confirming that we are able to determine > > > > hostname (if > > > > exists) > > > > and name in every case correctly. > > > > > > > > There are some workaround in code for that, I can see that we > > > > are not > > > > appending > > > > homehost if ":" is in the name. It is not user friendly. I > > > > prefer to > > > > not > > > > allow ":" to keep in simpler unless you have really good reason > > > > to > > > > have it back > > > > - there is no reason in commit message. > > > > > > > > > > > > > Signed-off-by: Laurence Oberman <loberman@redhat.com> > > > > > --- > > > > > lib.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/lib.c b/lib.c > > > > > index f36ae03a..cb4b6a0f 100644 > > > > > --- a/lib.c > > > > > +++ b/lib.c > > > > > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char > > > > > * > > > > > const name) > > > > > { > > > > > assert(name); > > > > > > > > > > - char allowed_symbols[] = "-_."; > > > > > + char allowed_symbols[] = "-_.:"; > > > > > > > > Because the function has been made to follow POSIX, this cannot > > > > be > > > > simply added > > > > here. Please at least explain that in description. > > > > > > > > It is not POSIX compatible with this change. > > > > > > > > > > > > > const char *n = name; > > > > > > > > > > if (!is_string_lq(name, NAME_MAX)) > > > > > > > > Thanks, > > > > Mariusz > > > > > > > > > > Hello > > > Apologies Christoph and Mariusz, this could have definitely done > > > with > > > more of an explanation. > > > > > > We have customers complaining about a regression in mdadm since > > > these > > > changes happened. They have 1000's of raid devices that can no > > > longer > > > be started because they have ":" in the name. Example > > > mdadm: Value "tbz:pv1" cannot be set as devname. Reason: Not > > > POSIX > > > compatible. > > > > > > Should we add a --legacy flag where the original behavior is > > > still an > > > option, what are your thoughts ? > > > > > > Regards > > > Laurence > > > > > > > > > > Hi Laurence and Mariusz > > > > Can we allow assemble an array whose devname has ":"? So the users > > can > > assemble the arrays which were created before patch e2eb503bd797 > > (mdadm: Follow POSIX Portable Character Set). > > > > For creating an array, we can still follow the POSIX policy. So it > > can > > keep the naming rule simpler. > > > > Thanks Xiao, > > Please give me some time to reproduce and understand this case. I > will find some > time on monday, maybe tomorrow. > > I hope that works for you guys. > > Mariusz > Hello Yes, Thank you, if we can have the option to start already created arrays but make customers aware of the POSIX limitation in creating new arrays that would be fine with me. Regards Laurence
On Wed, Oct 16, 2024 at 09:10:44AM -0400, Laurence Oberman wrote: > Apologies Christoph and Mariusz, this could have definitely done with > more of an explanation. > > We have customers complaining about a regression in mdadm since these > changes happened. They have 1000's of raid devices that can no longer > be started because they have ":" in the name. Example > mdadm: Value "tbz:pv1" cannot be set as devname. Reason: Not POSIX > compatible. > > Should we add a --legacy flag where the original behavior is still an > option, what are your thoughts ? I don't really care either way, just please write an understandable commit message.
On Thu, 2024-10-17 at 08:23 -0400, Laurence Oberman wrote: > On Thu, 2024-10-17 at 13:26 +0200, Mariusz Tkaczyk wrote: > > On Thu, 17 Oct 2024 17:24:59 +0800 > > Xiao Ni <xni@redhat.com> wrote: > > > > > On Wed, Oct 16, 2024 at 9:11 PM Laurence Oberman > > > <loberman@redhat.com> wrote: > > > > > > > > On Wed, 2024-10-16 at 10:14 +0200, Mariusz Tkaczyk wrote: > > > > > On Tue, 15 Oct 2024 13:35:24 -0400 > > > > > Laurence Oberman <loberman@redhat.com> wrote: > > > > > > > > > > Hello Laurence, > > > > > Thanks for the patch. > > > > > > > > > > ":" is used internally by native metadata name, we have > > > > > "hostname:name". We are > > > > > searching for it specifically, for that reason I think that I > > > > > cannot > > > > > accept it. > > > > > Name must stay simple. > > > > > > > > > > If you want this again, I need to full set of mdadm tests > > > > > that > > > > > is > > > > > covering every > > > > > scenario and is confirming that we are able to determine > > > > > hostname (if > > > > > exists) > > > > > and name in every case correctly. > > > > > > > > > > There are some workaround in code for that, I can see that we > > > > > are not > > > > > appending > > > > > homehost if ":" is in the name. It is not user friendly. I > > > > > prefer to > > > > > not > > > > > allow ":" to keep in simpler unless you have really good > > > > > reason > > > > > to > > > > > have it back > > > > > - there is no reason in commit message. > > > > > > > > > > > > > > > > Signed-off-by: Laurence Oberman <loberman@redhat.com> > > > > > > --- > > > > > > lib.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/lib.c b/lib.c > > > > > > index f36ae03a..cb4b6a0f 100644 > > > > > > --- a/lib.c > > > > > > +++ b/lib.c > > > > > > @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const > > > > > > char > > > > > > * > > > > > > const name) > > > > > > { > > > > > > assert(name); > > > > > > > > > > > > - char allowed_symbols[] = "-_."; > > > > > > + char allowed_symbols[] = "-_.:"; > > > > > > > > > > Because the function has been made to follow POSIX, this > > > > > cannot > > > > > be > > > > > simply added > > > > > here. Please at least explain that in description. > > > > > > > > > > It is not POSIX compatible with this change. > > > > > > > > > > > > > > > > const char *n = name; > > > > > > > > > > > > if (!is_string_lq(name, NAME_MAX)) > > > > > > > > > > Thanks, > > > > > Mariusz > > > > > > > > > > > > > Hello > > > > Apologies Christoph and Mariusz, this could have definitely > > > > done > > > > with > > > > more of an explanation. > > > > > > > > We have customers complaining about a regression in mdadm since > > > > these > > > > changes happened. They have 1000's of raid devices that can no > > > > longer > > > > be started because they have ":" in the name. Example > > > > mdadm: Value "tbz:pv1" cannot be set as devname. Reason: Not > > > > POSIX > > > > compatible. > > > > > > > > Should we add a --legacy flag where the original behavior is > > > > still an > > > > option, what are your thoughts ? > > > > > > > > Regards > > > > Laurence > > > > > > > > > > > > > > Hi Laurence and Mariusz > > > > > > Can we allow assemble an array whose devname has ":"? So the > > > users > > > can > > > assemble the arrays which were created before patch e2eb503bd797 > > > (mdadm: Follow POSIX Portable Character Set). > > > > > > For creating an array, we can still follow the POSIX policy. So > > > it > > > can > > > keep the naming rule simpler. > > > > > > > Thanks Xiao, > > > > Please give me some time to reproduce and understand this case. I > > will find some > > time on monday, maybe tomorrow. > > > > I hope that works for you guys. > > > > Mariusz > > > > Hello > > Yes, Thank you, if we can have the option to start already created > arrays but make customers aware of the POSIX limitation in creating > new > arrays that would be fine with me. > > Regards > Laurence > Hello Mariusz and Xiao I re-ran all the tests in house and I can actually start the arrays that have the ":" in the names, just not create new ones. So I think we leave this as is and we keep the adherence to POSIX for newly created arrays only. My patch or request for a change for creation can be denied, its fine. Regards Laurence
On Thu, Oct 17, 2024 at 11:27:24AM -0400, Laurence Oberman wrote: > I re-ran all the tests in house and I can actually start the arrays > that have the ":" in the names, just not create new ones. So I think we > leave this as is and we keep the adherence to POSIX for newly created > arrays only. Can we please stop talking about Posix compliance here? The POSIX Portable Character Set is the minimum set of characters that need to be supported in each character set for Posix compliance. Including other characters is perfectly Posix complain, just not fully portable.
On Thu, 17 Oct 2024 08:31:17 -0700 "Hellwig, Christoph" <hch@infradead.org> wrote: > On Thu, Oct 17, 2024 at 11:27:24AM -0400, Laurence Oberman wrote: > > I re-ran all the tests in house and I can actually start the arrays > > that have the ":" in the names, just not create new ones. So I think we > > leave this as is and we keep the adherence to POSIX for newly created > > arrays only. > > Can we please stop talking about Posix compliance here? > > The POSIX Portable Character Set is the minimum set of characters that > need to be supported in each character set for Posix compliance. > > Including other characters is perfectly Posix complain, just not fully > portable. > Right, good one. I named it just POSIX for simplicity and now we are abusing it. Cool, the main reasons ":" is excluded is special meaning for native metadata and I still think it is right change. They can use --homehost option if they want to customize it, it should be perfectly valid: # mdadm -CR vol0 --homehost myhost -l0 -n2 /dev/nvme[45]n1 -z5G --assume-clean # mdadm -E /dev/nvme4n1 | grep Name Name : myhost:vol0 Isn't result the same? I still need to look into assemble issue to understand why it is not working. Maybe there is small functional difference I'm not familiar with. Thanks, Mariusz
diff --git a/lib.c b/lib.c index f36ae03a..cb4b6a0f 100644 --- a/lib.c +++ b/lib.c @@ -485,7 +485,7 @@ bool is_name_posix_compatible(const char * const name) { assert(name); - char allowed_symbols[] = "-_."; + char allowed_symbols[] = "-_.:"; const char *n = name; if (!is_string_lq(name, NAME_MAX))
Signed-off-by: Laurence Oberman <loberman@redhat.com> --- lib.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)