Syslinux - Jun 2015 - [PATCH] chrreplace: Don't skip the first character

> Check if the first character matches the character to replace, rather
> than skipping it and starting with the second.
> 
> Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> ---
> 
> I'm assuming, based on a look at the callers, that this is not
> intentional, and that it just happened that none of the callers happened
> to ever need to replace the first character.
> 
>  com32/lib/chrreplace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
> index 65786f9..cfbf5d4 100644
> --- a/com32/lib/chrreplace.c
> +++ b/com32/lib/chrreplace.c
> @@ -4,8 +4,8 @@
>  void chrreplace(char *source, char old, char new) 
>  {
>      while (*source) { 
> -	source++;
>  	if (source[0] == old) source[0]=new;
> +	source++;
>      }
>  }
>  
> -- 
> 2.1.4
> 
 
May I ask, where is this code having some effect?

Is there some way to trigger certain issue / effect / behavior with the 
code as in version 6.03 that would be corrected / improved by this 
patch?

Which would be the context / setup / config where some issue related to 
this code could be triggered?

I am not criticizing the patch. I just would like to know, in the eyes 
of a final user, what / where the effect of the code as found in v.6.03 
could be seen, and the effect of this patch.

TIA,
Ady.

On Mon, Jun 15, 2015 at 08:57:24PM +0300, Ady wrote:
> 
> > Check if the first character matches the character to replace, rather
> > than skipping it and starting with the second.
> > 
> > Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> > ---
> > 
> > I'm assuming, based on a look at the callers, that this is not
> > intentional, and that it just happened that none of the callers
happened
> > to ever need to replace the first character.
> > 
> >  com32/lib/chrreplace.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
> > index 65786f9..cfbf5d4 100644
> > --- a/com32/lib/chrreplace.c
> > +++ b/com32/lib/chrreplace.c
> > @@ -4,8 +4,8 @@
> >  void chrreplace(char *source, char old, char new) 
> >  {
> >      while (*source) { 
> > -	source++;
> >  	if (source[0] == old) source[0]=new;
> > +	source++;
> >      }
> >  }
> >  
> > -- 
> > 2.1.4
> > 
>  
> May I ask, where is this code having some effect?
> 
> Is there some way to trigger certain issue / effect / behavior with the 
> code as in version 6.03 that would be corrected / improved by this 
> patch?
> 
> Which would be the context / setup / config where some issue related to 
> this code could be triggered?
> 
> I am not criticizing the patch. I just would like to know, in the eyes 
> of a final user, what / where the effect of the code as found in v.6.03 
> could be seen, and the effect of this patch.
To the best of my knowledge, this doesn't cause any user-visible bug in
syslinux; as mentioned above, all the callers happen to never need to
replace the first character.  However, it's also very surprising and
undocumented behavior; I only found it while digging through the
available com32 libraries to find out which memory and string functions
were available.  If this behavior is intentional, it needs a pile of
documentation and a good explanation for why skipping the first
character is useful.

- Josh Triplett

> On Mon, Jun 15, 2015 at 08:57:24PM +0300, Ady wrote:
> > 
> > > Check if the first character matches the character to replace,
rather
> > > than skipping it and starting with the second.
> > > 
> > > Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> > > ---
> > > 
> > > I'm assuming, based on a look at the callers, that this is
not
> > > intentional, and that it just happened that none of the callers
happened
> > > to ever need to replace the first character.
> > > 
> > >  com32/lib/chrreplace.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/com32/lib/chrreplace.c b/com32/lib/chrreplace.c
> > > index 65786f9..cfbf5d4 100644
> > > --- a/com32/lib/chrreplace.c
> > > +++ b/com32/lib/chrreplace.c
> > > @@ -4,8 +4,8 @@
> > >  void chrreplace(char *source, char old, char new) 
> > >  {
> > >      while (*source) { 
> > > -	source++;
> > >  	if (source[0] == old) source[0]=new;
> > > +	source++;
> > >      }
> > >  }
> > >  
> > > -- 
> > > 2.1.4
> > > 
> >  
> > May I ask, where is this code having some effect?
> > 
> > Is there some way to trigger certain issue / effect / behavior with
the
> > code as in version 6.03 that would be corrected / improved by this 
> > patch?
> > 
> > Which would be the context / setup / config where some issue related
to
> > this code could be triggered?
> > 
> > I am not criticizing the patch. I just would like to know, in the eyes
> > of a final user, what / where the effect of the code as found in
v.6.03
> > could be seen, and the effect of this patch.
> 
> To the best of my knowledge, this doesn't cause any user-visible bug in
> syslinux; as mentioned above, all the callers happen to never need to
> replace the first character.  However, it's also very surprising and
> undocumented behavior; I only found it while digging through the
> available com32 libraries to find out which memory and string functions
> were available.  If this behavior is intentional, it needs a pile of
> documentation and a good explanation for why skipping the first
> character is useful.
> 
> - Josh Triplett
 
I have no idea where the code is being actually used. Let's imagine for 
a moment that it is used in PXELINUX, to search for a configuration 
filename that (partially) matches an "IP address as upper case 
hexadecimal" syntax:

pxelinux.cfg/C000025B
pxelinux.cfg/C000025
pxelinux.cfg/C00002
pxelinux.cfg/C0000
pxelinux.cfg/C000
pxelinux.cfg/C00
pxelinux.cfg/C0
pxelinux.cfg/C
pxelinux.cfg/default

If the current code iterates the search all the way until the first 
character ("C" in the above example) included, then I guess you would 
not want to waste any more time in some additional iteration, as there 
is no additional search to perform (i.e. move on to search for 
"default").

I repeat, I have no idea where the code is actually being called / 
used. I am just throwing here some hypothetical case.

So, my questions are:
_ Is the current code generating the correct effect / behavior 
(wherever this code is being called)?
_ Is the current code effective and efficient?
_ Are the effects of this patch actually improving the behavior / 
efficiency?

I don't know the answers to these questions. I am just presenting them, 
so this patch can be considered / evaluated by Syslinux developers.

TIA,
Ady.