Fix glob() function

  Hello,

While porting 'mandoc' [1] to my hobbyOS FiwixOS, I've discovered that the
glob() function in Newlib always returns zero (success) even when the
pathname was not found.

I was comparing the file 'libc/posix/glob.c' with the one from Apple [2] 
and then I applied the following patch:




The following is a simple test program:

# cat glob.c
#include <stdio.h>
#include <glob.h>

int main(void)
{
 	int globres;
 	glob_t globinfo;
 	char *ok1 = "/usr/share/man/man1/ls.[01-9]*";
 	char *ko1 = "/usr/share/man/man1/lsx.[01-9]*";
 	char *ok2 = "glob.c";
 	char *ko2 = "glob.x";

 	globres = glob(ok1, 0, NULL, &globinfo);
 	printf("globres = %d (%s)\n", globres, ok1);
 	globres = glob(ko1, 0, NULL, &globinfo);
 	printf("globres = %d (%s)\n", globres, ko1);

 	globres = glob(ok2, 0, NULL, &globinfo);
 	printf("globres = %d (%s)\n", globres, ok2);
 	globres = glob(ko2, 0, NULL, &globinfo);
 	printf("globres = %d (%s)\n", globres, ko2);

 	return 0;
}


The results before applying the patch were:

# ./glob
globres = 0 (/usr/share/man/man1/ls.[01-9]*)
globres = 0 (/usr/share/man/man1/lsx.[01-9]*)
globres = 0 (glob.c)
globres = 0 (glob.x)


The results after applying the patch are:
# ./glob
globres = 0 (/usr/share/man/man1/ls.[01-9]*)
globres = 3 (/usr/share/man/man1/lsx.[01-9]*)
globres = 0 (glob.c)
globres = 3 (glob.x)


Thanks.

[1] https://mandoc.bsd.lv/
[2] 
https://opensource.apple.com/source/Libinfo/Libinfo-129/util.subproj/glob.c

--
Jordi Sanfeliu
FIBRANET Network Services Provider
https://www.fibranet.cat
  

Hi Jordi,

