Re: [PATCH] Add the ":" to the allowed_symbols list to work with the latest POSIX changes

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

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