I wrote:
> So I went through the psql commands which don't fail on parse errors
> for booleans
> [...]
Here's a v5 patch implementing the suggestions mentioned upthread:
all meta-commands calling ParseVariableBool() now fail
when the boolean argument can't be parsed successfully.
Also includes a minor change to SetVariableAssignHook() that now
returns the result of the hook it calls after installing it.
It doesn't make any difference in psql behavior since callers
of SetVariableAssignHook() ignore its return value, but it's
more consistent with SetVariable().
Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..356467b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,25 +254,30 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1)
== 0)
{
reuse_previous =
- ParseVariableBool(opt1 + sizeof(prefix) - 1,
prefix) ?
+ ParseVariableBool(opt1 + sizeof(prefix) - 1,
prefix, &success) ?
TRI_YES : TRI_NO;
-
- free(opt1);
- opt1 = read_connect_arg(scan_state);
+ if (success)
+ {
+ free(opt1);
+ opt1 = read_connect_arg(scan_state);
+ }
}
else
reuse_previous = TRI_DEFAULT;
- opt2 = read_connect_arg(scan_state);
- opt3 = read_connect_arg(scan_state);
- opt4 = read_connect_arg(scan_state);
+ if (success) /* do not attempt to connect if
reuse_previous argument was invalid */
+ {
+ opt2 = read_connect_arg(scan_state);
+ opt3 = read_connect_arg(scan_state);
+ opt4 = read_connect_arg(scan_state);
- success = do_connect(reuse_previous, opt1, opt2, opt3, opt4);
+ success = do_connect(reuse_previous, opt1, opt2, opt3,
opt4);
+ free(opt2);
+ free(opt3);
+ free(opt4);
+ }
free(opt1);
- free(opt2);
- free(opt3);
- free(opt4);
}
/* \cd */
@@ -1548,7 +1553,11 @@ exec_command(const char *cmd,
OT_NORMAL, NULL, false);
if (opt)
- pset.timing = ParseVariableBool(opt, "\\timing");
+ {
+ bool newval = ParseVariableBool(opt, "\\timing",
&success);
+ if (success)
+ pset.timing = newval;
+ }
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2669,14 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
- popt->topt.expanded = ParseVariableBool(value, param);
+ {
+ bool valid;
+ bool newval = ParseVariableBool(value, param,
&valid);
+ if (valid)
+ popt->topt.expanded = newval;
+ else
+ return false;
+ }
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2668,8 +2684,16 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
/* locale-aware numeric output */
else if (strcmp(param, "numericlocale") == 0)
{
+
if (value)
- popt->topt.numericLocale = ParseVariableBool(value,
param);
+ {
+ bool valid;
+ bool newval = ParseVariableBool(value, param,
&valid);
+ if (valid)
+ popt->topt.numericLocale = newval;
+ else
+ return false;
+ }
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2748,14 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
- popt->topt.tuples_only = ParseVariableBool(value,
param);
+ {
+ bool valid;
+ bool newval = ParseVariableBool(value, param,
&valid);
+ if (valid)
+ popt->topt.tuples_only = newval;
+ else
+ return false;
+ }
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
@@ -2756,10 +2787,11 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
popt->topt.pager = 2;
else if (value)
{
- if (ParseVariableBool(value, param))
- popt->topt.pager = 1;
- else
- popt->topt.pager = 0;
+ bool valid;
+ bool on = ParseVariableBool(value, param, &valid);
+ if (!valid)
+ return false;
+ popt->topt.pager = on ? 1 : 0;
}
else if (popt->topt.pager == 1)
popt->topt.pager = 0;
@@ -2778,7 +2810,14 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "footer") == 0)
{
if (value)
- popt->topt.default_footer = ParseVariableBool(value,
param);
+ {
+ bool valid;
+ bool newval = ParseVariableBool(value, param,
&valid);
+ if (valid)
+ popt->topt.default_footer = newval;
+ else
+ return false;
+ }
else
popt->topt.default_footer = !popt->topt.default_footer;
}
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7ce05fb..ba094f0 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,68 @@ showVersion(void)
* This isn't an amazingly good place for them, but neither is anywhere else.
*/
-static void
+/*
+ * Hook to set an internal flag from a user-supplied string value.
+ * If the syntax is correct, affect *flag and return true.
+ * Otherwise, keep *flag untouched and return false.
+ */
+static bool
+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
+{
+ bool isvalid;
+ bool val = ParseVariableBool(newval, varname, &isvalid);
+ if (isvalid)
+ *flag = val;
+ return isvalid;
+}
+
+static bool
autocommit_hook(const char *newval)
{
- pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+ return generic_boolean_hook(newval, "AUTOCOMMIT", &pset.autocommit);
}
-static void
+static bool
on_error_stop_hook(const char *newval)
{
- pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
+ return generic_boolean_hook(newval, "ON_ERROR_STOP",
&pset.on_error_stop);
}
-static void
+static bool
quiet_hook(const char *newval)
{
- pset.quiet = ParseVariableBool(newval, "QUIET");
+ return generic_boolean_hook(newval, "QUIET", &pset.quiet);
}
-static void
+static bool
singleline_hook(const char *newval)
{
- pset.singleline = ParseVariableBool(newval, "SINGLELINE");
+ return generic_boolean_hook(newval, "SINGLELINE", &pset.singleline);
}
-static void
+static bool
singlestep_hook(const char *newval)
{
- pset.singlestep = ParseVariableBool(newval, "SINGLESTEP");
+ return generic_boolean_hook(newval, "SINGLESTEP", &pset.singlestep);
}
-static void
+static bool
fetch_count_hook(const char *newval)
{
- pset.fetch_count = ParseVariableNum(newval, -1, -1, false);
+ bool valid;
+ int value = ParseCheckVariableNum(newval, -1, &valid);
+ if (valid)
+ pset.fetch_count = value;
+ else
+ {
+ psql_error("invalid value \"%s\" for \"%s\"\n",
+ newval, "FETCH_COUNT");
+ return false;
+ }
+ return true;
}
-static void
+static bool
echo_hook(const char *newval)
{
if (newval == NULL)
@@ -837,39 +862,52 @@ echo_hook(const char *newval)
pset.echo = PSQL_ECHO_NONE;
else
{
- psql_error("unrecognized value \"%s\" for \"%s\"; assuming
\"%s\"\n",
- newval, "ECHO", "none");
- pset.echo = PSQL_ECHO_NONE;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "ECHO");
+ return false;
}
+ return true;
}
-static void
+static bool
echo_hidden_hook(const char *newval)
{
if (newval == NULL)
pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
else if (pg_strcasecmp(newval, "noexec") == 0)
pset.echo_hidden = PSQL_ECHO_HIDDEN_NOEXEC;
- else if (ParseVariableBool(newval, "ECHO_HIDDEN"))
- pset.echo_hidden = PSQL_ECHO_HIDDEN_ON;
- else /* ParseVariableBool printed msg if needed */
- pset.echo_hidden = PSQL_ECHO_HIDDEN_OFF;
+ else
+ {
+ bool isvalid;
+ bool val = ParseVariableBool(newval, "ECHO_HIDDEN", &isvalid);
+ if (!isvalid)
+ return false; /* ParseVariableBool printed msg */
+ pset.echo_hidden = val ? PSQL_ECHO_HIDDEN_ON :
PSQL_ECHO_HIDDEN_OFF;
+ }
+ return true;
}
-static void
+static bool
on_error_rollback_hook(const char *newval)
{
if (newval == NULL)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
else if (pg_strcasecmp(newval, "interactive") == 0)
pset.on_error_rollback = PSQL_ERROR_ROLLBACK_INTERACTIVE;
- else if (ParseVariableBool(newval, "ON_ERROR_ROLLBACK"))
- pset.on_error_rollback = PSQL_ERROR_ROLLBACK_ON;
- else /* ParseVariableBool printed msg if needed */
- pset.on_error_rollback = PSQL_ERROR_ROLLBACK_OFF;
+ else
+ {
+ bool isvalid;
+ bool val = ParseVariableBool(newval, "ON_ERROR_ROLLBACK",
&isvalid);
+ if (isvalid)
+ pset.on_error_rollback = val ? PSQL_ERROR_ROLLBACK_ON :
PSQL_ERROR_ROLLBACK_OFF;
+ else
+ /* ParseVariableBool printed msg if needed */
+ return false;
+ }
+ return true;
}
-static void
+static bool
comp_keyword_case_hook(const char *newval)
{
if (newval == NULL)
@@ -884,13 +922,14 @@ comp_keyword_case_hook(const char *newval)
pset.comp_case = PSQL_COMP_CASE_LOWER;
else
{
- psql_error("unrecognized value \"%s\" for \"%s\"; assuming
\"%s\"\n",
- newval, "COMP_KEYWORD_CASE",
"preserve-upper");
- pset.comp_case = PSQL_COMP_CASE_PRESERVE_UPPER;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "COMP_KEYWORD_CASE");
+ return false;
}
+ return true;
}
-static void
+static bool
histcontrol_hook(const char *newval)
{
if (newval == NULL)
@@ -905,31 +944,35 @@ histcontrol_hook(const char *newval)
pset.histcontrol = hctl_none;
else
{
- psql_error("unrecognized value \"%s\" for \"%s\"; assuming
\"%s\"\n",
- newval, "HISTCONTROL", "none");
- pset.histcontrol = hctl_none;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "HISTCONTROL");
+ return false;
}
+ return true;
}
-static void
+static bool
prompt1_hook(const char *newval)
{
pset.prompt1 = newval ? newval : "";
+ return true;
}
-static void
+static bool
prompt2_hook(const char *newval)
{
pset.prompt2 = newval ? newval : "";
+ return true;
}
-static void
+static bool
prompt3_hook(const char *newval)
{
pset.prompt3 = newval ? newval : "";
+ return true;
}
-static void
+static bool
verbosity_hook(const char *newval)
{
if (newval == NULL)
@@ -942,16 +985,17 @@ verbosity_hook(const char *newval)
pset.verbosity = PQERRORS_VERBOSE;
else
{
- psql_error("unrecognized value \"%s\" for \"%s\"; assuming
\"%s\"\n",
- newval, "VERBOSITY", "default");
- pset.verbosity = PQERRORS_DEFAULT;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "VERBOSITY");
+ return false;
}
if (pset.db)
PQsetErrorVerbosity(pset.db, pset.verbosity);
+ return true;
}
-static void
+static bool
show_context_hook(const char *newval)
{
if (newval == NULL)
@@ -964,13 +1008,14 @@ show_context_hook(const char *newval)
pset.show_context = PQSHOW_CONTEXT_ALWAYS;
else
{
- psql_error("unrecognized value \"%s\" for \"%s\"; assuming
\"%s\"\n",
- newval, "SHOW_CONTEXT", "errors");
- pset.show_context = PQSHOW_CONTEXT_ERRORS;
+ psql_error("unrecognized value \"%s\" for \"%s\"\n",
+ newval, "SHOW_CONTEXT");
+ return false;
}
if (pset.db)
PQsetErrorContextVisibility(pset.db, pset.show_context);
+ return true;
}
diff --git a/src/bin/psql/variables.c b/src/bin/psql/variables.c
index f43f418..1302a32 100644
--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -86,24 +86,28 @@ GetVariable(VariableSpace space, const char *name)
*
* "name" is the name of the variable we're assigning to, to use in error
* report if any. Pass name == NULL to suppress the error report.
+ *
+ * "*valid" reports whether "value" was syntactically valid, unless valid ==
NULL
*/
bool
-ParseVariableBool(const char *value, const char *name)
+ParseVariableBool(const char *value, const char *name, bool *valid)
{
size_t len;
+ if (valid)
+ *valid = true;
if (value == NULL)
return false; /* not set -> assume "off" */
len = strlen(value);
- if (pg_strncasecmp(value, "true", len) == 0)
+ if (len > 0 && pg_strncasecmp(value, "true", len) == 0)
return true;
- else if (pg_strncasecmp(value, "false", len) == 0)
+ else if (len > 0 && pg_strncasecmp(value, "false", len) == 0)
return false;
- else if (pg_strncasecmp(value, "yes", len) == 0)
+ else if (len > 0 && pg_strncasecmp(value, "yes", len) == 0)
return true;
- else if (pg_strncasecmp(value, "no", len) == 0)
+ else if (len > 0 && pg_strncasecmp(value, "no", len) == 0)
return false;
/* 'o' is not unique enough */
else if (pg_strncasecmp(value, "on", (len > 2 ? len : 2)) == 0)
@@ -116,10 +120,16 @@ ParseVariableBool(const char *value, const char *name)
return false;
else
{
- /* NULL is treated as false, so a non-matching value is 'true'
*/
+ /*
+ * NULL is treated as false, so a non-matching value is 'true'.
+ * A caller that cares about syntactic conformance should
+ * check *valid to know whether the value was recognized.
+ */
if (name)
- psql_error("unrecognized value \"%s\" for \"%s\";
assuming \"%s\"\n",
- value, name, "on");
+ psql_error("unrecognized value \"%s\" for \"%s\":
boolean expected\n",
+ value, name);
+ if (valid)
+ *valid = false;
return true;
}
}
@@ -154,6 +164,35 @@ ParseVariableNum(const char *val,
return result;
}
+/*
+ * Read numeric variable, or defaultval if it is not set.
+ * Trailing contents are not allowed.
+ * "valid" points to a bool reporting whether the value was valid.
+ */
+int
+ParseCheckVariableNum(const char *val,
+ int defaultval,
+ bool *valid)
+{
+ int result;
+
+ if (!val)
+ {
+ *valid = true;
+ result = defaultval;
+ }
+ else
+ {
+ char *end;
+
+ errno = 0;
+ result = strtol(val, &end, 0);
+ *valid = (val[0] != '\0' && errno == 0 && *end == '\0');
+ }
+
+ return result;
+}
+
int
GetVariableNum(VariableSpace space,
const char *name,
@@ -205,13 +244,26 @@ SetVariable(VariableSpace space, const char *name, const
char *value)
{
if (strcmp(current->name, name) == 0)
{
- /* found entry, so update */
- if (current->value)
- free(current->value);
- current->value = pg_strdup(value);
- if (current->assign_hook)
- (*current->assign_hook) (current->value);
- return true;
+ /*
+ * Found entry, so update, unless a hook returns false.
+ * The hook needs the value in a buffer with the same
lifespan
+ * as the variable, so allocate it right away, even if
it needs
+ * to be freed just after when the hook returns false.
+ */
+ char *new_value = pg_strdup(value);
+
+ bool confirmed = current->assign_hook ?
+ (*current->assign_hook)
(new_value) : true;
+
+ if (confirmed)
+ {
+ pg_free(current->value);
+ current->value = new_value;
+ }
+ else
+ pg_free(new_value); /* and current->value is
left unchanged */
+
+ return confirmed;
}
}
@@ -248,8 +300,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
{
/* found entry, so update */
current->assign_hook = hook;
- (*hook) (current->value);
- return true;
+ return (*hook) (current->value);
}
}
@@ -260,8 +311,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
current->assign_hook = hook;
current->next = NULL;
previous->next = current;
- (*hook) (NULL);
- return true;
+ return (*hook) (NULL);
}
bool
diff --git a/src/bin/psql/variables.h b/src/bin/psql/variables.h
index d7a05a1..38396ba 100644
--- a/src/bin/psql/variables.h
+++ b/src/bin/psql/variables.h
@@ -20,7 +20,7 @@
* Note: if value == NULL then the variable is logically unset, but we are
* keeping the struct around so as not to forget about its hook function.
*/
-typedef void (*VariableAssignHook) (const char *newval);
+typedef bool (*VariableAssignHook) (const char *newval);
struct _variable
{
@@ -35,11 +35,14 @@ typedef struct _variable *VariableSpace;
VariableSpace CreateVariableSpace(void);
const char *GetVariable(VariableSpace space, const char *name);
-bool ParseVariableBool(const char *value, const char *name);
+bool ParseVariableBool(const char *value, const char *name, bool
*valid);
int ParseVariableNum(const char *val,
int defaultval,
int faultval,
bool allowtrail);
+int ParseCheckVariableNum(const char *val,
+ int defaultval,
+ bool *valid);
int GetVariableNum(VariableSpace space,
const char *name,
int defaultval,
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
