From 3b5b1b3d68fabb8aac076a069f1a4e3a2f1e4f3b Mon Sep 17 00:00:00 2001
From: g0dil <g0dil@wiback.org>
Date: Tue, 23 Sep 2008 17:58:09 +0000
Subject: [PATCH] Console: Parser error handling
---
Console/ConfigFile.cc | 7 ++--
Console/Parse.cc | 98 ++++++++++++++++++++++++++++++-------------
Console/Parse.cci | 17 --------
Console/Parse.hh | 33 +++++++--------
Console/Parse.ih | 39 +++++++++++++----
Console/Parse.test.cc | 37 ++++++++++++++--
Console/Server.cc | 9 ++--
7 files changed, 154 insertions(+), 86 deletions(-)
diff --git a/Console/ConfigFile.cc b/Console/ConfigFile.cc
index d57a0a83..45e0eeed 100644
--- a/Console/ConfigFile.cc
+++ b/Console/ConfigFile.cc
@@ -37,10 +37,9 @@
prefix_ void senf::console::detail::ConfigFileSource::v_parse(RestrictedExecutor & executor)
{
- if (! parser_.parseFile(filename_, boost::bind( boost::ref(executor),
- boost::ref(std::cerr),
- _1 )) )
- throw SyntaxErrorException();
+ parser_.parseFile(filename_, boost::bind( boost::ref(executor),
+ boost::ref(std::cerr),
+ _1 ));
}
///////////////////////////////////////////////////////////////////////////
diff --git a/Console/Parse.cc b/Console/Parse.cc
index 57a5532b..83ff10c6 100644
--- a/Console/Parse.cc
+++ b/Console/Parse.cc
@@ -30,7 +30,9 @@
#include <cerrno>
#include <boost/iterator/transform_iterator.hpp>
#include <boost/spirit/iterator/file_iterator.hpp>
+#include <boost/spirit/iterator/position_iterator.hpp>
#include "../Utils/Exception.hh"
+#include "../Utils/senfassert.hh"
//#include "Parse.mpp"
#define prefix_
@@ -241,6 +243,22 @@ struct senf::console::CommandParser::Impl
#endif
+namespace {
+
+ template <class Error>
+ void throwParserError(Error const & err)
+ {
+ static char const * msg [] = { "end of statement expected",
+ "'{' or arguments expected",
+ "path expected",
+ "')' expected",
+ "'\"' expected" };
+ boost::spirit::file_position pos (err.where.get_position());
+ throw senf::console::CommandParser::ParserErrorException(msg[err.descriptor])
+ << "\nat " << pos.file << ":" << pos.line << ":" << pos.column;
+ }
+}
+
prefix_ senf::console::CommandParser::CommandParser()
: impl_ (new Impl())
{}
@@ -252,52 +270,83 @@ prefix_ senf::console::CommandParser::~CommandParser()
// we would need to expose the Impl member to the public, which we don't want to do.
template <class Iterator>
-prefix_ Iterator senf::console::CommandParser::parseLoop(Iterator b, Iterator e, Callback cb)
+prefix_ Iterator senf::console::CommandParser::parseLoop(Iterator npb, Iterator npe,
+ std::string const & source, Callback cb)
{
+ typedef boost::spirit::position_iterator<Iterator> PositionIterator;
+ PositionIterator b (npb, npe, source);
+ PositionIterator e (npe, npe, source);
ParseCommandInfo info;
detail::ParseDispatcher::BindInfo bind (impl().dispatcher, info);
- boost::spirit::parse_info<Iterator> result;
+ boost::spirit::parse_info<PositionIterator> result;
for(;;) {
result = boost::spirit::parse(
b, e, * impl().grammar.use_parser<Impl::Grammar::SkipParser>());
b = result.stop;
if (b == e)
- return e;
+ return e.base();
info.clear();
- result = boost::spirit::parse(b, e,
- impl().grammar.use_parser<Impl::Grammar::CommandParser>(),
- impl().grammar.use_parser<Impl::Grammar::SkipParser>());
- if (! result.hit)
- return b;
+ try {
+ result = boost::spirit::parse(b, e,
+ impl().grammar.use_parser<Impl::Grammar::CommandParser>(),
+ impl().grammar.use_parser<Impl::Grammar::SkipParser>());
+ }
+ catch (boost::spirit::parser_error<Impl::Grammar::Errors, PositionIterator> & ex) {
+ if (impl().grammar.incremental && ex.where == e)
+ return b.base();
+ else
+ throwParserError(ex);
+ }
+ // Otherwise the error handling in the parser is broken
+ SENF_ASSERT( result.hit );
if (! info.empty())
- cb(info);
+ try {
+ cb(info);
+ }
+ catch (senf::ExceptionMixin & ex) {
+ boost::spirit::file_position pos (result.stop.get_position());
+ ex << "\nat " << pos.file << ":" << pos.line << ":" << pos.column;
+ throw;
+ }
b = result.stop;
}
}
-prefix_ bool senf::console::CommandParser::parse(std::string const & command, Callback cb)
+prefix_ void senf::console::CommandParser::parse(std::string const & command, Callback cb)
{
- return parseLoop(command.begin(), command.end(), cb) == command.end();
+ parseLoop(command.begin(), command.end(), "<unknown>", cb);
}
-prefix_ bool senf::console::CommandParser::parseFile(std::string const & filename, Callback cb)
+prefix_ void senf::console::CommandParser::parseFile(std::string const & filename, Callback cb)
{
boost::spirit::file_iterator<> i (filename);
if (!i) throw SystemException(ENOENT SENF_EXC_DEBUGINFO);
boost::spirit::file_iterator<> const i_end (i.make_end());
-
- return parseLoop(i, i_end, cb) == i_end;
+ parseLoop(i, i_end, filename, cb);
}
-prefix_ bool senf::console::CommandParser::parseArguments(std::string const & arguments,
+prefix_ void senf::console::CommandParser::parseArguments(std::string const & arguments,
ParseCommandInfo & info)
{
+ typedef boost::spirit::position_iterator<std::string::const_iterator> PositionIterator;
+ PositionIterator b (arguments.begin(), arguments.end(), std::string("<unknown>"));
+ PositionIterator e (arguments.end(), arguments.end(), std::string("<unknown>"));
detail::ParseDispatcher::BindInfo bind (impl().dispatcher, info);
- return boost::spirit::parse( arguments.begin(), arguments.end(),
- impl().grammar.use_parser<Impl::Grammar::ArgumentsParser>(),
- impl().grammar.use_parser<Impl::Grammar::SkipParser>()
- ).full;
+ boost::spirit::parse_info<PositionIterator> result;
+ try {
+ result = boost::spirit::parse( b, e,
+ impl().grammar.use_parser<Impl::Grammar::ArgumentsParser>(),
+ impl().grammar.use_parser<Impl::Grammar::SkipParser>() );
+ }
+ catch (boost::spirit::parser_error<Impl::Grammar::Errors, PositionIterator> & ex) {
+ throwParserError(ex);
+ }
+ if (! result.full) {
+ boost::spirit::file_position pos (result.stop.get_position());
+ throw ParserErrorException("argument expected")
+ << "\nat " << pos.file << ":" << pos.line << ":" << pos.column;
+ }
}
struct senf::console::CommandParser::SetIncremental
@@ -318,16 +367,7 @@ senf::console::CommandParser::parseIncremental(std::string const & commands, Cal
{
SetIncremental si (*this);
return std::distance( commands.begin(),
- parseLoop(commands.begin(), commands.end(), cb) );
-}
-
-///////////////////////////////////////////////////////////////////////////
-// senf::console::SyntaxErrorException
-
-prefix_ char const * senf::console::SyntaxErrorException::what()
- const throw()
-{
- return message().empty() ? "syntax error" : message().c_str();
+ parseLoop(commands.begin(), commands.end(), "<unknown>", cb) );
}
///////////////////////////////cc.e////////////////////////////////////////
diff --git a/Console/Parse.cci b/Console/Parse.cci
index 322fc44f..b5a71be8 100644
--- a/Console/Parse.cci
+++ b/Console/Parse.cci
@@ -228,23 +228,6 @@ prefix_ void senf::console::ParseCommandInfo::ArgumentIterator::increment()
b_ = e_;
}
-///////////////////////////////////////////////////////////////////////////
-// senf::console::SyntaxErrorException
-
-prefix_ senf::console::SyntaxErrorException::SyntaxErrorException(std::string const & msg)
- : message_(msg)
-{}
-
-prefix_ senf::console::SyntaxErrorException::~SyntaxErrorException()
- throw()
-{}
-
-prefix_ std::string const & senf::console::SyntaxErrorException::message()
- const
-{
- return message_;
-}
-
///////////////////////////////////////////////////////////////////////////
prefix_ senf::console::CheckedArgumentIteratorWrapper::
diff --git a/Console/Parse.hh b/Console/Parse.hh
index 56535661..288166eb 100644
--- a/Console/Parse.hh
+++ b/Console/Parse.hh
@@ -197,6 +197,7 @@
#include <boost/iterator/iterator_facade.hpp>
#include <boost/function.hpp>
#include "../Utils/safe_bool.hh"
+#include "../Utils/Exception.hh"
//#include "Parse.mpp"
///////////////////////////////hh.p////////////////////////////////////////
@@ -323,7 +324,7 @@ namespace console {
Every command parsed is returned in a ParseCommandInfo instance. This information is purely
taken from the parser, no semantic information is attached at this point, the config/console
- node tree is not involved in any why. ParseCommandInfo consist of
+ node tree is not involved in any way. ParseCommandInfo consist of
\li the type of command: built-in or normal command represented by a possibly relative path
into the command tree.
@@ -438,17 +439,9 @@ namespace console {
All errors while parsing the arguments of a command must be signaled by throwing an instance
of SyntaxErrorException. This is important, so command overloading works.
*/
- struct SyntaxErrorException : public std::exception
- {
- explicit SyntaxErrorException(std::string const & msg = "");
- virtual ~SyntaxErrorException() throw();
-
- virtual char const * what() const throw();
- std::string const & message() const;
-
- private:
- std::string message_;
- };
+ struct SyntaxErrorException : public senf::Exception
+ { explicit SyntaxErrorException(std::string const & msg = "syntax error")
+ : senf::Exception(msg) {} };
/** \brief Wrapper checking argument iterator access for validity
@@ -543,7 +536,7 @@ namespace console {
using IteratorFacade::operator++;
ParseCommandInfo::ArgumentIterator operator++(int);
-
+
private:
reference dereference() const;
void increment();
@@ -596,13 +589,13 @@ namespace console {
///@}
///////////////////////////////////////////////////////////////////////////
- bool parse(std::string const & command, Callback cb); ///< Parse string
- bool parseFile(std::string const & filename, Callback cb); ///< Parse file
+ void parse(std::string const & command, Callback cb); ///< Parse string
+ void parseFile(std::string const & filename, Callback cb); ///< Parse file
/**< \throws SystemException if the file cannot be
read. */
- bool parseArguments(std::string const & arguments, ParseCommandInfo & info);
- ///< Parse \a argumtns
+ void parseArguments(std::string const & arguments, ParseCommandInfo & info);
+ ///< Parse \a arguments
/**< parseArguments() parses the string \a arguments which
contains arbitrary command arguments (without the name
of the command). The argument tokens are written into
@@ -619,12 +612,16 @@ namespace console {
to be terminated explicitly. This means, that the
last ';' is \e not optional in this case. */
+ /** \brief Exception thrown when the parser detects an error */
+ struct ParserErrorException : public SyntaxErrorException
+ { explicit ParserErrorException(std::string const & msg) : SyntaxErrorException(msg) {} };
+
private:
struct Impl;
struct SetIncremental;
template <class Iterator>
- Iterator parseLoop(Iterator b, Iterator e, Callback cb);
+ Iterator parseLoop(Iterator b, Iterator e, std::string const & source, Callback cb);
Impl & impl();
diff --git a/Console/Parse.ih b/Console/Parse.ih
index e2859ff2..70791d4f 100644
--- a/Console/Parse.ih
+++ b/Console/Parse.ih
@@ -77,6 +77,17 @@ namespace detail {
ParseDispatcher & dispatcher;
+ ///////////////////////////////////////////////////////////////////////////
+ // Errors
+
+ enum Errors {
+ EndOfStatementExpected,
+ GroupOrArgumentsExpected,
+ PathExpected,
+ ClosingParenExpected,
+ QuoteExpected
+ };
+
///////////////////////////////////////////////////////////////////////////
CommandGrammar(ParseDispatcher & d, Context & c)
@@ -98,7 +109,7 @@ namespace detail {
definition(CommandGrammar const & self) :
// Characters with a special meaning within the parser
- special_p ("/(){};"),
+ special_p ("/(){};\""),
// Additional characters which are returned as punctuation tokens
// (only allowed within '()').
@@ -131,6 +142,12 @@ namespace detail {
actor< variable< Token > > token_ (self.context.token);
actor< variable< ParseDispatcher > > d_ (self.dispatcher);
+ assertion<Errors> end_of_statement_expected (EndOfStatementExpected);
+ assertion<Errors> group_or_arguments_expected (GroupOrArgumentsExpected);
+ assertion<Errors> path_expected (PathExpected);
+ assertion<Errors> closing_paren_expected (ClosingParenExpected);
+ assertion<Errors> quote_expected (QuoteExpected);
+
///////////////////////////////////////////////////////////////////
// Spirit grammar
//
@@ -170,15 +187,16 @@ namespace detail {
// More info is in the Boost.Spirit documentation
command
- = builtin >> statement_end
- | path >> ( group_start | statement )
+ = builtin >> end_of_statement_expected(statement_end)
| group_close
| ch_p(';') // Ignore empty commands
+ | path_expected(path)
+ >> group_or_arguments_expected( group_start | statement )
;
builtin
= keyword_p("cd")
- >> path
+ >> path_expected(path)
>> eps_p [ bind(&PD::builtin_cd)(d_, path_) ]
| keyword_p("ls")
>> ! path
@@ -200,7 +218,8 @@ namespace detail {
statement
= eps_p [ bind(&PD::beginCommand)(d_, path_) ]
>> arguments
- >> statement_end [ bind(&PD::endCommand)(d_) ]
+ >> end_of_statement_expected(statement_end)
+ [ bind(&PD::endCommand)(d_) ]
;
arguments
@@ -227,14 +246,17 @@ namespace detail {
- '"'
) [ str_ += ch_ ]
)
- >> ch_p('"') [ token_ = construct_<Token>(Token::BasicString,
+ >> quote_expected(ch_p('"'))
+ [ token_ = construct_<Token>(Token::BasicString,
str_) ]
]
;
hexstring // Returns value in context.token
= eps_p [ clear(str_) ]
- >> confix_p( "x\"", * hexbyte, '"' )
+ >> "x\""
+ >> * ( hexbyte - ch_p('"') )
+ >> quote_expected(ch_p('"'))
[ token_ = construct_<Token>(Token::HexString,
str_) ]
;
@@ -262,7 +284,8 @@ namespace detail {
"(") ]
[ bind(&PD::pushToken)(d_, token_) ]
>> * token
- >> ch_p(')') [ token_ = construct_<Token>(
+ >> closing_paren_expected(ch_p(')'))
+ [ token_ = construct_<Token>(
Token::ArgumentGroupClose,
")") ]
[ bind(&PD::pushToken)(d_, token_) ]
diff --git a/Console/Parse.test.cc b/Console/Parse.test.cc
index c650745a..39819724 100644
--- a/Console/Parse.test.cc
+++ b/Console/Parse.test.cc
@@ -197,7 +197,7 @@ BOOST_AUTO_UNIT_TEST(commandParser)
" 0304\";"
"ls /foo/bar; ";
- BOOST_CHECK( parser.parse(text, &setInfo) );
+ BOOST_CHECK_NO_THROW( parser.parse(text, &setInfo) );
BOOST_CHECK_EQUAL( commands.size(), 2u );
{
@@ -271,14 +271,14 @@ BOOST_AUTO_UNIT_TEST(checkedArgumentIterator)
{
senf::console::CommandParser parser;
- BOOST_CHECK( parser.parse("foo a", &setInfo) );
+ BOOST_CHECK_NO_THROW( parser.parse("foo a", &setInfo) );
BOOST_CHECK_THROW( parseArgs(commands.back().arguments()),
senf::console::SyntaxErrorException );
- BOOST_CHECK( parser.parse("foo a b", &setInfo) );
+ BOOST_CHECK_NO_THROW( parser.parse("foo a b", &setInfo) );
BOOST_CHECK_NO_THROW( parseArgs(commands.back().arguments()) );
- BOOST_CHECK( parser.parse("foo a b c", &setInfo) );
+ BOOST_CHECK_NO_THROW( parser.parse("foo a b c", &setInfo) );
BOOST_CHECK_THROW( parseArgs(commands.back().arguments()),
senf::console::SyntaxErrorException );
@@ -311,6 +311,35 @@ BOOST_AUTO_UNIT_TEST(parseIncremental)
commands.clear();
}
+namespace {
+ std::string parseErrorMessage(std::string const & msg)
+ {
+ std::string::size_type i (msg.find("-- \n"));
+ return i == std::string::npos ? msg : msg.substr(i+4);
+ }
+}
+
+BOOST_AUTO_UNIT_TEST(parseExceptions)
+{
+ senf::console::CommandParser parser;
+ std::string msg;
+
+# define CheckParseEx(c, e) \
+ commands.clear(); \
+ msg.clear(); \
+ try { parser.parse(c, &setInfo); } \
+ catch (std::exception & ex) { msg = parseErrorMessage(ex.what()); } \
+ BOOST_CHECK_EQUAL( msg, e )
+
+ CheckParseEx( "/foo/bar;\n ()", "path expected\nat <unknown>:2:3" );
+ CheckParseEx( "cd /foo/bar foo/bar", "end of statement expected\nat <unknown>:1:13" );
+ CheckParseEx( "/foo/bar foo /", "end of statement expected\nat <unknown>:1:14" );
+ CheckParseEx( "cd \"foo\"", "path expected\nat <unknown>:1:4" );
+ CheckParseEx( "/foo/bar \"string", "'\"' expected\nat <unknown>:1:17" );
+ CheckParseEx( "/foo/bar x\"hi\"", "'\"' expected\nat <unknown>:1:12" );
+ CheckParseEx( "/foo/bar (", "')' expected\nat <unknown>:1:11" );
+}
+
///////////////////////////////cc.e////////////////////////////////////////
#undef prefix_
diff --git a/Console/Server.cc b/Console/Server.cc
index 63b922bc..e69bca13 100644
--- a/Console/Server.cc
+++ b/Console/Server.cc
@@ -271,7 +271,6 @@ prefix_ std::string::size_type senf::console::Client::handleInput(std::string da
else
lastCommand_ = data;
- bool state (true);
std::string::size_type n (data.size());
try {
@@ -280,11 +279,9 @@ prefix_ std::string::size_type senf::console::Client::handleInput(std::string da
boost::ref(stream()),
_1 ));
else
- state = parser_.parse(data, boost::bind<void>( boost::ref(executor_),
- boost::ref(stream()),
- _1 ));
- if (! state )
- stream() << "syntax error" << std::endl;
+ parser_.parse(data, boost::bind<void>( boost::ref(executor_),
+ boost::ref(stream()),
+ _1 ));
}
catch (Executor::ExitException &) {
// This generates an EOF condition on the Handle. This EOF condition is expected
--
GitLab