Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
> Note that starting with commit 67a472d71c98 you can use pg_get_line and
> not worry about the hard part of this anymore :-)
pg_get_line as it stands isn't quite suitable, because it just hands
back a "char *" string, not a StringInfo that you can do further
processing on.
However, I'd already grown a bit dissatisfied with exposing only that
API, because the code 8f8154a50 added to hba.c couldn't use pg_get_line
either, and had to duplicate the logic. So the attached revised patch
splits pg_get_line into two pieces, one with the existing char * API
and one that appends to a caller-provided StringInfo. (hba.c needs the
append-rather-than-reset behavior, and it might be useful elsewhere
too.)
While here, I couldn't resist getting rid of ecpg_filter()'s hard-wired
line length limit too.
This version looks committable to me, though perhaps someone has
further thoughts?
regards, tom lane
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 5991a21cf2..9f106653f3 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -502,33 +502,8 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
/* Collect the next input line, handling backslash continuations */
resetStringInfo(&buf);
- while (!feof(file) && !ferror(file))
+ while (pg_get_line_append(file, &buf))
{
- /* Make sure there's a reasonable amount of room in the buffer */
- enlargeStringInfo(&buf, 128);
-
- /* Read some data, appending it to what we already have */
- if (fgets(buf.data + buf.len, buf.maxlen - buf.len, file) == NULL)
- {
- int save_errno = errno;
-
- if (!ferror(file))
- break; /* normal EOF */
- /* I/O error! */
- ereport(elevel,
- (errcode_for_file_access(),
- errmsg("could not read file \"%s\": %m", filename)));
- err_msg = psprintf("could not read file \"%s\": %s",
- filename, strerror(save_errno));
- resetStringInfo(&buf);
- break;
- }
- buf.len += strlen(buf.data + buf.len);
-
- /* If we haven't got a whole line, loop to read more */
- if (!(buf.len > 0 && buf.data[buf.len - 1] == '\n'))
- continue;
-
/* Strip trailing newline, including \r in case we're on Windows */
buf.len = pg_strip_crlf(buf.data);
@@ -551,6 +526,19 @@ tokenize_file(const char *filename, FILE *file, List **tok_lines, int elevel)
break;
}
+ if (ferror(file))
+ {
+ /* I/O error! */
+ int save_errno = errno;
+
+ ereport(elevel,
+ (errcode_for_file_access(),
+ errmsg("could not read file \"%s\": %m", filename)));
+ err_msg = psprintf("could not read file \"%s\": %s",
+ filename, strerror(save_errno));
+ break;
+ }
+
/* Parse fields */
lineptr = buf.data;
while (*lineptr && err_msg == NULL)
diff --git a/src/common/pg_get_line.c b/src/common/pg_get_line.c
index 38433675d4..eae02b9596 100644
--- a/src/common/pg_get_line.c
+++ b/src/common/pg_get_line.c
@@ -49,21 +49,7 @@ pg_get_line(FILE *stream)
initStringInfo(&buf);
- /* Read some data, appending it to whatever we already have */
- while (fgets(buf.data + buf.len, buf.maxlen - buf.len, stream) != NULL)
- {
- buf.len += strlen(buf.data + buf.len);
-
- /* Done if we have collected a newline */
- if (buf.len > 0 && buf.data[buf.len - 1] == '\n')
- return buf.data;
-
- /* Make some more room in the buffer, and loop to read more data */
- enlargeStringInfo(&buf, 128);
- }
-
- /* Did fgets() fail because of an I/O error? */
- if (ferror(stream))
+ if (!pg_get_line_append(stream, &buf))
{
/* ensure that free() doesn't mess up errno */
int save_errno = errno;
@@ -73,13 +59,49 @@ pg_get_line(FILE *stream)
return NULL;
}
- /* If we read no data before reaching EOF, we should return NULL */
- if (buf.len == 0)
+ return buf.data;
+}
+
+/*
+ * pg_get_line_append()
+ *
+ * This has similar behavior to pg_get_line(), and thence to fgets(),
+ * except that the collected data is appended to whatever is in *buf.
+ *
+ * Returns true if a line was successfully collected (including the
+ * case of a non-newline-terminated line at EOF). Returns false if
+ * there was an I/O error or no data was available before EOF.
+ * (Check ferror(stream) to distinguish these cases.)
+ *
+ * In the false-result case, the contents of *buf are logically unmodified,
+ * though it's possible that the buffer has been resized.
+ */
+bool
+pg_get_line_append(FILE *stream, StringInfo buf)
+{
+ int orig_len = buf->len;
+
+ /* Read some data, appending it to whatever we already have */
+ while (fgets(buf->data + buf->len, buf->maxlen - buf->len, stream) != NULL)
{
- pfree(buf.data);
- return NULL;
+ buf->len += strlen(buf->data + buf->len);
+
+ /* Done if we have collected a newline */
+ if (buf->len > orig_len && buf->data[buf->len - 1] == '\n')
+ return true;
+
+ /* Make some more room in the buffer, and loop to read more data */
+ enlargeStringInfo(buf, 128);
}
- /* No newline at EOF ... so return what we have */
- return buf.data;
+ /* Check for I/O errors and EOF */
+ if (ferror(stream) || buf->len == orig_len)
+ {
+ /* Discard any data we collected before detecting error */
+ buf->len = orig_len;
+ return false;
+ }
+
+ /* No newline at EOF, but we did collect some data */
+ return true;
}
diff --git a/src/include/common/string.h b/src/include/common/string.h
index 18aa1dc5aa..50c241a811 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -10,6 +10,8 @@
#ifndef COMMON_STRING_H
#define COMMON_STRING_H
+struct StringInfoData; /* avoid including stringinfo.h here */
+
/* functions in src/common/string.c */
extern bool pg_str_endswith(const char *str, const char *end);
extern int strtoint(const char *pg_restrict str, char **pg_restrict endptr,
@@ -19,6 +21,7 @@ extern int pg_strip_crlf(char *str);
/* functions in src/common/pg_get_line.c */
extern char *pg_get_line(FILE *stream);
+extern bool pg_get_line_append(FILE *stream, struct StringInfoData *buf);
/* functions in src/common/sprompt.c */
extern char *simple_prompt(const char *prompt, bool echo);
diff --git a/src/interfaces/ecpg/test/pg_regress_ecpg.c b/src/interfaces/ecpg/test/pg_regress_ecpg.c
index 46b9e78fe5..a2d7b70d9a 100644
--- a/src/interfaces/ecpg/test/pg_regress_ecpg.c
+++ b/src/interfaces/ecpg/test/pg_regress_ecpg.c
@@ -19,8 +19,9 @@
#include "postgres_fe.h"
#include "pg_regress.h"
+#include "common/string.h"
+#include "lib/stringinfo.h"
-#define LINEBUFSIZE 300
static void
ecpg_filter(const char *sourcefile, const char *outfile)
@@ -31,7 +32,7 @@ ecpg_filter(const char *sourcefile, const char *outfile)
*/
FILE *s,
*t;
- char linebuf[LINEBUFSIZE];
+ StringInfoData linebuf;
s = fopen(sourcefile, "r");
if (!s)
@@ -46,13 +47,14 @@ ecpg_filter(const char *sourcefile, const char *outfile)
exit(2);
}
- while (fgets(linebuf, LINEBUFSIZE, s))
+ initStringInfo(&linebuf);
+
+ while (pg_get_line_append(s, &linebuf))
{
/* check for "#line " in the beginning */
- if (strstr(linebuf, "#line ") == linebuf)
+ if (strstr(linebuf.data, "#line ") == linebuf.data)
{
- char *p = strchr(linebuf, '"');
- char *n;
+ char *p = strchr(linebuf.data, '"');
int plen = 1;
while (*p && (*(p + plen) == '.' || strchr(p + plen, '/') != NULL))
@@ -62,13 +64,15 @@ ecpg_filter(const char *sourcefile, const char *outfile)
/* plen is one more than the number of . and / characters */
if (plen > 1)
{
- n = (char *) malloc(plen);
- strlcpy(n, p + 1, plen);
- replace_string(linebuf, n, "");
+ memmove(p + 1, p + plen, strlen(p + plen) + 1);
+ /* we don't bother to fix up linebuf.len */
}
}
- fputs(linebuf, t);
+ fputs(linebuf.data, t);
+ resetStringInfo(&linebuf);
}
+
+ pfree(linebuf.data);
fclose(s);
fclose(t);
}
@@ -87,40 +91,42 @@ ecpg_start_test(const char *testname,
PID_TYPE pid;
char inprg[MAXPGPATH];
char insource[MAXPGPATH];
- char *outfile_stdout,
+ StringInfoData testname_dash;
+ char outfile_stdout[MAXPGPATH],
expectfile_stdout[MAXPGPATH];
- char *outfile_stderr,
+ char outfile_stderr[MAXPGPATH],
expectfile_stderr[MAXPGPATH];
- char *outfile_source,
+ char outfile_source[MAXPGPATH],
expectfile_source[MAXPGPATH];
char cmd[MAXPGPATH * 3];
- char *testname_dash;
char *appnameenv;
snprintf(inprg, sizeof(inprg), "%s/%s", inputdir, testname);
+ snprintf(insource, sizeof(insource), "%s.c", testname);
+
+ initStringInfo(&testname_dash);
+ appendStringInfoString(&testname_dash, testname);
+ replace_string(&testname_dash, "/", "-");
- testname_dash = strdup(testname);
- replace_string(testname_dash, "/", "-");
snprintf(expectfile_stdout, sizeof(expectfile_stdout),
"%s/expected/%s.stdout",
- outputdir, testname_dash);
+ outputdir, testname_dash.data);
snprintf(expectfile_stderr, sizeof(expectfile_stderr),
"%s/expected/%s.stderr",
- outputdir, testname_dash);
+ outputdir, testname_dash.data);
snprintf(expectfile_source, sizeof(expectfile_source),
"%s/expected/%s.c",
- outputdir, testname_dash);
-
- /*
- * We can use replace_string() here because the replacement string does
- * not occupy more space than the replaced one.
- */
- outfile_stdout = strdup(expectfile_stdout);
- replace_string(outfile_stdout, "/expected/", "/results/");
- outfile_stderr = strdup(expectfile_stderr);
- replace_string(outfile_stderr, "/expected/", "/results/");
- outfile_source = strdup(expectfile_source);
- replace_string(outfile_source, "/expected/", "/results/");
+ outputdir, testname_dash.data);
+
+ snprintf(outfile_stdout, sizeof(outfile_stdout),
+ "%s/results/%s.stdout",
+ outputdir, testname_dash.data);
+ snprintf(outfile_stderr, sizeof(outfile_stderr),
+ "%s/results/%s.stderr",
+ outputdir, testname_dash.data);
+ snprintf(outfile_source, sizeof(outfile_source),
+ "%s/results/%s.c",
+ outputdir, testname_dash.data);
add_stringlist_item(resultfiles, outfile_stdout);
add_stringlist_item(expectfiles, expectfile_stdout);
@@ -134,18 +140,15 @@ ecpg_start_test(const char *testname,
add_stringlist_item(expectfiles, expectfile_source);
add_stringlist_item(tags, "source");
- snprintf(insource, sizeof(insource), "%s.c", testname);
ecpg_filter(insource, outfile_source);
- snprintf(inprg, sizeof(inprg), "%s/%s", inputdir, testname);
-
snprintf(cmd, sizeof(cmd),
"\"%s\" >\"%s\" 2>\"%s\"",
inprg,
outfile_stdout,
outfile_stderr);
- appnameenv = psprintf("PGAPPNAME=ecpg/%s", testname_dash);
+ appnameenv = psprintf("PGAPPNAME=ecpg/%s", testname_dash.data);
putenv(appnameenv);
pid = spawn_process(cmd);
@@ -160,10 +163,7 @@ ecpg_start_test(const char *testname,
unsetenv("PGAPPNAME");
free(appnameenv);
- free(testname_dash);
- free(outfile_stdout);
- free(outfile_stderr);
- free(outfile_source);
+ free(testname_dash.data);
return pid;
}
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index d82e0189dc..d83442f467 100644
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -31,8 +31,10 @@
#include "common/logging.h"
#include "common/restricted_token.h"
+#include "common/string.h"
#include "common/username.h"
#include "getopt_long.h"
+#include "lib/stringinfo.h"
#include "libpq/pqcomm.h" /* needed for UNIXSOCK_PATH() */
#include "pg_config_paths.h"
#include "pg_regress.h"
@@ -435,22 +437,32 @@ string_matches_pattern(const char *str, const char *pattern)
}
/*
- * Replace all occurrences of a string in a string with a different string.
- * NOTE: Assumes there is enough room in the target buffer!
+ * Replace all occurrences of "replace" in "string" with "replacement".
+ * The StringInfo will be suitably enlarged if necessary.
+ *
+ * Note: this is optimized on the assumption that most calls will find
+ * no more than one occurrence of "replace", and quite likely none.
*/
void
-replace_string(char *string, const char *replace, const char *replacement)
+replace_string(StringInfo string, const char *replace, const char *replacement)
{
+ int pos = 0;
char *ptr;
- while ((ptr = strstr(string, replace)) != NULL)
+ while ((ptr = strstr(string->data + pos, replace)) != NULL)
{
- char *dup = pg_strdup(string);
+ /* Must copy the remainder of the string out of the StringInfo */
+ char *suffix = pg_strdup(ptr + strlen(replace));
- strlcpy(string, dup, ptr - string + 1);
- strcat(string, replacement);
- strcat(string, dup + (ptr - string) + strlen(replace));
- free(dup);
+ /* Truncate StringInfo at start of found string ... */
+ string->len = ptr - string->data;
+ /* ... and append the replacement */
+ appendStringInfoString(string, replacement);
+ /* Next search should start after the replacement */
+ pos = string->len;
+ /* Put back the remainder of the string */
+ appendStringInfoString(string, suffix);
+ free(suffix);
}
}
@@ -521,7 +533,7 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch
char prefix[MAXPGPATH];
FILE *infile,
*outfile;
- char line[1024];
+ StringInfoData line;
/* reject filenames not finishing in ".source" */
if (strlen(*name) < 8)
@@ -551,15 +563,21 @@ convert_sourcefiles_in(const char *source_subdir, const char *dest_dir, const ch
progname, destfile, strerror(errno));
exit(2);
}
- while (fgets(line, sizeof(line), infile))
+
+ initStringInfo(&line);
+
+ while (pg_get_line_append(infile, &line))
{
- replace_string(line, "@abs_srcdir@", inputdir);
- replace_string(line, "@abs_builddir@", outputdir);
- replace_string(line, "@testtablespace@", testtablespace);
- replace_string(line, "@libdir@", dlpath);
- replace_string(line, "@DLSUFFIX@", DLSUFFIX);
- fputs(line, outfile);
+ replace_string(&line, "@abs_srcdir@", inputdir);
+ replace_string(&line, "@abs_builddir@", outputdir);
+ replace_string(&line, "@testtablespace@", testtablespace);
+ replace_string(&line, "@libdir@", dlpath);
+ replace_string(&line, "@DLSUFFIX@", DLSUFFIX);
+ fputs(line.data, outfile);
+ resetStringInfo(&line);
}
+
+ pfree(line.data);
fclose(infile);
fclose(outfile);
}
diff --git a/src/test/regress/pg_regress.h b/src/test/regress/pg_regress.h
index ee6e0d42f4..726f9c9048 100644
--- a/src/test/regress/pg_regress.h
+++ b/src/test/regress/pg_regress.h
@@ -18,6 +18,8 @@
#define INVALID_PID INVALID_HANDLE_VALUE
#endif
+struct StringInfoData; /* avoid including stringinfo.h here */
+
/* simple list of strings */
typedef struct _stringlist
{
@@ -49,5 +51,6 @@ int regression_main(int argc, char *argv[],
init_function ifunc, test_function tfunc);
void add_stringlist_item(_stringlist **listhead, const char *str);
PID_TYPE spawn_process(const char *cmdline);
-void replace_string(char *string, const char *replace, const char *replacement);
+void replace_string(struct StringInfoData *string,
+ const char *replace, const char *replacement);
bool file_exists(const char *file);
