This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [RFC][BZ #10278] glob() gives inconsistent results with trailing "/"
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: OndÅej BÃlka <neleai at seznam dot cz>
- Cc: eblake at redhat dot com, libc-alpha at sourceware dot org
- Date: Thu, 17 Oct 2013 12:37:17 -0400
- Subject: Re: [RFC][BZ #10278] glob() gives inconsistent results with trailing "/"
- Authentication-results: sourceware.org; auth=none
- References: <20131003075937 dot GA22576 at domone dot podge> <524D2FA4 dot 2020403 at redhat dot com> <20131011152427 dot GC18534 at domone dot podge> <525F377C dot 8070507 at redhat dot com> <20131017162058 dot GA3385 at domone dot podge>
On 10/17/2013 12:20 PM, OndÅej BÃlka wrote:
> On Wed, Oct 16, 2013 at 09:03:56PM -0400, Carlos O'Donell wrote:
>> On 10/11/2013 11:24 AM, OndÅej BÃlka wrote:
>>> On Thu, Oct 03, 2013 at 04:49:40AM -0400, Carlos O'Donell wrote:
>>>> On 10/03/2013 03:59 AM, OndÅej BÃlka wrote:
>>>>> Hi,
>>>>>
>>>>> For this bug https://sourceware.org/bugzilla/show_bug.cgi?id=10278 there
>>>>> is a simple fix when we have DIRENT_H defined.
>>>>>
>>>>> For cross platform solution we would need surround this by ifdefs when
>>>>> separator is not slash and handling case of no dirent.
>>>>>
>>>>> Comments?
>>>>
>>>> Sounds correct.
>>>>
>>>> We should add a regression test for this.
>>>>
>>>
>>> When I tried add a test I encountered an unexpected result, any insight
>>> into this?
>>>
>>> results for glob ("*/*/", GLOB_ALTDIRFUNC)
>>> dir1lev1/dir1lev2/
>>> dir1lev1/dir2lev2/
>>> dir1lev1/dir3lev2/
>>> dir2lev1/dir1lev2/
>>> file2lev1/dir2lev1 *** WRONG
>>
>> No idea, I'd have to dig into this to figure out why.
>>
>> You definitely need to figure out why before we can
>> checkin the patch.
>>
>
> It turned out that testcase was wrong, after uncommenting DEBUG a output
> was
>
> my_opendir(".") == { level: 1, idx: 0 }
> my_readdir ({ level: 1, idx: 0 }) = { d_ino: 0, d_type: 4, d_name: "." }
> my_readdir ({ level: 1, idx: 1 }) = { d_ino: 1, d_type: 4, d_name: ".." }
> my_readdir ({ level: 1, idx: 2 }) = { d_ino: 2, d_type: 8, d_name: "file1lev1" }
> my_readdir ({ level: 1, idx: 3 }) = { d_ino: 3, d_type: 0, d_name: "file2lev1" }
> looking for file2lev1, level 1
> my_stat ("./file2lev1", { st_mode: 40777 }) = 0
> my_readdir ({ level: 1, idx: 4 }) = { d_ino: 4, d_type: 0, d_name: "dir1lev1" }
> looking for dir1lev1, level 1
> my_stat ("./dir1lev1", { st_mode: 40777 }) = 0
> my_readdir ({ level: 1, idx: 28 }) = { d_ino: 28, d_type: 4, d_name: "dir2lev1" }
> my_readdir ({ level: 1, idx: -1 }) = NULL
> my_closedir ()
> looking for file2lev1, level 1
> my_opendir("file2lev1") == { level: 1, idx: 4 }
> my_readdir ({ level: 1, idx: 4 }) = { d_ino: 4, d_type: 0, d_name: "dir1lev1" }
>
> So my_opendir lacked check that files are not directories. After fixing
> that test passes.
That makes complete sense. I just reviewed that function and I agree with your fix.
> [BZ #10278]
> * posix/glob.c: Match only directories when trailing slash is present.
> * posix/tst-gnuglob.c (my_opendir): Do not open files.
> (main): Add testcase.
>
> diff --git a/posix/glob.c b/posix/glob.c
> index ece71c1..c1d61f0 100644
> --- a/posix/glob.c
> +++ b/posix/glob.c
> @@ -276,6 +276,11 @@ glob (pattern, flags, errfunc, pglob)
> return -1;
> }
>
> + /* Posix requires all slashes to be matched. This means that with
s/Posix/POSIX/g
> + trailing slash we could match only directories. */
s/trailing/a trailing/g
s/could/must/g
> + if (pattern[0] && pattern[strlen (pattern) - 1] == '/')
> + flags |= GLOB_ONLYDIR;
> +
OK.
> if (!(flags & GLOB_DOOFFS))
> /* Have to do this so `globfree' knows where to start freeing. It
> also makes all the code that uses gl_offs simpler. */
> diff --git a/posix/tst-gnuglob.c b/posix/tst-gnuglob.c
> index 0c967d0..5ef8909 100644
> --- a/posix/tst-gnuglob.c
> +++ b/posix/tst-gnuglob.c
> @@ -168,7 +168,7 @@ my_opendir (const char *s)
> my_DIR *dir;
>
>
> - if (idx == -1)
> + if (idx == -1 || filesystem[idx].type != DT_DIR)
OK.
> {
> PRINTF ("my_opendir(\"%s\") == NULL\n", s);
> return NULL;
> @@ -358,7 +358,7 @@ test_result (const char *fmt, int flags, glob_t *gl, const char *str[])
> break;
>
> if (str[inner] == NULL)
> - errstr = ok ? "" : " *** WRONG";
> + errstr = ok ? "" : " *** WRONG";
OK... next time leave out the whitespace change please.
> else
> errstr = ok ? "" : " * wrong position";
>
> @@ -483,6 +483,12 @@ main (void)
> "/file1lev1",
> "/file2lev1");
>
> + test ("*/*/", 0 , 0,
> + "dir1lev1/dir1lev2/",
> + "dir1lev1/dir2lev2/",
> + "dir1lev1/dir3lev2/",
> + "dir2lev1/dir1lev2/");
> +
Looks good.
> test ("", 0, GLOB_NOMATCH, NULL);
>
> test ("", GLOB_NOCHECK, 0, "");
>
Looks good to me.
Cheers,
Carlos.