Re: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'

[Kjetil Barvik <barvik@broadpark.no>]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Thu, 9 Jul 2009 13:35:31 -0700
> Subject: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'
>
> The threaded index preloading will want it, so that it can avoid
> locking by simply using a per-thread symlink/directory cache.
>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> This just exposes a thread-safe version of the symlink checking by 
> allowing a caller to pass in its own local 'struct cache_def' to the 
> function.
>
> No users of this yet, but the next step is trivial and obvious..
>
>  cache.h    |   10 ++++++++++
>  symlinks.c |   21 ++++++++++-----------
>  2 files changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 871c984..f1e5ede 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -744,7 +744,17 @@ struct checkout {
>  };
>  
>  extern int checkout_entry(struct cache_entry *ce, const struct checkout *state, char *topath);
> +
> +struct cache_def {
> +	char path[PATH_MAX + 1];
> +	int len;
> +	int flags;
> +	int track_flags;
> +	int prefix_len_stat_func;
> +};
> +
>  extern int has_symlink_leading_path(const char *name, int len);
> +extern int threaded_has_symlink_leading_path(struct cache_def *, const char *, int);
>  extern int has_symlink_or_noent_leading_path(const char *name, int len);
>  extern int has_dirs_only_path(const char *name, int len, int prefix_len);
>  extern void invalidate_lstat_cache(const char *name, int len);
> diff --git a/symlinks.c b/symlinks.c
> index 08ad353..4bdded3 100644
> --- a/symlinks.c
> +++ b/symlinks.c
> @@ -32,13 +32,7 @@ static int longest_path_match(const char *name_a, int len_a,
>  	return match_len;
>  }
>  
> -static struct cache_def {
> -	char path[PATH_MAX + 1];
> -	int len;
> -	int flags;
> -	int track_flags;
> -	int prefix_len_stat_func;
> -} default_cache;
> +static struct cache_def default_cache;
>  
>  static inline void reset_lstat_cache(struct cache_def *cache)
>  {
> @@ -217,12 +211,17 @@ void clear_lstat_cache(void)
>  /*
>   * Return non-zero if path 'name' has a leading symlink component
>   */
> +int threaded_has_symlink_leading_path(struct cache_def *cache, const char *name, int len)
> +{
> +	return lstat_cache(cache, name, len, FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) & FL_SYMLINK;

  OK, to follow the style the 3 previous lstat_cache() calls was made
  with (and also let the line length be less than 80), it should have
  been written like this:

     	return lstat_cache(cache, name, len,
			   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &
		FL_SYMLINK;

  Notice that the parmeters which is just copied as arguments to l_c()
  is in the same order and on the first line for it self.  The next line
  contains the rest of the arguments, and the &-part is also on it
  a separate line.

  Stylefix only, so not a big deal.

> +}
> +
> +/*
> + * Return non-zero if path 'name' has a leading symlink component
> + */
>  int has_symlink_leading_path(const char *name, int len)
>  {
> -	struct cache_def *cache = &default_cache;	/* FIXME */

   This would make it inconsistent with the 2 has_*_() functions below,
   which both have such a line.  Only stylefix, no change in semantics.

   I personally liked this line, since it will then be easier to
   "threadify" the function with an extra parameter named "cache".

> -	return lstat_cache(cache, name, len,
> -			   FL_SYMLINK|FL_DIR, USE_ONLY_LSTAT) &
> -		FL_SYMLINK;
> +	return threaded_has_symlink_leading_path(&default_cache, name, len);
>  }
>  
>  /*

  I have looked at and tested (the version from the origin/pu branch, so
  it contains the memset() line squashed in) patch 5/3, 6/3 and 7/3, and
  all 3 patches looks correct, so you can add

     Reviewed-and-tested-by: Kjetil Barvik

  if you want to.

  But, I guess it is me which is a litle late to comment things, since I
  already see that all 3 patches is in the pu, next and master branches
  already, less than 3 days after beeing posted to the malinglist.

  But, would'nt it be a good thing to let all patches at least be in the
  pu branch for minimum x days before entering next and master?  Or: let
  it go minimum x days after beeing posted to the list before entering
  the next and master branch?  x = 4?

  Since the patches is already in master and next, I guess it is not as
  easy as if the patche(es) has been in pu to make a new version of a
  patch, since both master and next is expected to be fast-forward
  branches.

  -- kjetil, which was too late this time, too  :-)