From 63229f465aaea8dd5553814535220817319a64f5 Mon Sep 17 00:00:00 2001 From: Boris Kolpackov Date: Mon, 4 May 2015 21:22:14 +0200 Subject: Cleanup the code --- brep/module | 2 +- brep/module.cxx | 61 ++++---------- brep/options.cli | 7 +- brep/search | 4 +- brep/search.cxx | 6 +- brep/view | 4 +- brep/view.cxx | 6 +- web/apache/log | 30 +++---- web/apache/request | 70 ++-------------- web/apache/request.cxx | 224 ++++++++++++++++++++++++++++++++++++++++++++++--- web/apache/request.ixx | 147 ++------------------------------ web/apache/service | 192 ++++-------------------------------------- web/apache/service.cxx | 109 ++++++++++++++++++++++++ web/apache/service.txx | 85 +++++++++++++++++++ web/apache/stream | 23 +++-- web/module | 2 - 16 files changed, 489 insertions(+), 483 deletions(-) create mode 100644 web/apache/service.cxx create mode 100644 web/apache/service.txx diff --git a/brep/module b/brep/module index d1399fb..ef9154f 100644 --- a/brep/module +++ b/brep/module @@ -106,7 +106,7 @@ namespace brep // options. // virtual void - init (::cli::scanner& s) + init (cli::scanner& s) { // Just scan options to ensure there is no misspelled ones. // diff --git a/brep/module.cxx b/brep/module.cxx index fc239ba..612e521 100644 --- a/brep/module.cxx +++ b/brep/module.cxx @@ -7,7 +7,7 @@ #include #include -#include // unique_ptr +#include #include #include #include @@ -41,7 +41,7 @@ namespace brep try { static const char* sev_str[] = {"error", "warning", "info", "trace"}; - ostream& o = rs.content (500, "text/plain;charset=utf-8"); + ostream& o (rs.content (500, "text/plain;charset=utf-8")); for (const auto& d: e.data) { @@ -81,16 +81,16 @@ namespace brep init (const name_values& options, log& log) { log_ = &log; - - int argc = 0; - unique_ptr argv (new const char*[options.size () * 2]); + vector argv; for (const auto& nv: options) { - argv[argc++] = nv.name.c_str (); - argv[argc++] = nv.value.c_str (); + argv.push_back (nv.name.c_str ()); + argv.push_back (nv.value.c_str ()); } + int argc (argv.size()); + try { { @@ -98,7 +98,7 @@ namespace brep // cli::argv_file_scanner s (0, argc, - const_cast (argv.get ()), + const_cast (argv.data ()), "conf"); init (s); @@ -108,13 +108,10 @@ namespace brep // cli::argv_file_scanner s (0, argc, - const_cast (argv.get ()), + const_cast (argv.data ()), "conf"); - module_options o (s, - ::cli::unknown_mode::skip, - ::cli::unknown_mode::skip); - + module_options o (s, cli::unknown_mode::skip, cli::unknown_mode::skip); verb_ = o.verb (); } catch (const server_error& e) @@ -149,7 +146,7 @@ namespace brep string module:: func_name (const char* pretty_name) { - const char* e = strchr (pretty_name, ')'); + const char* e (strchr (pretty_name, ')')); if (e && e > pretty_name) { @@ -198,7 +195,9 @@ namespace brep if (log_ == nullptr) return; // No backend yet. - auto al = dynamic_cast<::web::apache::log*> (log_); + //@@ Cast log_ to apache::log and write the records. + // + auto al (dynamic_cast<::web::apache::log*> (log_)); if (al) { @@ -228,37 +227,5 @@ namespace brep e.msg.c_str ()); } } - - //@@ Cast log_ to apache::log and write the records. - // - - //@@ __PRETTY_FUNCTION__ contains a lot of fluff that we probably - // don't want in the logs (like return value and argument list; - // though the argument list would distinguish between several - // overloads). If that's the case, then this is probably the - // best place to process the name and convert something like: - // - // void module::handle(request, response) - // - // To just: - // - // module::handle - // - // Note to someone who is going to implement this: searching for a - // space to determine the end of the return type may not work if - // the return type is, say, a template id or a pointer to function - // type. It seems a more robust approach would be to scan backwards - // until we find the first ')' -- this got to be the end of the - // function argument list. Now we continue scanning backwards keeping - // track of the ')' vs '(' balance (arguments can also be of pointer - // to function type). Once we see an unbalanced '(', then we know this - // is the beginning of the argument list. Everything between it and - // the preceding space is the qualified function name. Good luck ;-). - // - // If we also use the name in handle() above (e.g., to return to - // the user as part of 505), then we should do it there as well - // (in which case factoring this functionality into a separate - // function seem to make a lot of sense). - // } } diff --git a/brep/options.cli b/brep/options.cli index cbd5d3a..8da666f 100644 --- a/brep/options.cli +++ b/brep/options.cli @@ -1,21 +1,22 @@ include ; +include ; namespace brep { class module_options { - unsigned int verb = 0; + std::uint16_t verb = 0; }; class db_options { std::string db-host = "localhost"; - unsigned short db-port = 3306; + std::uint16_t db-port = 3306; }; class search_options: module_options, db_options { - unsigned int results-on-page = 10; + size_t results-on-page = 10; }; class view_options: module_options, db_options diff --git a/brep/search b/brep/search index d744c30..b604db4 100644 --- a/brep/search +++ b/brep/search @@ -14,12 +14,12 @@ namespace brep { class search: public module { - public: + private: virtual void handle (request&, response&); virtual void - init (::cli::scanner&); + init (cli::scanner&); private: std::shared_ptr options_; diff --git a/brep/search.cxx b/brep/search.cxx index 5f9f507..e282f79 100644 --- a/brep/search.cxx +++ b/brep/search.cxx @@ -15,13 +15,13 @@ using namespace std; namespace brep { void search:: - init (::cli::scanner& s) + init (cli::scanner& s) { MODULE_DIAG; options_ = make_shared (s, - ::cli::unknown_mode::fail, - ::cli::unknown_mode::fail); + cli::unknown_mode::fail, + cli::unknown_mode::fail); if (options_->results_on_page () > 30) fail << "too many search results on page: " diff --git a/brep/view b/brep/view index da31481..a418872 100644 --- a/brep/view +++ b/brep/view @@ -14,12 +14,12 @@ namespace brep { class view: public module { - public: + private: virtual void handle (request&, response&); virtual void - init (::cli::scanner&); + init (cli::scanner&); private: std::shared_ptr options_; diff --git a/brep/view.cxx b/brep/view.cxx index a644aec..1402d1f 100644 --- a/brep/view.cxx +++ b/brep/view.cxx @@ -14,11 +14,11 @@ using namespace std; namespace brep { void view:: - init (::cli::scanner& s) + init (cli::scanner& s) { options_ = make_shared (s, - ::cli::unknown_mode::fail, - ::cli::unknown_mode::fail); + cli::unknown_mode::fail, + cli::unknown_mode::fail); } void view:: diff --git a/web/apache/log b/web/apache/log index 50e2f7c..acc7c92 100644 --- a/web/apache/log +++ b/web/apache/log @@ -48,25 +48,25 @@ namespace web if (func) ap_log_error (file, - line, - APLOG_NO_MODULE, - level, - 0, - server_, - "[%s]: %s", - func, - msg); + line, + APLOG_NO_MODULE, + level, + 0, + server_, + "[%s]: %s", + func, + msg); else // skip function name placeholder from log line // ap_log_error (file, - line, - APLOG_NO_MODULE, - level, - 0, - server_, - ": %s", - msg); + line, + APLOG_NO_MODULE, + level, + 0, + server_, + ": %s", + msg); } private: diff --git a/web/apache/request b/web/apache/request index 4071bd1..1fb9cbe 100644 --- a/web/apache/request +++ b/web/apache/request @@ -46,48 +46,12 @@ namespace web // Get request body data stream. // virtual std::istream& - content () - { - if (!in_) - { - std::unique_ptr in_buf ( - new istreambuf (rec_, *this)); - - in_.reset (new std::istream (in_buf.get ())); - in_buf_ = std::move (in_buf); - in_->exceptions (std::ios::failbit | std::ios::badbit); - - // Save form data now otherwise will not be available to do later - // when data read from stream. - // - form_data (); - } - - return *in_; - } + content (); // Get request parameters. // virtual const name_values& - parameters () - { - if (!parameters_) - { - parameters_.reset (new name_values ()); - - try - { - parse_parameters (rec_->args); - parse_parameters (form_data ()->c_str ()); - } - catch (const std::invalid_argument& ) - { - throw invalid_request (); - } - } - - return *parameters_; - } + parameters (); // Get request cookies. // @@ -101,26 +65,7 @@ namespace web // Set response status code. // virtual void - status (status_code status) - { - if (status != rec_->status) - { - // Setting status code in exception handler is a common usecase - // where no sense to throw but still need to signal apache a - // proper status code. - // - if (get_write_state () && !std::current_exception ()) - { - throw sequence_error ("::web::apache::request::status"); - } - - rec_->status = status; - buffer_ = true; - out_.reset (); - out_buf_.reset (); - ap_set_content_type (rec_, nullptr); - } - } + status (status_code status); // Set response status code, content type and get body stream. // @@ -140,11 +85,9 @@ namespace web bool secure = false); private: - using string_ptr = std::unique_ptr; - // Get application/x-www-form-urlencoded form data. // - const string_ptr& + const std::string& form_data (); void @@ -167,7 +110,7 @@ namespace web // Preparing to write a response read and discard request // body if any. // - int r = ap_discard_request_body (rec_); + int r (ap_discard_request_body (rec_)); if (r != OK) { @@ -179,7 +122,6 @@ namespace web } private: - request_rec* rec_; bool buffer_ {true}; bool write_state_ {false}; @@ -189,7 +131,7 @@ namespace web std::unique_ptr in_; std::unique_ptr parameters_; std::unique_ptr cookies_; - string_ptr form_data_; + std::unique_ptr form_data_; }; } } diff --git a/web/apache/request.cxx b/web/apache/request.cxx index 3711437..caf0d60 100644 --- a/web/apache/request.cxx +++ b/web/apache/request.cxx @@ -25,6 +25,48 @@ namespace web { namespace apache { + + istream& request:: + content () + { + if (!in_) + { + unique_ptr in_buf (new istreambuf (rec_, *this)); + + in_.reset (new istream (in_buf.get ())); + in_buf_ = move (in_buf); + in_->exceptions (ios::failbit | ios::badbit); + + // Save form data now otherwise will not be available to do later + // when data already read from stream. + // + form_data (); + } + + return *in_; + } + + const name_values& request:: + parameters () + { + if (!parameters_) + { + parameters_.reset (new name_values ()); + + try + { + parse_parameters (rec_->args); + parse_parameters (form_data ().c_str ()); + } + catch (const invalid_argument& ) + { + throw invalid_request (); + } + } + + return *parameters_; + } + const name_values& request:: cookies () { @@ -32,8 +74,8 @@ namespace web { cookies_.reset (new name_values ()); - const apr_array_header_t* ha = apr_table_elts (rec_->headers_in); - size_t n = ha->nelts; + const apr_array_header_t* ha (apr_table_elts (rec_->headers_in)); + size_t n (ha->nelts); for (auto h (reinterpret_cast (ha->elts)); n--; ++h) @@ -42,8 +84,8 @@ namespace web { for (const char* n (h->val); n != 0; ) { - const char* v = strchr (n, '='); - const char* e = strchr (n, ';'); + const char* v (strchr (n, '=')); + const char* e (strchr (n, ';')); if (e && e < v) v = 0; @@ -97,12 +139,12 @@ namespace web // form_data (); - unique_ptr out_buf ( + unique_ptr out_buf ( buffer - ? static_cast (new std::stringbuf ()) - : static_cast (new ostreambuf (rec_, *this))); + ? static_cast (new stringbuf ()) + : static_cast (new ostreambuf (rec_, *this))); - out_.reset (new std::ostream (out_buf.get ())); + out_.reset (new ostream (out_buf.get ())); out_buf_ = move (out_buf); out_->exceptions (ios::eofbit | ios::failbit | ios::badbit); @@ -117,6 +159,28 @@ namespace web } void request:: + status (status_code status) + { + if (status != rec_->status) + { + // Setting status code in exception handler is a common usecase + // where no sense to throw but still need to signal apache a + // proper status code. + // + if (get_write_state () && !current_exception ()) + { + throw sequence_error ("::web::apache::request::status"); + } + + rec_->status = status; + buffer_ = true; + out_.reset (); + out_buf_.reset (); + ap_set_content_type (rec_, nullptr); + } + } + + void request:: cookie (const char* name, const char* value, const chrono::seconds* max_age, @@ -126,6 +190,8 @@ namespace web { if (get_write_state ()) { + // Too late to send cookie if content is already written. + // throw sequence_error ("::web::apache::request::cookie"); } @@ -136,12 +202,12 @@ namespace web if (max_age) { - chrono::system_clock::time_point tp = - chrono::system_clock::now () + *max_age; + chrono::system_clock::time_point tp ( + chrono::system_clock::now () + *max_age); - time_t t = chrono::system_clock::to_time_t (tp); + time_t t (chrono::system_clock::to_time_t (tp)); - // Assume global "C" locale is not changed. + // Assume global locale is not changed and still "C". // char b[100]; strftime (b, sizeof (b), "%a, %d-%b-%Y %H:%M:%S GMT", gmtime (&t)); @@ -166,5 +232,139 @@ namespace web apr_table_add (rec_->err_headers_out, "Set-Cookie", s.str ().c_str ()); } + void request:: + parse_parameters (const char* args) + { + for (auto n (args); n != 0; ) + { + const char* v (strchr (n, '=')); + const char* e (strchr (n, '&')); + + if (e && e < v) + v = 0; + + string name (v + ? mime_url_decode (n, v) : + (e + ? mime_url_decode (n, e) + : mime_url_decode (n, n + strlen (n)))); + + string value; + + if (v++) + { + value = e + ? mime_url_decode (v, e) + : mime_url_decode (v, v + strlen (v)); + } + + if (!name.empty () || !value.empty ()) + parameters_->emplace_back (move (name), move (value)); + + n = e ? e + 1 : 0; + } + } + + void request:: + mime_url_encode (const char* v, ostream& o) + { + char f (o.fill ()); + ios_base::fmtflags g (o.flags ()); + o << hex << uppercase << right << setfill ('0'); + + char c; + + while ((c = *v++) != '\0') + { + if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9')) + { + o << c; + } + else + switch (c) + { + case ' ': o << '+'; break; + case '.': + case '_': + case '-': + case '~': o << c; break; + default: + { + o << "%" << setw (2) << static_cast (c); + break; + } + } + } + + o.flags (g); + o.fill (f); + } + + string request:: + mime_url_decode (const char* b, const char* e, bool trim) + { + if (trim) + { + b += strspn (b, " "); + + if (b >= e) + return string (); + + while (*--e == ' '); + ++e; + } + + string value; + value.reserve (e - b); + + char bf[3]; + bf[2] = '\0'; + + while (b != e) + { + char c (*b++); + + switch (c) + { + case '+': + { + value.append (" "); + break; + } + case '%': + { + if (*b == '\0' || b[1] == '\0') + { + throw invalid_argument ( + "::web::apache::request::mime_url_decode short"); + } + + *bf = *b; + bf[1] = b[1]; + + char* ebf (nullptr); + size_t vl (strtoul (bf, &ebf, 16)); + + if (*ebf != '\0') + { + throw invalid_argument ( + "::web::apache::request::mime_url_decode wrong"); + } + + value.append (1, static_cast (vl)); + b += 2; + break; + } + default: + { + value.append (1, c); + break; + } + } + } + + return value; + } } } diff --git a/web/apache/request.ixx b/web/apache/request.ixx index 06284fb..f7446e5 100644 --- a/web/apache/request.ixx +++ b/web/apache/request.ixx @@ -18,7 +18,7 @@ namespace web { if (buffer_ && out_buf_) { - auto b = dynamic_cast (out_buf_.get ()); + auto b (dynamic_cast (out_buf_.get ())); assert (b); std::string s (b->str ()); @@ -27,7 +27,7 @@ namespace web { // Before writing response read and discard request body if any. // - int r = ap_discard_request_body (rec_); + int r (ap_discard_request_body (rec_)); if (r == OK) { @@ -36,7 +36,6 @@ namespace web if (ap_rwrite (s.c_str (), s.length (), rec_) < 0) rec_->status = HTTP_REQUEST_TIME_OUT; } - else rec_->status = r; } @@ -48,13 +47,13 @@ namespace web return rec_->status == HTTP_OK || get_write_state () ? OK : rec_->status; } - inline const request::string_ptr& request:: + inline const std::string& request:: form_data () { if (!form_data_) { form_data_.reset (new std::string ()); - const char *ct = apr_table_get (rec_->headers_in, "Content-Type"); + const char* ct (apr_table_get (rec_->headers_in, "Content-Type")); if (ct && !strncasecmp ("application/x-www-form-urlencoded", ct, 33)) { @@ -72,143 +71,7 @@ namespace web } } - return form_data_; - } - - inline void request:: - parse_parameters (const char* args) - { - for (auto n (args); n != 0; ) - { - const char* v = std::strchr (n, '='); - const char* e = ::strchr (n, '&'); - - if (e && e < v) - v = 0; - - std::string name (v - ? mime_url_decode (n, v) : - (e - ? mime_url_decode (n, e) - : mime_url_decode (n, n + std::strlen (n)))); - - std::string value; - - if (v++) - { - value = e - ? mime_url_decode (v, e) - : mime_url_decode (v, v + std::strlen (v)); - } - - if (!name.empty () || !value.empty ()) - parameters_->emplace_back (std::move (name), std::move (value)); - - n = e ? e + 1 : 0; - } + return *form_data_; } - - inline void request:: - mime_url_encode (const char* v, std::ostream& o) - { - char f = o.fill (); - std::ios_base::fmtflags g = o.flags (); - o << std::hex << std::uppercase << std::right << std::setfill ('0'); - - char c; - - while ((c = *v++) != '\0') - { - if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || - (c >= '0' && c <= '9')) - { - o << c; - } - else - switch (c) - { - case ' ': o << '+'; break; - case '.': - case '_': - case '-': - case '~': o << c; break; - default: - { - o << "%" << std::setw (2) << static_cast (c); - break; - } - } - } - - o.flags (g); - o.fill (f); - } - - inline std::string request:: - mime_url_decode (const char* b, const char* e, bool trim) - { - if (trim) - { - b += std::strspn (b, " "); - - if (b >= e) - return std::string (); - - while (*--e == ' '); - ++e; - } - - std::string value; - value.reserve (e - b); - - char bf[3]; - bf[2] = '\0'; - - while (b != e) - { - char c = *b++; - - switch (c) - { - case '+': - { - value.append (" "); - break; - } - case '%': - { - if (*b == '\0' || b[1] == '\0') - { - throw std::invalid_argument ( - "::web::apache::request::mime_url_decode short"); - } - - *bf = *b; - bf[1] = b[1]; - - char* ebf = 0; - size_t vl = std::strtoul (bf, &ebf, 16); - - if (*ebf != '\0') - { - throw std::invalid_argument ( - "::web::apache::request::mime_url_decode wrong"); - } - - value.append (1, static_cast (vl)); - b += 2; - break; - } - default: - { - value.append (1, c); - break; - } - } - } - - return value; - } - } } diff --git a/web/apache/service b/web/apache/service index cd405ee..8456f12 100644 --- a/web/apache/service +++ b/web/apache/service @@ -5,18 +5,11 @@ #ifndef WEB_APACHE_SERVICE #define WEB_APACHE_SERVICE -#include // memset() -#include // getppid() -#include // kill() - #include -#include #include #include -#include // unique_ptr #include -#include #include // move() #include @@ -30,7 +23,6 @@ namespace web class service: ::module { public: - using option_names = std::vector; // Note that the module exemplar is stored by-reference. @@ -52,48 +44,17 @@ namespace web name_ (name), exemplar_ (exemplar), option_names_ (std::move (opts)) -// Doesn't look like handle_ member is required at all. -// handle_ (&handle_impl) { - // Fill apache module directive definitions. Directives share - // common name space in apache configuration file, so to prevent name - // clash have to form directive name as a combination of module and - // option names: -