Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer
Duy Nguyen pclo...@gmail.com writes:
On Thu, Feb 20, 2014 at 6:26 AM, Junio C Hamano gits...@pobox.com wrote:
Is there a reason not to do just an equivalent of
#define git_pathdup mkpathdup
and be done with it? Am I missing something?
They have a subtle difference: mkpathdup() calls cleanup_path() while
git_pathdup() does not. Without auditing all call sites, we can't be
sure this difference is insignificant. So I keep both functions
separate for now.
Thanks; that is a very good explanation why #define'ing two to be
the same would not be a workable solution to the ownership issue I
raised.
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)?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer
On Thu, Feb 20, 2014 at 6:26 AM, Junio C Hamano gits...@pobox.com wrote: Is there a reason not to do just an equivalent of #define git_pathdup mkpathdup and be done with it? Am I missing something? They have a subtle difference: mkpathdup() calls cleanup_path() while git_pathdup() does not. Without auditing all call sites, we can't be sure this difference is insignificant. So I keep both functions separate for now. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: (Only nitpicks during this round of review). -static char *get_pathname(void) +static struct strbuf *get_pathname() static struct strbuf *get_pathname(void) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
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 pclo...@gmail.com
---
path.c | 119
+++--
1 file changed, 50 insertions(+), 69 deletions(-)
Nice.
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?
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.
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer
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 pclo...@gmail.com
---
path.c | 119 +++--
1 file changed, 50 insertions(+), 69 deletions(-)
diff --git a/path.c b/path.c
index 24594c4..1a1b784 100644
--- a/path.c
+++ b/path.c
@@ -16,11 +16,15 @@ static int get_st_mode_bits(const char *path, int *mode)
static char bad_path[] = /bad-path/;
-static char *get_pathname(void)
+static struct strbuf *get_pathname()
{
- static char pathname_array[4][PATH_MAX];
+ static struct strbuf pathname_array[4] = {
+ STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
+ };
static int index;
- return pathname_array[3 ++index];
+ struct strbuf *sb = pathname_array[3 ++index];
+ strbuf_reset(sb);
+ return sb;
}
static char *cleanup_path(char *path)
@@ -34,6 +38,13 @@ static char *cleanup_path(char *path)
return path;
}
+static void strbuf_cleanup_path(struct strbuf *sb)
+{
+ char *path = cleanup_path(sb-buf);
+ if (path sb-buf)
+ strbuf_remove(sb, 0, path - sb-buf);
+}
+
char *mksnpath(char *buf, size_t n, const char *fmt, ...)
{
va_list args;
@@ -49,85 +60,69 @@ char *mksnpath(char *buf, size_t n, const char *fmt, ...)
return cleanup_path(buf);
}
-static char *vsnpath(char *buf, size_t n, const char *fmt, va_list args)
+static void vsnpath(struct strbuf *buf, const char *fmt, va_list args)
{
const char *git_dir = get_git_dir();
- size_t len;
-
- len = strlen(git_dir);
- if (n len + 1)
- goto bad;
- memcpy(buf, git_dir, len);
- if (len !is_dir_sep(git_dir[len-1]))
- buf[len++] = '/';
- len += vsnprintf(buf + len, n - len, fmt, args);
- if (len = n)
- goto bad;
- return cleanup_path(buf);
-bad:
- strlcpy(buf, bad_path, n);
- return buf;
+ strbuf_addstr(buf, git_dir);
+ if (buf-len !is_dir_sep(buf-buf[buf-len - 1]))
+ strbuf_addch(buf, '/');
+ strbuf_vaddf(buf, fmt, args);
+ strbuf_cleanup_path(buf);
}
char *git_snpath(char *buf, size_t n, const char *fmt, ...)
{
- char *ret;
+ struct strbuf *sb = get_pathname();
va_list args;
va_start(args, fmt);
- ret = vsnpath(buf, n, fmt, args);
+ vsnpath(sb, fmt, args);
va_end(args);
- return ret;
+ if (sb-len = n)
+ strlcpy(buf, bad_path, n);
+ else
+ memcpy(buf, sb-buf, sb-len + 1);
+ return buf;
}
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);
}
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);
}
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;
}
void home_config_paths(char **global, char **xdg, char *file)
@@ -158,41 +153,27 @@ void home_config_paths(char **global, char **xdg, char
*file)
char *git_path_submodule(const char *path, const char *fmt, ...)
{
- char *pathname = get_pathname();
- struct strbuf buf = STRBUF_INIT;
+ struct strbuf *buf = get_pathname();
const char *git_dir;
va_list args;
- unsigned len;
-
- len = strlen(path);
- if (len PATH_MAX-100)
- return bad_path;
- strbuf_addstr(buf, path);
- if (len path[len-1] !=