On Aug 17 17:38, Jordi Sanfeliu wrote:
> Hello,
> 
> While porting 'mandoc' [1] to my hobbyOS FiwixOS, I've discovered that the
> glob() function in Newlib always returns zero (success) even when the
> pathname was not found.
> 
> I was comparing the file 'libc/posix/glob.c' with the one from Apple [2] and
> then I applied the following patch:
> 
> diff --git a/newlib/libc/posix/glob.c b/newlib/libc/posix/glob.c
> index 5e6c2fcba..0347979de 100644
> --- a/newlib/libc/posix/glob.c
> +++ b/newlib/libc/posix/glob.c
> @@ -502,11 +502,14 @@ glob0(pattern, pglob, limit)
>   	 * and the pattern did not contain any magic characters
>   	 * GLOB_NOMAGIC is there just for compatibility with csh.
>   	 */
> -	if (pglob->gl_pathc == oldpathc &&
> -	    ((pglob->gl_flags & GLOB_NOCHECK) ||
> -	      ((pglob->gl_flags & GLOB_NOMAGIC) &&
> -	       !(pglob->gl_flags & GLOB_MAGCHAR))))
> -		return(globextend(pattern, pglob, limit));
> +	if (pglob->gl_pathc == oldpathc) {
> +	    if (((pglob->gl_flags & GLOB_NOCHECK) ||
> +	        ((pglob->gl_flags & GLOB_NOMAGIC) &&
> +	        !(pglob->gl_flags & GLOB_MAGCHAR))))
> +		    return(globextend(pattern, pglob, limit));
> +	    else

This is correct and as upstream, but...

> +		    return(3);	/* GLOB_NOMATCH */

...this isn't.  Check with libc/include/glob.h.  Your patch should
add a

  #define GLOB_NOMATCH (-3)

to glob.h instead.

Also, check your whitespaces.  I'm getting quite a few warnings from
`git am':

  Applying: Fix glob() function
  .git/rebase-apply/patch:6: space before tab in indent.
	   * and the pattern did not contain any magic characters
  .git/rebase-apply/patch:7: space before tab in indent.
	   * GLOB_NOMAGIC is there just for compatibility with csh.
  .git/rebase-apply/patch:8: space before tab in indent.
	   */
  .git/rebase-apply/patch:22: space before tab in indent.
	  else if (!(pglob->gl_flags & GLOB_NOSORT))
  .git/rebase-apply/patch:23: space before tab in indent.
		  qsort(pglob->gl_pathv + pglob->gl_offs + oldpathc,
  warning: squelched 1 whitespace error
  warning: 6 lines add whitespace errors.


Thanks,
Corinna
  

Find the patch attached, I hope is OK this time.
Thanks.

--
Jordi Sanfeliu
FIBRANET Network Services Provider
https://www.fibranet.cat


On Mon, 19 Aug 2024, Corinna Vinschen wrote:

> Hi Jordi,
>
> On Aug 17 17:38, Jordi Sanfeliu wrote:
>> Hello,
>>
>> While porting 'mandoc' [1] to my hobbyOS FiwixOS, I've discovered that the
>> glob() function in Newlib always returns zero (success) even when the
>> pathname was not found.
>>
>> I was comparing the file 'libc/posix/glob.c' with the one from Apple [2] and
>> then I applied the following patch:
>>
>> diff --git a/newlib/libc/posix/glob.c b/newlib/libc/posix/glob.c
>> index 5e6c2fcba..0347979de 100644
>> --- a/newlib/libc/posix/glob.c
>> +++ b/newlib/libc/posix/glob.c
>> @@ -502,11 +502,14 @@ glob0(pattern, pglob, limit)
>>   	 * and the pattern did not contain any magic characters
>>   	 * GLOB_NOMAGIC is there just for compatibility with csh.
>>   	 */
>> -	if (pglob->gl_pathc == oldpathc &&
>> -	    ((pglob->gl_flags & GLOB_NOCHECK) ||
>> -	      ((pglob->gl_flags & GLOB_NOMAGIC) &&
>> -	       !(pglob->gl_flags & GLOB_MAGCHAR))))
>> -		return(globextend(pattern, pglob, limit));
>> +	if (pglob->gl_pathc == oldpathc) {
>> +	    if (((pglob->gl_flags & GLOB_NOCHECK) ||
>> +	        ((pglob->gl_flags & GLOB_NOMAGIC) &&
>> +	        !(pglob->gl_flags & GLOB_MAGCHAR))))
>> +		    return(globextend(pattern, pglob, limit));
>> +	    else
>
> This is correct and as upstream, but...
>
>> +		    return(3);	/* GLOB_NOMATCH */
>
> ...this isn't.  Check with libc/include/glob.h.  Your patch should
> add a
>
>  #define GLOB_NOMATCH (-3)
>
> to glob.h instead.
>
> Also, check your whitespaces.  I'm getting quite a few warnings from
> `git am':
>
>  Applying: Fix glob() function
>  .git/rebase-apply/patch:6: space before tab in indent.
> 	   * and the pattern did not contain any magic characters
>  .git/rebase-apply/patch:7: space before tab in indent.
> 	   * GLOB_NOMAGIC is there just for compatibility with csh.
>  .git/rebase-apply/patch:8: space before tab in indent.
> 	   */
>  .git/rebase-apply/patch:22: space before tab in indent.
> 	  else if (!(pglob->gl_flags & GLOB_NOSORT))
>  .git/rebase-apply/patch:23: space before tab in indent.
> 		  qsort(pglob->gl_pathv + pglob->gl_offs + oldpathc,
>  warning: squelched 1 whitespace error
>  warning: 6 lines add whitespace errors.
>
>
> Thanks,
> Corinna
>
>
diff --git a/newlib/libc/include/glob.h b/newlib/libc/include/glob.h
index 7a300e69d..c14840cf0 100644
--- a/newlib/libc/include/glob.h
+++ b/newlib/libc/include/glob.h
@@ -80,6 +80,7 @@ typedef struct {
 
 #define	GLOB_NOSPACE	(-1)	/* Malloc call failed. */
 #define	GLOB_ABEND	(-2)	/* Unignored error. */
+#define	GLOB_NOMATCH	(-3)	/* No match and GLOB_NOCHECK not set. */
 
 __BEGIN_DECLS
 int	glob(const char *__restrict, int, int (*)(const char *, int), 
diff --git a/newlib/libc/posix/glob.c b/newlib/libc/posix/glob.c
index 5e6c2fcba..20eec0263 100644
--- a/newlib/libc/posix/glob.c
+++ b/newlib/libc/posix/glob.c
@@ -502,11 +502,14 @@ glob0(pattern, pglob, limit)
 	 * and the pattern did not contain any magic characters
 	 * GLOB_NOMAGIC is there just for compatibility with csh.
 	 */
-	if (pglob->gl_pathc == oldpathc &&
-	    ((pglob->gl_flags & GLOB_NOCHECK) ||
-	      ((pglob->gl_flags & GLOB_NOMAGIC) &&
-	       !(pglob->gl_flags & GLOB_MAGCHAR))))
-		return(globextend(pattern, pglob, limit));
+	if (pglob->gl_pathc == oldpathc) {
+		if ((pglob->gl_flags & GLOB_NOCHECK) ||
+		    ((pglob->gl_flags & GLOB_NOMAGIC) &&
+		    !(pglob->gl_flags & GLOB_MAGCHAR)))
+			return(globextend(pattern, pglob, limit));
+		else
+			return(GLOB_NOMATCH);
+	}
 	else if (!(pglob->gl_flags & GLOB_NOSORT))
 		qsort(pglob->gl_pathv + pglob->gl_offs + oldpathc,
 		    pglob->gl_pathc - oldpathc, sizeof(char *), compare);
  

Hi Jordi,

the patch looks good, thank you.  Can you please send it as a git patch
created with `git format-patch' and a nice commit message?


Thanks,
Corinna


On Aug 20 08:03, Jordi Sanfeliu wrote:
> Find the patch attached, I hope is OK this time.
> Thanks.
> 
> --
> Jordi Sanfeliu
> FIBRANET Network Services Provider
> https://www.fibranet.cat
> 
> 
> On Mon, 19 Aug 2024, Corinna Vinschen wrote:
> 
> > Hi Jordi,
> > 
> > On Aug 17 17:38, Jordi Sanfeliu wrote:
> > > Hello,
> > > 
> > > While porting 'mandoc' [1] to my hobbyOS FiwixOS, I've discovered that the
> > > glob() function in Newlib always returns zero (success) even when the
> > > pathname was not found.
> > > 
> > > I was comparing the file 'libc/posix/glob.c' with the one from Apple [2] and
> > > then I applied the following patch:
> > > 
> > > diff --git a/newlib/libc/posix/glob.c b/newlib/libc/posix/glob.c
> > > index 5e6c2fcba..0347979de 100644
> > > --- a/newlib/libc/posix/glob.c
> > > +++ b/newlib/libc/posix/glob.c
> > > @@ -502,11 +502,14 @@ glob0(pattern, pglob, limit)
> > >   	 * and the pattern did not contain any magic characters
> > >   	 * GLOB_NOMAGIC is there just for compatibility with csh.
> > >   	 */
> > > -	if (pglob->gl_pathc == oldpathc &&
> > > -	    ((pglob->gl_flags & GLOB_NOCHECK) ||
> > > -	      ((pglob->gl_flags & GLOB_NOMAGIC) &&
> > > -	       !(pglob->gl_flags & GLOB_MAGCHAR))))
> > > -		return(globextend(pattern, pglob, limit));
> > > +	if (pglob->gl_pathc == oldpathc) {
> > > +	    if (((pglob->gl_flags & GLOB_NOCHECK) ||
> > > +	        ((pglob->gl_flags & GLOB_NOMAGIC) &&
> > > +	        !(pglob->gl_flags & GLOB_MAGCHAR))))
> > > +		    return(globextend(pattern, pglob, limit));
> > > +	    else
> > 
> > This is correct and as upstream, but...
> > 
> > > +		    return(3);	/* GLOB_NOMATCH */
> > 
> > ...this isn't.  Check with libc/include/glob.h.  Your patch should
> > add a
> > 
> >  #define GLOB_NOMATCH (-3)
> > 
> > to glob.h instead.
> > 
> > Also, check your whitespaces.  I'm getting quite a few warnings from
> > `git am':
> > 
> >  Applying: Fix glob() function
> >  .git/rebase-apply/patch:6: space before tab in indent.
> > 	   * and the pattern did not contain any magic characters
> >  .git/rebase-apply/patch:7: space before tab in indent.
> > 	   * GLOB_NOMAGIC is there just for compatibility with csh.
> >  .git/rebase-apply/patch:8: space before tab in indent.
> > 	   */
> >  .git/rebase-apply/patch:22: space before tab in indent.
> > 	  else if (!(pglob->gl_flags & GLOB_NOSORT))
> >  .git/rebase-apply/patch:23: space before tab in indent.
> > 		  qsort(pglob->gl_pathv + pglob->gl_offs + oldpathc,
> >  warning: squelched 1 whitespace error
> >  warning: 6 lines add whitespace errors.
> > 
> > 
> > Thanks,
> > Corinna
> > 
> > 

> diff --git a/newlib/libc/include/glob.h b/newlib/libc/include/glob.h
> index 7a300e69d..c14840cf0 100644
> --- a/newlib/libc/include/glob.h
> +++ b/newlib/libc/include/glob.h
> @@ -80,6 +80,7 @@ typedef struct {
>  
>  #define	GLOB_NOSPACE	(-1)	/* Malloc call failed. */
>  #define	GLOB_ABEND	(-2)	/* Unignored error. */
> +#define	GLOB_NOMATCH	(-3)	/* No match and GLOB_NOCHECK not set. */
>  
>  __BEGIN_DECLS
>  int	glob(const char *__restrict, int, int (*)(const char *, int), 
> diff --git a/newlib/libc/posix/glob.c b/newlib/libc/posix/glob.c
> index 5e6c2fcba..20eec0263 100644
> --- a/newlib/libc/posix/glob.c
> +++ b/newlib/libc/posix/glob.c
> @@ -502,11 +502,14 @@ glob0(pattern, pglob, limit)
>  	 * and the pattern did not contain any magic characters
>  	 * GLOB_NOMAGIC is there just for compatibility with csh.
>  	 */
> -	if (pglob->gl_pathc == oldpathc &&
> -	    ((pglob->gl_flags & GLOB_NOCHECK) ||
> -	      ((pglob->gl_flags & GLOB_NOMAGIC) &&
> -	       !(pglob->gl_flags & GLOB_MAGCHAR))))
> -		return(globextend(pattern, pglob, limit));
> +	if (pglob->gl_pathc == oldpathc) {
> +		if ((pglob->gl_flags & GLOB_NOCHECK) ||
> +		    ((pglob->gl_flags & GLOB_NOMAGIC) &&
> +		    !(pglob->gl_flags & GLOB_MAGCHAR)))
> +			return(globextend(pattern, pglob, limit));
> +		else
> +			return(GLOB_NOMATCH);
> +	}
>  	else if (!(pglob->gl_flags & GLOB_NOSORT))
>  		qsort(pglob->gl_pathv + pglob->gl_offs + oldpathc,
>  		    pglob->gl_pathc - oldpathc, sizeof(char *), compare);
  

Hi Corinna,

Find the new patch attached.
Thanks.

--
Jordi Sanfeliu
FIBRANET Network Services Provider
https://www.fibranet.cat


On Tue, 20 Aug 2024, Corinna Vinschen wrote:

> Hi Jordi,
>
> the patch looks good, thank you.  Can you please send it as a git patch
> created with `git format-patch' and a nice commit message?
>
>
> Thanks,
> Corinna
>
From: Jordi Sanfeliu <jordi@fibranet.cat>

Fixed glob() function to return GLOB_NOMATCH if pattern does
not match any existing pathname (and GLOB_NOCHECK was not set in flags).

---
 newlib/libc/include/glob.h |  1 +
 newlib/libc/posix/glob.c   | 13 ++++++++-----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/newlib/libc/include/glob.h b/newlib/libc/include/glob.h
index 7a300e69d..c14840cf0 100644
--- a/newlib/libc/include/glob.h
+++ b/newlib/libc/include/glob.h
@@ -80,6 +80,7 @@ typedef struct {
 
 #define	GLOB_NOSPACE	(-1)	/* Malloc call failed. */
 #define	GLOB_ABEND	(-2)	/* Unignored error. */
+#define	GLOB_NOMATCH	(-3)	/* No match and GLOB_NOCHECK not set. */
 
 __BEGIN_DECLS
 int	glob(const char *__restrict, int, int (*)(const char *, int), 
diff --git a/newlib/libc/posix/glob.c b/newlib/libc/posix/glob.c
index 5e6c2fcba..20eec0263 100644
--- a/newlib/libc/posix/glob.c
+++ b/newlib/libc/posix/glob.c
@@ -502,11 +502,14 @@ glob0(pattern, pglob, limit)
 	 * and the pattern did not contain any magic characters
 	 * GLOB_NOMAGIC is there just for compatibility with csh.
 	 */
-	if (pglob->gl_pathc == oldpathc &&
-	    ((pglob->gl_flags & GLOB_NOCHECK) ||
-	      ((pglob->gl_flags & GLOB_NOMAGIC) &&
-	       !(pglob->gl_flags & GLOB_MAGCHAR))))
-		return(globextend(pattern, pglob, limit));
+	if (pglob->gl_pathc == oldpathc) {
+		if ((pglob->gl_flags & GLOB_NOCHECK) ||
+		    ((pglob->gl_flags & GLOB_NOMAGIC) &&
+		    !(pglob->gl_flags & GLOB_MAGCHAR)))
+			return(globextend(pattern, pglob, limit));
+		else
+			return(GLOB_NOMATCH);
+	}
 	else if (!(pglob->gl_flags & GLOB_NOSORT))
 		qsort(pglob->gl_pathv + pglob->gl_offs + oldpathc,
 		    pglob->gl_pathc - oldpathc, sizeof(char *), compare);
-- 
2.46.0
  

On Aug 20 14:07, Jordi Sanfeliu wrote:
> Hi Corinna,
> 
> Find the new patch attached.

Still not quite in the git format-patch style, but never mind for now.

Patch pushed.


Thanks,
Corinna
  

Hi Corinna,

Thanks for accepting my patch.

I'm afraid I'm too idiotized with the simplicity of GitHub, excuse my 
ignorance.

I've executed 'git format-patch' but it returned nothing. Then, I made a 
local commit and after executing the command 'git format-patch -1 HEAD' 
it created a file named 
'0001-fixed-glob-function-to-return-GLOB_NOMATCH-if-patter.patch', which 
is the file I sent you.

Thanks again.
Regards.


On 8/20/24 14:47, Corinna Vinschen wrote:
> On Aug 20 14:07, Jordi Sanfeliu wrote:
>> Hi Corinna,
>>
>> Find the new patch attached.
> 
> Still not quite in the git format-patch style, but never mind for now.
> 
> Patch pushed.
> 
> 
> Thanks,
> Corinna
>
  

Hi Jordi,

On Aug 20 16:03, Jordi Sanfeliu wrote:
> Hi Corinna,
> 
> Thanks for accepting my patch.
> 
> I'm afraid I'm too idiotized with the simplicity of GitHub, excuse my
> ignorance.
> 
> I've executed 'git format-patch' but it returned nothing. Then, I made a
> local commit and after executing the command 'git format-patch -1 HEAD' it
> created a file named
> '0001-fixed-glob-function-to-return-GLOB_NOMATCH-if-patter.patch', which is
> the file I sent you.

It's not much of a prolem now, but I'm a bit puzzled because the output
of `git format-patch' is usually a mail including some basic headers.
These headers were missing from your attachment, so it didn't look like
`git format-patch' output to me.  Did I miss something?


Thanks,
Corinna
  

Hi Corinna,

Argh!, my fault. I removed the email headers because I found the Subject 
line redundant, and I thought that these headers were irrelevant. TIL, 
so this shouldn't happen again.

I'm sorry for the excessive noise because of my ignorance, but thanks 
for your patience.

Best regards.


On 8/21/24 11:06, Corinna Vinschen wrote:
> Hi Jordi,
> 
> On Aug 20 16:03, Jordi Sanfeliu wrote:
>> Hi Corinna,
>>
>> Thanks for accepting my patch.
>>
>> I'm afraid I'm too idiotized with the simplicity of GitHub, excuse my
>> ignorance.
>>
>> I've executed 'git format-patch' but it returned nothing. Then, I made a
>> local commit and after executing the command 'git format-patch -1 HEAD' it
>> created a file named
>> '0001-fixed-glob-function-to-return-GLOB_NOMATCH-if-patter.patch', which is
>> the file I sent you.
> 
> It's not much of a prolem now, but I'm a bit puzzled because the output
> of `git format-patch' is usually a mail including some basic headers.
> These headers were missing from your attachment, so it didn't look like
> `git format-patch' output to me.  Did I miss something?
> 
> 
> Thanks,
> Corinna
>