Hi,
I'm attaching v3 of the patch with error reporting for
FETCH_COUNT as mentioned upthread, and rebased
on the most recent master.
> I was suggesting change on the lines of extending generic_boolean_hook to
> include enum(part in enum_hooks which calls ParseVariableBool) and
> integer parsing as well.
Well, generic_boolean_hook() is meant to change this, for instance:
static void
on_error_stop_hook(const char *newval)
{
pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
}
into that:
static bool
on_error_stop_hook(const char *newval)
{
return generic_boolean_hook(newval, "ON_ERROR_STOP",
&pset.on_error_stop);
}
with the goal that the assignment does not occur if "newval" is bogus.
The change is really minimal.
When we're dealing with enum-or-bool variables, such as for instance
ECHO_HIDDEN, the patch replaces this:
static void
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;
}
with that:
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
{
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;
}
The goal being again to reject a bogus assignment, as opposed to replacing
it with any hardwired value.
Code-wise, we can't call generic_boolean_hook() here because we need
to assign a non-boolean specific value after having parsed the ON/OFF
user-supplied string.
More generally, it turns out that the majority of hooks are concerned
by this patch, as they parse user-supplied values, but there
are 4 distinct categories of variables:
1- purely ON/OFF vars:
AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP
2- ON/OFF mixed with enum values:
ECHO_HIDDEN, ON_ERROR_ROLLBACK
3- purely enum values:
COMP_KEYWORD_CASE, HISTCONTROL, ECHO, VERBOSITY, SHOW_CONTEXT
4- numeric values:
FETCH_COUNT
If you suggest that the patch should refactor the implementation
of hooks for case #2, only two hooks are concerned and they consist
of non-mergeable enum-specific code interleaved with generic code,
so I don't foresee any gain in fusioning. I have the same opinion about
merging any of #1, #2, #3, #4 together.
But feel free to post code implementing your idea if you disagree,
maybe I just don't figure out what you have in mind.
For case #3, these hooks clearly follow a common pattern, but I also
don't see any benefit in an opportunistic rewrite given the nature of
the functions.
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..61b2304 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,7 +254,7 @@ 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, NULL) ?
TRI_YES : TRI_NO;
free(opt1);
@@ -1548,7 +1548,7 @@ exec_command(const char *cmd,
OT_NORMAL, NULL, false);
if (opt)
- pset.timing = ParseVariableBool(opt, "\\timing");
+ pset.timing = ParseVariableBool(opt, "\\timing", NULL);
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2660,7 @@ 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);
+ popt->topt.expanded = ParseVariableBool(value, param,
NULL);
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2669,7 +2669,7 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
- popt->topt.numericLocale = ParseVariableBool(value,
param);
+ popt->topt.numericLocale = ParseVariableBool(value,
param, NULL);
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2724,7 @@ 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);
+ popt->topt.tuples_only = ParseVariableBool(value,
param, NULL);
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
@@ -2756,7 +2756,7 @@ do_pset(const char *param, const char *value,
printQueryOpt *popt, bool quiet)
popt->topt.pager = 2;
else if (value)
{
- if (ParseVariableBool(value, param))
+ if (ParseVariableBool(value, param, NULL))
popt->topt.pager = 1;
else
popt->topt.pager = 0;
@@ -2778,7 +2778,7 @@ 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);
+ popt->topt.default_footer = ParseVariableBool(value,
param, NULL);
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..5e68b67 100644
--- a/src/bin/psql/variables.c
+++ b/src/bin/psql/variables.c
@@ -86,12 +86,16 @@ 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" */
@@ -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,17 @@ 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
*/
+ bool confirmed = current->assign_hook ?
+ (*current->assign_hook)
(value) : true;
+
+ if (confirmed)
+ {
+ if (current->value)
+ free(current->value);
+ current->value = pg_strdup(value);
+ }
+ return confirmed;
}
}
@@ -248,7 +291,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
{
/* found entry, so update */
current->assign_hook = hook;
- (*hook) (current->value);
+ (*hook) (current->value); /* ignore return value */
return true;
}
}
@@ -260,7 +303,7 @@ SetVariableAssignHook(VariableSpace space, const char
*name, VariableAssignHook
current->assign_hook = hook;
current->next = NULL;
previous->next = current;
- (*hook) (NULL);
+ (void)(*hook) (NULL); /* ignore return value */
return true;
}
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
