[Top][All Lists]

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

Re: [PATCH] stat: don't explicitly request file size for filenames

From: Andreas Dilger
Subject: Re: [PATCH] stat: don't explicitly request file size for filenames
Date: Thu, 4 Jul 2019 23:19:04 +0000

On Jul 4, 2019, at 04:44, Pádraig Brady <address@hidden> wrote:
> diff --git a/lib/areadlink-with-size.c b/lib/areadlink-with-size.c
> index eacad3f..2fbe51c 100644
> --- a/lib/areadlink-with-size.c
> +++ b/lib/areadlink-with-size.c
> @@ -36,14 +36,15 @@
>     check, so it's OK to guess too small on hosts where there is no
>     arbitrary limit to symbolic link length.  */
>  #ifndef SYMLINK_MAX
> -# define SYMLINK_MAX 1024
> +# define SYMLINK_MAX 1023
>  #endif
>  /* Call readlink to get the symbolic link value of FILE.
>     SIZE is a hint as to how long the link is expected to be;
> -   typically it is taken from st_size.  It need not be correct.
> +   typically it is taken from st_size.  It need not be correct,
> +   and a value of 0 (or more than 8Ki) will select an appropriate lower 
> bound.
>     Return a pointer to that NUL-terminated string in malloc'd storage.
>     If readlink fails, malloc fails, or if the link value is longer
>     than SSIZE_MAX, return NULL (caller may use errno to diagnose).  */
> @@ -61,7 +62,7 @@ areadlink_with_size (char const *file, size_t size)
>                            : INITIAL_LIMIT_BOUND);
>    /* The initial buffer size for the link value.  */
> -  size_t buf_size = size < initial_limit ? size + 1 : initial_limit;
> +  size_t buf_size = size && size < initial_limit ? size + 1 : initial_limit;
>    while (1)

I'd thought of something similar with the 1KB limit, but then I was worried if
"ls" or something is saving all of the symlink targets in memory that using a
too-large buffer size would cause excess memory to be allocated for a long time.
The above logic (AFAICS) is intended to limit the *maximum* allocation size if
"size" is a bogus value, but I don't think initial_limit is a good lower bound.

I picked the lower bound of the malloc() (32 bytes = 4 * sizeof(void *)) as
documented in
My version of the patch (merged with your comments) using this size is attached.

My stats show over 99% of symlinks are under 128 bytes, which isn't _too_ much
to allocate for every file.  If we want this code to very efficient (minimize
readlink() calls) at the cost of some RAM, it would be enough to use 127 bytes
as the starting point.

Cheers, Andreas

Attachment: 0001-areadlink_with_size-handle-size-0-better.patch
Description: 0001-areadlink_with_size-handle-size-0-better.patch

reply via email to

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