Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2014, 23:26 UTC
Message-ID: <xmqqk3cq7kgm.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1392730814-19656-2-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 9 quoted lines

> We've been avoiding PATH_MAX whenever possible. This patch makes
> get_pathname() return a strbuf and updates the callers to take
> advantage of this. The code is simplified as we no longer need to
> worry about buffer overflow.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  path.c | 119 +++++++++++++++++++++++++++--------------------------------------
>  1 file changed, 50 insertions(+), 69 deletions(-)

Nice.

Show 12 quoted lines

>  char *git_pathdup(const char *fmt, ...)
>  {
> -	char path[PATH_MAX], *ret;
> +	struct strbuf *path = get_pathname();
>  	va_list args;
>  	va_start(args, fmt);
> -	ret = vsnpath(path, sizeof(path), fmt, args);
> +	vsnpath(path, fmt, args);
>  	va_end(args);
> -	return xstrdup(ret);
> +	return strbuf_detach(path, NULL);
>  }

This feels somewhat wrong.

This function is for callers who are willing to take ownership of the path buffer and promise to free the returned buffer when they are done, because you are returning strbuf_detach()'ed piece of memory, giving the ownership away.

The whole point of using get_pathname() is to allow callers not to care about allocation issues on the paths they scribble on during their short-and-simple codepaths that do not have too many uses of similar temporary path buffers. Why borrow from that round-robin pool (which may now cause some codepaths to overflow the number of such active temporary path buffers---have they been all audited)?

Is there a reason not to do just an equivalent of

    #define git_pathdup mkpathdup

and be done with it?  Am I missing something?

Show 33 quoted lines

>  char *mkpathdup(const char *fmt, ...)
>  {
> -	char *path;
>  	struct strbuf sb = STRBUF_INIT;
>  	va_list args;
> -
>  	va_start(args, fmt);
>  	strbuf_vaddf(&sb, fmt, args);
>  	va_end(args);
> -	path = xstrdup(cleanup_path(sb.buf));
> -
> -	strbuf_release(&sb);
> -	return path;
> +	strbuf_cleanup_path(&sb);
> +	return strbuf_detach(&sb, NULL);
>  }
>  
>  char *mkpath(const char *fmt, ...)
>  {
>  	va_list args;
> -	unsigned len;
> -	char *pathname = get_pathname();
> -
> +	struct strbuf *pathname = get_pathname();
>  	va_start(args, fmt);
> -	len = vsnprintf(pathname, PATH_MAX, fmt, args);
> +	strbuf_vaddf(pathname, fmt, args);
>  	va_end(args);
> -	if (len >= PATH_MAX)
> -		return bad_path;
> -	return cleanup_path(pathname);
> +	return cleanup_path(pathname->buf);
>  }

On the other hand, this one does seem correct.

Show 14 quoted lines

>  char *git_path(const char *fmt, ...)
>  {
> -	char *pathname = get_pathname();
> +	struct strbuf *pathname = get_pathname();
>  	va_list args;
> -	char *ret;
> -
>  	va_start(args, fmt);
> -	ret = vsnpath(pathname, PATH_MAX, fmt, args);
> +	vsnpath(pathname, fmt, args);
>  	va_end(args);
> -	return ret;
> +	return pathname->buf;
>  }

So does this.

Thanks.