bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/8] posix: Use char_array for internal glob dirname


From: Adhemerval Zanella
Subject: Re: [PATCH 2/8] posix: Use char_array for internal glob dirname
Date: Wed, 24 Mar 2021 14:39:17 -0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1


On 23/03/2021 13:08, Arjun Shankar wrote:
> Hi Adhemerval,
> 
>> This is the first patch of the set to remove alloca usage on glob
>> implementation.  Internal path to search for file might expand to a
>> non static directory derived from pattern for some difference cases
>> (GLOB_NOESCAPE, GNU GLOB_TILDE) and to allow a non-static dirname
>> path glob uses a lot of boilerplate code to manage the buffer (which
>> is either allocated using alloca or malloc depending both to size
>> requested and the total alloca_used).
>>
>> The patch changes to use the char_array struct with the default size
>> (256 bytes).  It simplifies all the allocation code by using char_array
>> one and every internal buffer access is done using char_array provided
>> functions.  No functional changes are expected.
> 
> I've been going through this patch series. Here are my comments on the
> first one. I have mostly been looking at this from the point of view of
> making sure the logic remains equivalent, and not the way you tackled
> the problem itself.
> 
> In summary, I found a comparison reversed, a missed concatenation, and
> some minor indent issues. Other than that, this patch looks good to me.

Thanks for the review, however I would like to check if this approach is
what he want to move the glob implementation since it might diverge from
what Paul suggested for gnulib [1].

I tried to model the change to use something similar to a C++ string,
while Paul is caving out the required allocation from a scratch_buffer.
My view is since we expanding multiple different objects (the directory
name, the home directory, the username) it would make sense to make
then different dynarray, Paul in the other hand see that we can cave out
this multiple objects from a single scratch_buffer to simplify the
memory management. Maybe modelling like a C++ string is not the right
approach, since we need manual delocation anyway.

I don't have a strong opinion here, I will take a second look on his
patch to check if the outcome is simpler than my initial patchset.
My only goal here is get rid of the multuiple alloca and simplify
the memory management.

[1] https://sourceware.org/pipermail/libc-alpha/2021-January/121605.html

> 
> Details:
> 
>> diff --git a/posix/glob.c b/posix/glob.c
>> index 32c88e5d15..8c6e1914c5 100644
>> --- a/posix/glob.c
>> +++ b/posix/glob.c
>> @@ -85,6 +85,7 @@
>>  #include <flexmember.h>
>>  #include <glob_internal.h>
>>  #include <scratch_buffer.h>
>> +#include <malloc/char_array-skeleton.c>
> 
> Include the new dynamic character array implementation. OK.
> 
> Note:
> The patch that introduces char_array-skeleton.c will need a slight adjustment
> after de0e1b45b0ab (malloc: Sync dynarray with gnulib) due to the removal
> of the anonymous union.

Ack.

