linux-raid.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
@ 2024-10-15 17:35 Laurence Oberman
  2024-10-15 17:41 ` Nigel Croxon
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Laurence Oberman @ 2024-10-15 17:35 UTC (permalink / raw)
  To: linux-raid, loberman

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))
-- 
2.42.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-15 17:35 [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes Laurence Oberman
@ 2024-10-15 17:41 ` Nigel Croxon
  2024-10-16  3:59 ` Christoph Hellwig
  2024-10-16  8:14 ` Mariusz Tkaczyk
  2 siblings, 0 replies; 14+ messages in thread
From: Nigel Croxon @ 2024-10-15 17:41 UTC (permalink / raw)
  To: Laurence Oberman, linux-raid


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>



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-15 17:35 [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes Laurence Oberman
  2024-10-15 17:41 ` Nigel Croxon
@ 2024-10-16  3:59 ` Christoph Hellwig
  2024-10-16  7:50   ` Mariusz Tkaczyk
  2024-10-16  8:14 ` Mariusz Tkaczyk
  2 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2024-10-16  3:59 UTC (permalink / raw)
  To: Laurence Oberman; +Cc: linux-raid

What are "the latest POSIX changes"?


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-16  3:59 ` Christoph Hellwig
@ 2024-10-16  7:50   ` Mariusz Tkaczyk
  2024-10-16  7:53     ` Christoph Hellwig
  0 siblings, 1 reply; 14+ messages in thread
From: Mariusz Tkaczyk @ 2024-10-16  7:50 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: Laurence Oberman, linux-raid

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-16  7:50   ` Mariusz Tkaczyk
@ 2024-10-16  7:53     ` Christoph Hellwig
  0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2024-10-16  7:53 UTC (permalink / raw)
  To: Mariusz Tkaczyk; +Cc: Christoph Hellwig, Laurence Oberman, linux-raid

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-15 17:35 [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes Laurence Oberman
  2024-10-15 17:41 ` Nigel Croxon
  2024-10-16  3:59 ` Christoph Hellwig
@ 2024-10-16  8:14 ` Mariusz Tkaczyk
  2024-10-16 13:10   ` Laurence Oberman
  2 siblings, 1 reply; 14+ messages in thread
From: Mariusz Tkaczyk @ 2024-10-16  8:14 UTC (permalink / raw)
  To: Laurence Oberman; +Cc: linux-raid

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-16  8:14 ` Mariusz Tkaczyk
@ 2024-10-16 13:10   ` Laurence Oberman
  2024-10-17  9:24     ` Xiao Ni
  2024-10-17 15:04     ` Hellwig, Christoph
  0 siblings, 2 replies; 14+ messages in thread
From: Laurence Oberman @ 2024-10-16 13:10 UTC (permalink / raw)
  To: Mariusz Tkaczyk, Hellwig, Christoph; +Cc: linux-raid

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 


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-16 13:10   ` Laurence Oberman
@ 2024-10-17  9:24     ` Xiao Ni
  2024-10-17 11:26       ` Mariusz Tkaczyk
  2024-10-17 15:04     ` Hellwig, Christoph
  1 sibling, 1 reply; 14+ messages in thread
From: Xiao Ni @ 2024-10-17  9:24 UTC (permalink / raw)
  To: Laurence Oberman; +Cc: Mariusz Tkaczyk, Hellwig, Christoph, linux-raid

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-17  9:24     ` Xiao Ni
@ 2024-10-17 11:26       ` Mariusz Tkaczyk
  2024-10-17 12:23         ` Laurence Oberman
  0 siblings, 1 reply; 14+ messages in thread
From: Mariusz Tkaczyk @ 2024-10-17 11:26 UTC (permalink / raw)
  To: Xiao Ni; +Cc: Laurence Oberman, Hellwig, Christoph, linux-raid

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-17 11:26       ` Mariusz Tkaczyk
@ 2024-10-17 12:23         ` Laurence Oberman
  2024-10-17 15:27           ` Laurence Oberman
  0 siblings, 1 reply; 14+ messages in thread
From: Laurence Oberman @ 2024-10-17 12:23 UTC (permalink / raw)
  To: Mariusz Tkaczyk, Xiao Ni; +Cc: Hellwig, Christoph, linux-raid

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-16 13:10   ` Laurence Oberman
  2024-10-17  9:24     ` Xiao Ni
@ 2024-10-17 15:04     ` Hellwig, Christoph
  1 sibling, 0 replies; 14+ messages in thread
From: Hellwig, Christoph @ 2024-10-17 15:04 UTC (permalink / raw)
  To: Laurence Oberman; +Cc: Mariusz Tkaczyk, Hellwig, Christoph, linux-raid

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.  


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-17 12:23         ` Laurence Oberman
@ 2024-10-17 15:27           ` Laurence Oberman
  2024-10-17 15:31             ` Hellwig, Christoph
  0 siblings, 1 reply; 14+ messages in thread
From: Laurence Oberman @ 2024-10-17 15:27 UTC (permalink / raw)
  To: Mariusz Tkaczyk, Xiao Ni; +Cc: Hellwig, Christoph, linux-raid

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-17 15:27           ` Laurence Oberman
@ 2024-10-17 15:31             ` Hellwig, Christoph
  2024-10-18  6:28               ` Mariusz Tkaczyk
  0 siblings, 1 reply; 14+ messages in thread
From: Hellwig, Christoph @ 2024-10-17 15:31 UTC (permalink / raw)
  To: Laurence Oberman; +Cc: Mariusz Tkaczyk, Xiao Ni, Hellwig, Christoph, linux-raid

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.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes
  2024-10-17 15:31             ` Hellwig, Christoph
@ 2024-10-18  6:28               ` Mariusz Tkaczyk
  0 siblings, 0 replies; 14+ messages in thread
From: Mariusz Tkaczyk @ 2024-10-18  6:28 UTC (permalink / raw)
  To: Hellwig, Christoph; +Cc: Laurence Oberman, Xiao Ni, linux-raid

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2024-10-18  6:28 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-15 17:35 [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes Laurence Oberman
2024-10-15 17:41 ` Nigel Croxon
2024-10-16  3:59 ` Christoph Hellwig
2024-10-16  7:50   ` Mariusz Tkaczyk
2024-10-16  7:53     ` Christoph Hellwig
2024-10-16  8:14 ` Mariusz Tkaczyk
2024-10-16 13:10   ` Laurence Oberman
2024-10-17  9:24     ` Xiao Ni
2024-10-17 11:26       ` Mariusz Tkaczyk
2024-10-17 12:23         ` Laurence Oberman
2024-10-17 15:27           ` Laurence Oberman
2024-10-17 15:31             ` Hellwig, Christoph
2024-10-18  6:28               ` Mariusz Tkaczyk
2024-10-17 15:04     ` Hellwig, Christoph

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).