Re: [PATCH 6/3] Export thread-safe version of 'has_symlink_leading_path()'
Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 67 quoted lines
> 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.
Show 8 quoted lines
> +}
> +
> +/*
> + * 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".
Show 7 quoted lines
> - 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 :-)