> 
>>  static const char *next_brace_sub (const char *begin, int flags) __THROWNL;
>>  
>> @@ -298,16 +299,15 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>          glob_t *pglob)
>>  {
>>    const char *filename;
>> -  char *dirname = NULL;
>>    size_t dirlen;
>>    int status;
>>    size_t oldcount;
>>    int meta;
>> -  int dirname_modified;
>> -  int malloc_dirname = 0;
>>    glob_t dirs;
>>    int retval = 0;
>>    size_t alloca_used = 0;
>> +  struct char_array dirname;
>> +  bool dirname_modified;
> 
> dirname changes type, dirname_modified should be a boolean, and malloc_dirname
> is now redundant.
> 
> OK.
>  
>>    if (pattern == NULL || pglob == NULL || (flags & ~__GLOB_FLAGS) != 0)
>>      {
>> @@ -315,6 +315,10 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>        return -1;
>>      }
>>  
>> +  /* Default char array is stack allocated, so there is no need to check
>> +     if setting the initial '\0' succeeds.  */
>> +  char_array_init_empty (&dirname);
>> +
>>    /* POSIX requires all slashes to be matched.  This means that with
>>       a trailing slash we must match only directories.  */
>>    if (pattern[0] && pattern[strlen (pattern) - 1] == '/')
> 
> OK.
> 
>> @@ -335,12 +339,12 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>            size_t i;
>>  
>>            if (pglob->gl_offs >= ~((size_t) 0) / sizeof (char *))
>> -            return GLOB_NOSPACE;
>> +            goto err_nospace;
>>  
>>            pglob->gl_pathv = (char **) malloc ((pglob->gl_offs + 1)
>>                                                * sizeof (char *));
> 
> err_nospace frees dirname and returns GLOB_NOSPACE.
> So the code is equivalent.
> OK.
> 
>>            if (pglob->gl_pathv == NULL)
>> -            return GLOB_NOSPACE;
>> +            goto err_nospace;
>>  
>>            for (i = 0; i <= pglob->gl_offs; ++i)
>>              pglob->gl_pathv[i] = NULL;
>> @@ -392,7 +396,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>              {
>>                onealt = malloc (pattern_len);
>>                if (onealt == NULL)
>> -                return GLOB_NOSPACE;
>> +                goto err_nospace;
>>              }
>>  
>>            /* We know the prefix for all sub-patterns.  */
> 
> OK. Same.
> 
>> @@ -454,7 +458,8 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                        globfree (pglob);
>>                        pglob->gl_pathc = 0;
>>                      }
>> -                  return result;
>> +                  retval = result;
>> +                  goto out;
>>                  }
>>  
>>                if (*next == '}')
> 
> out frees dirname and returns retval.
> So the code is equivalent.
> OK.
> 
>> @@ -471,9 +476,10 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>  
>>            if (pglob->gl_pathc != firstc)
>>              /* We found some entries.  */
>> -            return 0;
>> +            retval = 0;
> 
> We will continue at 'out', which will also return after freeing dirname.
> So the code remains equivalent.
> OK.
> 
>>            else if (!(flags & (GLOB_NOCHECK|GLOB_NOMAGIC)))
>> -            return GLOB_NOMATCH;
>> +            retval = GLOB_NOMATCH;
>> +          goto out;
>>          }
>>      }
>>  
> 
> OK. Same here.
>  
>> @@ -492,14 +498,15 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>      filename = strchr (pattern, ':');
>>  #endif /* __MSDOS__ || WINDOWS32 */
>>  
>> -  dirname_modified = 0;
>> +  dirname_modified = false;
>>    if (filename == NULL)
>>      {
> 
> OK. It's declared as a boolean now.
> 
>>        /* This can mean two things: a simple name or "~name".  The latter
>>           case is nothing but a notation for a directory.  */
>>        if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK)) && pattern[0] == '~')
>>          {
>> -          dirname = (char *) pattern;
>> +         if (!char_array_set_str (&dirname, pattern))
>> +           goto err_nospace;
>>            dirlen = strlen (pattern);
>>  
>>            /* Set FILENAME to NULL as a special flag.  This is ugly but
> 
> OK. It's still equivalent. Since char_array_set_str can lead to a failed
> allocation, we add a check and exit with error if that happens.
> Indent is a bit off, though.

Ack.

> 
>> @@ -516,7 +523,8 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>              }
>>  
>>            filename = pattern;
>> -          dirname = (char *) ".";
>> +         if (!char_array_set_str (&dirname, "."))
>> +           goto err_nospace;
>>            dirlen = 0;
>>          }
>>      }
> 
> OK. Same as last. Return an error if the allocation fails.
> Indent is a bit off.
> 
>> @@ -525,13 +533,13 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                 && (flags & GLOB_NOESCAPE) == 0))
>>      {
>>        /* "/pattern" or "\\/pattern".  */
>> -      dirname = (char *) "/";
>> +      if (!char_array_set_str (&dirname, "/"))
>> +       goto err_nospace;
>>        dirlen = 1;
>>        ++filename;
>>      }
> 
> OK.
> 
>>    else
>>      {
>> -      char *newp;
>>        dirlen = filename - pattern;
>>  #if defined __MSDOS__ || defined WINDOWS32
>>        if (*filename == ':'
> 
> OK. newp was used for malloc/alloca allocations.
> 
>> @@ -545,31 +553,26 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>            /* For now, disallow wildcards in the drive spec, to
>>               prevent infinite recursion in glob.  */
>>            if (__glob_pattern_p (drive_spec, !(flags & GLOB_NOESCAPE)))
>> -            return GLOB_NOMATCH;
>> +           {
>> +             retval = GLOB_NOMATCH;
>> +             goto out;
>> +           }
> 
> We need to deallocate before every return now. This does that.
> OK.
> 
>>            /* If this is "d:pattern", we need to copy ':' to DIRNAME
>>               as well.  If it's "d:/pattern", don't remove the slash
>>               from "d:/", since "d:" and "d:/" are not the same.*/
>>          }
>>  #endif
>>  
>> -      if (glob_use_alloca (alloca_used, dirlen + 1))
>> -        newp = alloca_account (dirlen + 1, alloca_used);
>> -      else
>> -        {
>> -          newp = malloc (dirlen + 1);
>> -          if (newp == NULL)
>> -            return GLOB_NOSPACE;
>> -          malloc_dirname = 1;
>> -        }
>> -      *((char *) mempcpy (newp, pattern, dirlen)) = '\0';
>> -      dirname = newp;
>> +      if (!char_array_set_str_size (&dirname, pattern, dirlen))
>> +       goto err_nospace;
>>        ++filename;
> 
> We used to alloca/malloc some space and copy pattern into it. We still do
> the same but using a dynarray. So don't need malloc_dirname any more.
> OK.
> 
>>  #if defined __MSDOS__ || defined WINDOWS32
>>        bool drive_root = (dirlen > 1
>> -                         && (dirname[dirlen - 1] == ':'
>> -                             || (dirlen > 2 && dirname[dirlen - 2] == ':'
>> -                                 && dirname[dirlen - 1] == '/')));
>> +                         && (char_array_pos (&dirname, dirlen - 1) != ':'
>> +                             || (dirlen > 2
>> +                             && char_array_pos (&dirname, dirlen - 2) != ':'
>> +                                 && char_array_pos (&dirname, dirlen - 1) 
>> != '/')));
>>  #else
>>        bool drive_root = false;
>>  #endif
> 
> Looks like the comparisons have been reversed. I think they should be `=='
> instead of `!='.
> 
>> @@ -578,20 +581,24 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>          /* "pattern/".  Expand "pattern", appending slashes.  */
>>          {
>>            int orig_flags = flags;
>> -          if (!(flags & GLOB_NOESCAPE) && dirname[dirlen - 1] == '\\')
>> +          if (!(flags & GLOB_NOESCAPE)
>> +              && char_array_pos (&dirname, dirlen - 1) == '\\')
>>              {
> 
> OK.
> 
>>                /* "pattern\\/".  Remove the final backslash if it hasn't
>>                   been quoted.  */
>> -              char *p = (char *) &dirname[dirlen - 1];
>> -
>> -              while (p > dirname && p[-1] == '\\') --p;
>> -              if ((&dirname[dirlen] - p) & 1)
>> +              size_t p = dirlen - 1;
>> +              while (p > 0 && char_array_pos (&dirname, p - 1) == '\\') --p;
>> +              if ((dirlen - p) & 1)
>>                  {
>> -                  *(char *) &dirname[--dirlen] = '\0';
>> +                  /* Since we are shrinking the array, there is no need to
>> +                     check the function return.  */
>> +                  dirlen -= 1;
>> +                  char_array_crop (&dirname, dirlen);
>>                    flags &= ~(GLOB_NOCHECK | GLOB_NOMAGIC);
>>                  }
>>              }
> 
> p was a pointer to the last character in dirname, and we looped on it going
> backwards towards the start of dirname looking for a '\'.
> 
> Now, p is an index into dirname and we do the same thing.
> 
> Looks equivalent, and more readable.
> OK.
> 
>> -          int val = __glob (dirname, flags | GLOB_MARK, errfunc, pglob);
>> +          int val = __glob (char_array_str (&dirname), flags | GLOB_MARK,
>> +                            errfunc, pglob);
>>            if (val == 0)
>>              pglob->gl_flags = ((pglob->gl_flags & ~GLOB_MARK)
>>                                 | (flags & GLOB_MARK));
> 
> OK.
> 
>> @@ -608,11 +615,14 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>          }
>>      }
>>  
>> -  if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK)) && dirname[0] == '~')
>> +  if ((flags & (GLOB_TILDE|GLOB_TILDE_CHECK))
>> +      && char_array_pos (&dirname, 0) == '~')
>>      {
> 
> OK.
> 
>> -      if (dirname[1] == '\0' || dirname[1] == '/'
>> -          || (!(flags & GLOB_NOESCAPE) && dirname[1] == '\\'
>> -              && (dirname[2] == '\0' || dirname[2] == '/')))
>> +      if (char_array_pos (&dirname, 1) == '\0'
>> +         || char_array_pos (&dirname, 1) == '/'
>> +         || (!(flags & GLOB_NOESCAPE) && char_array_pos (&dirname, 1) == 
>> '\\'
>> +             && (char_array_pos (&dirname, 2) == '\0'
>> +                || char_array_pos (&dirname, 2) == '/')))
> 
> OK. They do the same thing.
> Indent needs a fix.
> 
>>          {
>>            /* Look up home directory.  */
>>            char *home_dir = getenv ("HOME");
>> @@ -663,10 +673,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                    if (err != ERANGE)
>>                      break;
>>                    if (!scratch_buffer_grow (&s))
>> -                    {
>> -                      retval = GLOB_NOSPACE;
>> -                      goto out;
>> -                    }
>> +                  goto err_nospace;
>>                  }
>>                if (err == 0)
>>                  {
> 
> OK.
> 
>> @@ -675,10 +682,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                  }
>>                scratch_buffer_free (&s);
>>                if (err == 0 && home_dir == NULL)
>> -                {
>> -                  retval = GLOB_NOSPACE;
>> -                  goto out;
>> -                }
>> +              goto err_nospace;
>>  #endif /* WINDOWS32 */
>>              }
>>            if (home_dir == NULL || home_dir[0] == '\0')
> 
> OK.
> 
>> @@ -697,53 +701,26 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                  }
>>              }
>>            /* Now construct the full directory.  */
>> -          if (dirname[1] == '\0')
>> +          if (char_array_pos (&dirname, 1) == '\0')
>>              {
> 
> OK.
> 
>> -              if (__glibc_unlikely (malloc_dirname))
>> -                free (dirname);
>> -
> 
> OK. We don't use malloc for dirname any more.
> 
>> -              dirname = home_dir;
>> -              dirlen = strlen (dirname);
>> -              malloc_dirname = malloc_home_dir;
>> +              if (!char_array_set_str (&dirname, home_dir))
>> +                goto err_nospace;
>> +              dirlen = char_array_size (&dirname) - 1;
> 
> Equivalent, and we don't use malloc_dirname any more so we throw away that
> assignment.
> OK.
> 
>>              }
>>            else
>>              {
>> -              char *newp;
>> -              size_t home_len = strlen (home_dir);
>> -              int use_alloca = glob_use_alloca (alloca_used, home_len + 
>> dirlen);
>> -              if (use_alloca)
>> -                newp = alloca_account (home_len + dirlen, alloca_used);
>> -              else
>> -                {
>> -                  newp = malloc (home_len + dirlen);
>> -                  if (newp == NULL)
>> -                    {
>> -                      if (__glibc_unlikely (malloc_home_dir))
>> -                        free (home_dir);
>> -                      retval = GLOB_NOSPACE;
>> -                      goto out;
>> -                    }
>> -                }
> 
> This code was allocating enough memory to concatenate home_dir and dirname.
> 
>> -              mempcpy (mempcpy (newp, home_dir, home_len),
>> -                       &dirname[1], dirlen);
> 
> ...Then concatenating it.
> 
>> -              if (__glibc_unlikely (malloc_dirname))
>> -                free (dirname);
>> -
>> -              dirname = newp;
>> -              dirlen += home_len - 1;
>> -              malloc_dirname = !use_alloca;
>> -
>> -              if (__glibc_unlikely (malloc_home_dir))
>> -                free (home_dir);
> 
> ...And freeing any previously allocated memory.
> 
>> +              /* Replaces '~' by the obtained HOME dir.  */
>> +              char_array_erase (&dirname, 0);
>> +              if (!char_array_prepend_str (&dirname, home_dir))
>> +               goto err_nospace;
> 
> Now we do it using a dynarray.
> OK. Indent needs a fix.
> 
>>              }
>> -          dirname_modified = 1;
>> +          dirname_modified = true;
>>          }
> 
> OK.
> 
>>        else
>>          {
>>  #ifndef WINDOWS32
>> -          char *end_name = strchr (dirname, '/');
>> +          char *dirnamestr = char_array_at (&dirname, 0);
>> +          char *end_name = strchr (dirnamestr, '/');
> 
> Equivalent. OK.
> 
>>            char *user_name;
>>            int malloc_user_name = 0;
>>            char *unescape = NULL;
>> @@ -752,23 +729,23 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>              {
>>                if (end_name == NULL)
>>                  {
>> -                  unescape = strchr (dirname, '\\');
>> +                unescape = strchr (dirnamestr, '\\');
>>                    if (unescape)
>>                      end_name = strchr (unescape, '\0');
>>                  }
> 
> OK. Indent needs a fix, and further down as well.
> 
>>                else
>> -                unescape = memchr (dirname, '\\', end_name - dirname);
>> +              unescape = memchr (dirnamestr, '\\', end_name - dirnamestr);
>>              }
> 
> OK.
> 
>>            if (end_name == NULL)
>> -            user_name = dirname + 1;
>> +            user_name = dirnamestr + 1;
> 
> OK.
> 
>>            else
>>              {
>>                char *newp;
>> -              if (glob_use_alloca (alloca_used, end_name - dirname))
>> -                newp = alloca_account (end_name - dirname, alloca_used);
>> +              if (glob_use_alloca (alloca_used, end_name - dirnamestr))
>> +                newp = alloca_account (end_name - dirnamestr, alloca_used);
>>                else
>>                  {
>> -                  newp = malloc (end_name - dirname);
>> +                  newp = malloc (end_name - dirnamestr);
>>                    if (newp == NULL)
>>                      {
>>                        retval = GLOB_NOSPACE;
> 
> OK. This newp is for user_name which is tackled in a separate patch.
> 
>> @@ -778,8 +755,8 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                  }
>>                if (unescape != NULL)
>>                  {
>> -                  char *p = mempcpy (newp, dirname + 1,
>> -                                     unescape - dirname - 1);
>> +                  char *p = mempcpy (newp, dirnamestr + 1,
>> +                                     unescape - dirnamestr - 1);
>>                    char *q = unescape;
>>                    while (q != end_name)
>>                      {
>> @@ -801,8 +778,9 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                    *p = '\0';
>>                  }
>>                else
>> -                *((char *) mempcpy (newp, dirname + 1, end_name - dirname - 
>> 1))
>> -                  = '\0';
>> +                *((char *) mempcpy (newp, dirnamestr + 1,
>> +                                    end_name - dirnamestr - 1))
>> +                   = '\0';
>>                user_name = newp;
>>              }
> 
> Same. OK. There appears to be a changed line due to a stray whitespace.
> 
>> @@ -835,39 +813,14 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>              /* If we found a home directory use this.  */
>>              if (p != NULL)
>>                {
>> -                size_t home_len = strlen (p->pw_dir);
>> -                size_t rest_len = end_name == NULL ? 0 : strlen (end_name);
>> -                /* dirname contains end_name; we can't free it now.  */
>> -                char *prev_dirname =
>> -                  (__glibc_unlikely (malloc_dirname) ? dirname : NULL);
>> -                char *d;
>> -
>> -                malloc_dirname = 0;
>> -
>> -                if (glob_use_alloca (alloca_used, home_len + rest_len + 1))
>> -                  dirname = alloca_account (home_len + rest_len + 1,
>> -                                            alloca_used);
>> -                else
>> +                if (!char_array_set_str (&dirname, p->pw_dir))
>>                    {
>> -                    dirname = malloc (home_len + rest_len + 1);
>> -                    if (dirname == NULL)
>> -                      {
>> -                        free (prev_dirname);
>> -                        scratch_buffer_free (&pwtmpbuf);
>> -                        retval = GLOB_NOSPACE;
>> -                        goto out;
>> -                      }
>> -                    malloc_dirname = 1;
>> +                    scratch_buffer_free (&pwtmpbuf);
>> +                    goto err_nospace;
>>                    }
>> -                d = mempcpy (dirname, p->pw_dir, home_len);
>> -                if (end_name != NULL)
>> -                  d = mempcpy (d, end_name, rest_len);
>> -                *d = '\0';
>> -
>> -                free (prev_dirname);
>>  
>> -                dirlen = home_len + rest_len;
>> -                dirname_modified = 1;
>> +                dirlen = strlen (p->pw_dir);
>> +                dirname_modified = true;
>>                }
> 
> It appears that a concatenation (p->pw_dir and end_name) got missed here.
> 
>>              else
>>                {
>> @@ -908,37 +861,33 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>          goto nospace;
>>        pglob->gl_pathv = new_gl_pathv;
>>  
>> -      if (flags & GLOB_MARK && is_dir (dirname, flags, pglob))
>> +      if (flags & GLOB_MARK
>> +          && is_dir (char_array_str (&dirname), flags, pglob))
> 
> OK.
> 
>>          {
>>            char *p;
>>            pglob->gl_pathv[newcount] = malloc (dirlen + 2);
>>            if (pglob->gl_pathv[newcount] == NULL)
>>              goto nospace;
>> -          p = mempcpy (pglob->gl_pathv[newcount], dirname, dirlen);
>> +          p = mempcpy (pglob->gl_pathv[newcount],
>> +                       char_array_str (&dirname), dirlen);
> 
> OK.
> 
>>            p[0] = '/';
>>            p[1] = '\0';
>> -          if (__glibc_unlikely (malloc_dirname))
>> -            free (dirname);
>>          }
> 
> OK.
> 
>>        else
>>          {
>> -          if (__glibc_unlikely (malloc_dirname))
>> -            pglob->gl_pathv[newcount] = dirname;
>> -          else
>> -            {
>> -              pglob->gl_pathv[newcount] = strdup (dirname);
>> -              if (pglob->gl_pathv[newcount] == NULL)
>> -                goto nospace;
>> -            }
>> +          pglob->gl_pathv[newcount] = strdup (char_array_str (&dirname));
>> +          if (pglob->gl_pathv[newcount] == NULL)
>> +            goto nospace;
>>          }
> 
> OK.
> 
>>        pglob->gl_pathv[++newcount] = NULL;
>>        ++pglob->gl_pathc;
>>        pglob->gl_flags = flags;
>>  
>> -      return 0;
>> +      goto out;
>>      }
> 
> OK. 'out' so we can deallocate before returning.
> 
>>  
>> -  meta = __glob_pattern_type (dirname, !(flags & GLOB_NOESCAPE));
>> +  meta = __glob_pattern_type (char_array_str (&dirname),
>> +                              !(flags & GLOB_NOESCAPE));
> 
> OK.
> 
>>    /* meta is 1 if correct glob pattern containing metacharacters.
>>       If meta has bit (1 << 2) set, it means there was an unterminated
>>       [ which we handle the same, using fnmatch.  Broken unterminated
>> @@ -951,15 +900,15 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>           the pattern in each directory found.  */
>>        size_t i;
>>  
>> -      if (!(flags & GLOB_NOESCAPE) && dirlen > 0 && dirname[dirlen - 1] == 
>> '\\')
>> +      if (!(flags & GLOB_NOESCAPE) && dirlen > 0
>> +          && char_array_pos (&dirname, dirlen - 1) == '\\')
> 
> OK.
> 
>>          {
>>            /* "foo\\/bar".  Remove the final backslash from dirname
>>               if it has not been quoted.  */
>> -          char *p = (char *) &dirname[dirlen - 1];
>> -
>> -          while (p > dirname && p[-1] == '\\') --p;
>> -          if ((&dirname[dirlen] - p) & 1)
>> -            *(char *) &dirname[--dirlen] = '\0';
>> +          size_t p = dirlen - 1;
>> +          while (p > 0 && char_array_pos (&dirname, p - 1) == '\\') --p;
>> +          if ((dirlen - p) & 1)
>> +            char_array_crop (&dirname, --dirlen);
> 
> Equivalent. We use an index instead of a pointer, and step back from the
> end. OK.
> 
>>          }
>>  
>>        if (__glibc_unlikely ((flags & GLOB_ALTDIRFUNC) != 0))
>> @@ -973,7 +922,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>            dirs.gl_lstat = pglob->gl_lstat;
>>          }
>>  
>> -      status = __glob (dirname,
>> +      status = __glob (char_array_str (&dirname),
>>                         ((flags & (GLOB_ERR | GLOB_NOESCAPE | 
>> GLOB_ALTDIRFUNC))
>>                          | GLOB_NOSORT | GLOB_ONLYDIR),
>>                         errfunc, &dirs);
> 
> OK.
> 
>> @@ -1020,8 +969,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                globfree (&dirs);
>>                globfree (pglob);
>>                pglob->gl_pathc = 0;
>> -              retval = GLOB_NOSPACE;
>> -              goto out;
>> +              goto err_nospace;
> 
> OK.
> 
>>              }
>>          }
>>  
>> @@ -1043,8 +991,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                  {
>>                  nospace2:
>>                    globfree (&dirs);
>> -                  retval = GLOB_NOSPACE;
>> -                  goto out;
>> +                  goto err_nospace;
>>                  }
>>  
>>                new_gl_pathv = realloc (pglob->gl_pathv,
>> @@ -1059,8 +1006,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                    globfree (&dirs);
>>                    globfree (pglob);
>>                    pglob->gl_pathc = 0;
>> -                  retval = GLOB_NOSPACE;
>> -                  goto out;
>> +                  goto err_nospace;
>>                  }
> 
> Same.
> 
>>  
>>                ++pglob->gl_pathc;
>> @@ -1086,7 +1032,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>  
>>        if (meta & GLOBPAT_BACKSLASH)
>>          {
>> -          char *p = strchr (dirname, '\\'), *q;
>> +          char *p = strchr (char_array_str (&dirname), '\\'), *q;
> 
> Okay.
> 
>>            /* We need to unescape the dirname string.  It is certainly
>>               allocated by alloca, as otherwise filename would be NULL
>>               or dirname wouldn't contain backslashes.  */
>> @@ -1103,12 +1049,12 @@ __glob (const char *pattern, int flags, int 
>> (*errfunc) (const char *, int),
>>                ++q;
>>              }
>>            while (*p++ != '\0');
>> -          dirname_modified = 1;
>> +          dirname_modified = true;
>>          }
>>        if (dirname_modified)
>>          flags &= ~(GLOB_NOCHECK | GLOB_NOMAGIC);
>> -      status = glob_in_dir (filename, dirname, flags, errfunc, pglob,
>> -                            alloca_used);
>> +      status = glob_in_dir (filename, char_array_str (&dirname), flags,
>> +                            errfunc, pglob, alloca_used);
> 
> OK.
> 
>>        if (status != 0)
>>          {
>>            if (status == GLOB_NOMATCH && flags != orig_flags
>> @@ -1126,14 +1072,13 @@ __glob (const char *pattern, int flags, int 
>> (*errfunc) (const char *, int),
>>        if (dirlen > 0)
>>          {
>>            /* Stick the directory on the front of each name.  */
>> -          if (prefix_array (dirname,
>> +          if (prefix_array (char_array_str (&dirname),
>>                              &pglob->gl_pathv[old_pathc + pglob->gl_offs],
>>                              pglob->gl_pathc - old_pathc))
> 
> OK.
> 
>>              {
>>                globfree (pglob);
>>                pglob->gl_pathc = 0;
>> -              retval = GLOB_NOSPACE;
>> -              goto out;
>> +              goto err_nospace;
> 
> OK.
> 
>>              }
>>          }
>>      }
>> @@ -1152,8 +1097,7 @@ __glob (const char *pattern, int flags, int (*errfunc) 
>> (const char *, int),
>>                {
>>                  globfree (pglob);
>>                  pglob->gl_pathc = 0;
>> -                retval = GLOB_NOSPACE;
>> -                goto out;
>> +                goto err_nospace;
> 
> Same.
> 
>>                }
>>              strcpy (&new[len - 2], "/");
>>              pglob->gl_pathv[i] = new;
>> @@ -1169,10 +1113,13 @@ __glob (const char *pattern, int flags, int 
>> (*errfunc) (const char *, int),
>>      }
>>  
>>   out:
>> -  if (__glibc_unlikely (malloc_dirname))
>> -    free (dirname);
>>  
>> +  char_array_free (&dirname);
>>    return retval;
>> +
>> + err_nospace:
>> +  char_array_free (&dirname);
>> +  return GLOB_NOSPACE;
>>  }
> 
> OK. out and err_nospace, both deallocate before returning.
> 
> Cheers,
> Arjun
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]