On Mon, Jan 16, 2012 at 08:54:53PM -0500, Robert Haas wrote:
> On Sun, Dec 18, 2011 at 11:53 AM, Noah Misch <n...@leadboat.com> wrote:
> > Here's a version that calls sigsetjmp() once per file. ?While
> > postgresql.conf
> > scanning hardly seems like the place to be shaving cycles, this does catch
> > fatal errors in functions other than yylex(), notably yy_create_buffer().
>
> This strikes me as more clever than necessary:
>
> #define fprintf(file, fmt, msg) \
> 0; /* eat cast to void */ \
> GUC_flex_fatal_errmsg = msg; \
> siglongjmp(*GUC_flex_fatal_jmp, 1)
>
> Can't we just define a function jumpoutoftheparser() here and call
> that function rather than doing that /* eat cast to void */ hack?
> That would also involve fewer assumptions about the coding style
> emitted by flex. For example, if flex chose to do something like:
>
> if (bumpity) fprintf(__FILE__, "%s", "dinglewump");
>
> ...the proposed definition would be a disaster. I doubt that inlining
> is a material performance benefit here; siglongjmp() can't be all that
> cheap, and the error path shouldn't be all that frequent.
>
> Instead of making ParseConfigFp responsible for restoring
> GUC_flex_fatal_jmp after calling anything that might recursively call
> ParseConfigFp, I think it would make more sense to define it to stash
> away the previous value and restore it on exit. That way it wouldn't
> need to know which of the things that it calls might in turn
> recursively call it, which seems likely to reduce the chances of
> present or future bugs. A few comments about whichever way we go with
> it seem like a good idea, too.
Agreed on all points. I also changed the save/restore of ConfigFileLineno to
work the same way. New version attached.
Thanks,
nm
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***************
*** 20,28 ****
#include "utils/guc.h"
! /* Avoid exit() on fatal scanner errors (a bit ugly -- see yy_fatal_error) */
#undef fprintf
! #define fprintf(file, fmt, msg) ereport(ERROR, (errmsg_internal("%s", msg)))
enum {
GUC_ID = 1,
--- 20,33 ----
#include "utils/guc.h"
! /*
! * flex emits a yy_fatal_error() function that it calls in response to
! * critical errors like malloc failure, file I/O errors, and detection of
! * internal inconsistency. That function prints a message and calls exit().
! * Mutate it to instead call our handler, which jumps out of the parser.
! */
#undef fprintf
! #define fprintf(file, fmt, msg) GUC_flex_fatal(msg)
enum {
GUC_ID = 1,
***************
*** 37,46 **** enum {
--- 42,54 ----
};
static unsigned int ConfigFileLineno;
+ static const char *GUC_flex_fatal_errmsg;
+ static sigjmp_buf *GUC_flex_fatal_jmp;
/* flex fails to supply a prototype for yylex, so provide one */
int GUC_yylex(void);
+ static int GUC_flex_fatal(const char *msg);
static char *GUC_scanstr(const char *s);
%}
***************
*** 437,442 **** ParseConfigFile(const char *config_file, const char
*calling_file, bool strict,
--- 445,466 ----
}
/*
+ * Flex fatal errors bring us here. Stash the error message and jump back to
+ * ParseConfigFp(). Assume all msg arguments point to string constants; this
+ * holds for flex 2.5.31 (earliest we support) and flex 2.5.35 (latest as of
+ * this writing). Otherwise, we would need to copy the message.
+ *
+ * We return "int" since this takes the place of calls to fprintf().
+ */
+ static int
+ GUC_flex_fatal(const char *msg)
+ {
+ GUC_flex_fatal_errmsg = msg;
+ siglongjmp(*GUC_flex_fatal_jmp, 1);
+ return 0; /* keep compiler quiet */
+ }
+
+ /*
* Read and parse a single configuration file. This function recurses
* to handle "include" directives.
*
***************
*** 464,482 **** ParseConfigFp(FILE *fp, const char *config_file, int depth,
int elevel,
ConfigVariable **head_p, ConfigVariable **tail_p)
{
bool OK = true;
! YY_BUFFER_STATE lex_buffer;
int errorcount;
int token;
/*
* Parse
*/
- lex_buffer = yy_create_buffer(fp, YY_BUF_SIZE);
- yy_switch_to_buffer(lex_buffer);
-
ConfigFileLineno = 1;
errorcount = 0;
/* This loop iterates once per logical line */
while ((token = yylex()))
{
--- 488,525 ----
ConfigVariable **head_p, ConfigVariable **tail_p)
{
bool OK = true;
! unsigned int save_ConfigFileLineno = ConfigFileLineno;
! sigjmp_buf *save_GUC_flex_fatal_jmp = GUC_flex_fatal_jmp;
! sigjmp_buf flex_fatal_jmp;
! volatile YY_BUFFER_STATE lex_buffer = NULL;
int errorcount;
int token;
+ if (sigsetjmp(flex_fatal_jmp, 1) == 0)
+ GUC_flex_fatal_jmp = &flex_fatal_jmp;
+ else
+ {
+ /*
+ * Regain control after a fatal, internal flex error. It may
have
+ * corrupted parser state. Consequently, abandon the file, but
trust
+ * that the state remains sane enough for yy_delete_buffer().
+ */
+ elog(elevel, "%s at file \"%s\" line %u",
+ GUC_flex_fatal_errmsg, config_file, ConfigFileLineno);
+
+ OK = false;
+ goto cleanup;
+ }
+
/*
* Parse
*/
ConfigFileLineno = 1;
errorcount = 0;
+ lex_buffer = yy_create_buffer(fp, YY_BUF_SIZE);
+ yy_switch_to_buffer(lex_buffer);
+
/* This loop iterates once per logical line */
while ((token = yylex()))
{
***************
*** 526,539 **** ParseConfigFp(FILE *fp, const char *config_file, int depth,
int elevel,
* An include_if_exists directive isn't a variable and
should be
* processed immediately.
*/
- unsigned int save_ConfigFileLineno = ConfigFileLineno;
-
if (!ParseConfigFile(opt_value, config_file, false,
depth + 1,
elevel,
head_p,
tail_p))
OK = false;
yy_switch_to_buffer(lex_buffer);
- ConfigFileLineno = save_ConfigFileLineno;
pfree(opt_name);
pfree(opt_value);
}
--- 569,579 ----
***************
*** 543,556 **** ParseConfigFp(FILE *fp, const char *config_file, int depth,
int elevel,
* An include directive isn't a variable and should be
processed
* immediately.
*/
- unsigned int save_ConfigFileLineno = ConfigFileLineno;
-
if (!ParseConfigFile(opt_value, config_file, true,
depth + 1,
elevel,
head_p,
tail_p))
OK = false;
yy_switch_to_buffer(lex_buffer);
- ConfigFileLineno = save_ConfigFileLineno;
pfree(opt_name);
pfree(opt_value);
}
--- 583,593 ----
***************
*** 620,626 **** ParseConfigFp(FILE *fp, const char *config_file, int depth,
int elevel,
--- 657,667 ----
break;
}
+ cleanup:
yy_delete_buffer(lex_buffer);
+ /* Each recursion level must save and restore these static variables. */
+ ConfigFileLineno = save_ConfigFileLineno;
+ GUC_flex_fatal_jmp = save_GUC_flex_fatal_jmp;
return OK;
}
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
