[5451] Various changes after review.

src/lib/asiolink/tcp_socket.h

@@ -92,7 +92,8 @@ public:
 
     /// \brief Checks if the connection is usable.
     ///
-    /// The connection is usable if the peer has closed it.
+    /// The connection is usable if the socket is open and the peer has not
+    /// closed its connection.
     ///
     /// \return true if the connection is usable.
     bool isUsable() const {

src/lib/http/client.cc

@@ -269,7 +269,7 @@ public:
                         HttpResponsePtr& response,
                         long& request_timeout,
                         HttpClient::RequestHandler& callback) {
-        // Check if the is a queue for this URL. If there is no queue, there
+        // Check if there is a queue for this URL. If there is no queue, there
         // is no request queued either.
         auto it = queue_.find(url);
         if (it != queue_.end()) {
@@ -580,7 +580,7 @@ Connection::sendCallback(const boost::system::error_code& ec, size_t length) {
     }
 
     // If there is no more data to be sent, start receiving a response. Otherwise,
-    // continue receiving.
+    // continue sending.
     if (buf_.empty()) {
         doReceive();
 

src/lib/http/tests/response_parser_unittests.cc

@@ -68,6 +68,58 @@ public:
         EXPECT_FALSE(parser.getErrorMessage().empty());
     }
 
+    /// @brief Tests that the response specified with (header, body) can
+    ///        be parsed properly.
+    ///
+    /// @param header specifies the header of the response to be parsed
+    /// @param json specifies the body of the response (JSON in text format) to be parsed
+    /// @param expect_success whether the parsing is expected to be successful
+    ///
+    /// @return a parser that parsed the response for further inspection
+    HttpResponseJson testResponseWithJson(const std::string& header,
+                                          const std::string& json,
+                                          bool expect_success = true) {
+        std::string http_resp = createResponseString(header, json);
+
+        // Create HTTP response which accepts JSON as a body.
+        HttpResponseJson response;
+
+        // Create a parser and make it use the response we created.
+        HttpResponseParser parser(response);
+        EXPECT_NO_THROW(parser.initModel());
+
+        // Simulate receiving HTTP response in chunks.
+        const unsigned chunk_size = 10;
+        while (!http_resp.empty()) {
+            size_t chunk = http_resp.size() % chunk_size;
+            if (chunk == 0) {
+                chunk = chunk_size;
+            }
+
+            parser.postBuffer(&http_resp[0], chunk);
+            http_resp.erase(0, chunk);
+            parser.poll();
+            if (chunk < chunk_size) {
+                EXPECT_TRUE(parser.needData());
+                if (!parser.needData()) {
+                    ADD_FAILURE() << "Parser completed prematurely";
+                    return (response);
+                }
+            }
+        }
+
+        if (expect_success) {
+            // Parser should have parsed the response and should expect no more data.
+            EXPECT_FALSE(parser.needData());
+            // Parsing should be successful.
+            EXPECT_TRUE(parser.httpParseOk()) << parser.getErrorMessage();
+            // There should be no error message.
+            EXPECT_TRUE(parser.getErrorMessage().empty());
+        }
+
+        return (response);
+    }
+
     /// @brief Instance of the HttpResponse used by the unit tests.
     HttpResponse response_;
 };
@@ -79,37 +131,7 @@ TEST_F(HttpResponseParserTest, responseWithJson) {
         "Content-Type: application/json\r\n";
     std::string json = "{ \"result\": 0, \"text\": \"All ok\" }";
 
-    http_resp = createResponseString(http_resp, json);
-
-    // Create HTTP response which accepts JSON as a body.
-    HttpResponseJson response;
-
-    // Create a parser and make it use the response we created.
-    HttpResponseParser parser(response);
-    ASSERT_NO_THROW(parser.initModel());
-
-    // Simulate receiving HTTP response in chunks.
-    const unsigned chunk_size = 10;
-    while (!http_resp.empty()) {
-        size_t chunk = http_resp.size() % chunk_size;
-        if (chunk == 0) {
-            chunk = chunk_size;
-        }
-
-        parser.postBuffer(&http_resp[0], chunk);
-        http_resp.erase(0, chunk);
-        parser.poll();
-        if (chunk < chunk_size) {
-            ASSERT_TRUE(parser.needData());
-        }
-    }
-
-    // Parser should have parsed the response and should expect no more data.
-    ASSERT_FALSE(parser.needData());
-    // Parsing should be successful.
-    ASSERT_TRUE(parser.httpParseOk()) << parser.getErrorMessage();
-    // There should be no error message.
-    EXPECT_TRUE(parser.getErrorMessage().empty());
+    HttpResponseJson response = testResponseWithJson(http_resp, json);
 
     // Verify HTTP version, status code and phrase.
     EXPECT_EQ(1, response.getHttpVersion().major_);
@@ -166,8 +188,8 @@ TEST_F(HttpResponseParserTest, extraneousDataInResponse) {
     EXPECT_TRUE(parser.getErrorMessage().empty());
 }
 
-// This test verifies that LWS is parsed correctly. The LWS marks line breaks
-// in the HTTP header values.
+// This test verifies that LWS is parsed correctly. The LWS (linear white
+// space) marks line breaks in the HTTP header values.
 TEST_F(HttpResponseParserTest, getLWS) {
     // "User-Agent" header contains line breaks with whitespaces in the new
     // lines to mark continuation of the header value.

src/lib/http/tests/server_client_unittests.cc

@@ -1236,7 +1236,7 @@ TEST_F(HttpClientTest, clientRequestTimeout) {
     PostHttpRequestJsonPtr request2 = createRequest("sequence", 1);
     HttpResponseJsonPtr response2(new HttpResponseJson());
     ASSERT_NO_THROW(client.asyncSendRequest(url, request2, response2,
-        [this, &cb_num](const boost::system::error_code& ec, const HttpResponsePtr&,
+                    [this, &cb_num](const boost::system::error_code& /*ec*/, const HttpResponsePtr&,
                const std::string&) {
         if (++cb_num > 1) {
             io_service_.stop();

src/lib/http/url.cc

-// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -80,18 +80,18 @@ Url::parse() {
     port_ = 0;
     path_.clear();
 
-    std::ostringstream e;
+    std::ostringstream error;
 
     // Retrieve scheme
-    size_t p = url_.find(":");
-    if ((p == 0) || (p == std::string::npos)) {
-        e << "url " << url_ << " lacks http or https scheme";
-        error_message_ = e.str();
+    size_t offset = url_.find(":");
+    if ((offset == 0) || (offset == std::string::npos)) {
+        error << "url " << url_ << " lacks http or https scheme";
+        error_message_ = error.str();
         return;
     }
 
     // Validate scheme.
-    std::string scheme = url_.substr(0, p);
+    std::string scheme = url_.substr(0, offset);
     if (scheme == "http") {
         scheme_ = Url::HTTP;
 
@@ -99,87 +99,87 @@ Url::parse() {
         scheme_ = Url::HTTPS;
 
     } else {
-        e << "invalid scheme " << scheme << " in " << url_;
-        error_message_ = e.str();
+        error << "invalid scheme " << scheme << " in " << url_;
+        error_message_ = error.str();
         return;
     }
 
     // Colon and two slashes should follow the scheme
-    if (url_.substr(p, 3) != "://") {
-        e << "expected :// after scheme in " << url_;
-        error_message_ = e.str();
+    if (url_.substr(offset, 3) != "://") {
+        error << "expected :// after scheme in " << url_;
+        error_message_ = error.str();
         return;
     }
 
     // Move forward to hostname.
-    p += 3;
-    if (p >= url_.length()) {
-        e << "hostname missing in " << url_;
-        error_message_ = e.str();
+    offset += 3;
+    if (offset >= url_.length()) {
+        error << "hostname missing in " << url_;
+        error_message_ = error.str();
         return;
     }
 
-    size_t h = 0;
+    size_t offset2 = 0;
 
     // IPv6 address is specified within [ ].
-    if (url_.at(p) == '[') {
-        h = url_.find(']', p);
-        if (h == std::string::npos) {
-            e << "expected ] after IPv6 address in " << url_;
-            error_message_ = e.str();
+    if (url_.at(offset) == '[') {
+        offset2 = url_.find(']', offset);
+        if (offset2 == std::string::npos) {
+            error << "expected ] after IPv6 address in " << url_;
+            error_message_ = error.str();
             return;
 
-        } else if (h == p + 1) {
-            e << "expected IPv6 address within [] in " << url_;
-            error_message_ = e.str();
+        } else if (offset2 == offset + 1) {
+            error << "expected IPv6 address within [] in " << url_;
+            error_message_ = error.str();
             return;
         }
 
         // Move one character beyond the ].
-        ++h;
+        ++offset2;
 
     } else {
         // There is a normal hostname or IPv4 address. It is terminated
         // by the colon (for port number), a slash (if no port number) or
         // goes up to the end of the URL.
-        h = url_.find(":", p);
-        if (h == std::string::npos) {
-            h = url_.find("/", p);
-            if (h == std::string::npos) {
+        offset2 = url_.find(":", offset);
+        if (offset2 == std::string::npos) {
+            offset2 = url_.find("/", offset);
+            if (offset2 == std::string::npos) {
                 // No port number and no slash.
-                h = url_.length();
+                offset2 = url_.length();
             }
         }
     }
 
     // Extract the hostname.
-    hostname_ = url_.substr(p, h - p);
+    hostname_ = url_.substr(offset, offset2 - offset);
 
     // If there is no port number and no path, simply return and mark the
     // URL as valid.
-    if (h == url_.length()) {
+    if (offset2 == url_.length()) {
         valid_ = true;
         return;
     }
 
     // If there is a port number, we need to read it and convert to
     // numeric value.
-    if (url_.at(h) == ':') {
-        if (h == url_.length() - 1) {
-            e << "expected port number after : in " << url_;
-            error_message_ = e.str();
+    if (url_.at(offset2) == ':') {
+        if (offset2 == url_.length() - 1) {
+            error << "expected port number after : in " << url_;
+            error_message_ = error.str();
             return;
         }
         // Move to the port number.
-        ++h;
+        ++offset2;
 
         // Port number may be terminated by a slash or by the end of URL.
-        size_t s = url_.find('/', h);
+        size_t slash_offset = url_.find('/', offset2);
         std::string port_str;
-        if (s == std::string::npos) {
-            port_str = url_.substr(h);
+        if (slash_offset == std::string::npos) {
+            port_str = url_.substr(offset2);
         } else {
-            port_str = url_.substr(h, s - h);
+            port_str = url_.substr(offset2, slash_offset - offset2);
         }
 
         try {
@@ -187,18 +187,18 @@ Url::parse() {
             port_ = boost::lexical_cast<unsigned>(port_str);
 
         } catch (...) {
-            e << "invalid port number " << port_str << " in " << url_;
-            error_message_ = e.str();
+            error << "invalid port number " << port_str << " in " << url_;
+            error_message_ = error.str();
             return;
         }
 
         // Go to the end of the port section.
-        h = s;
+        offset2 = slash_offset;
     }
 
     // If there is anything left in the URL, we consider it a path.
-    if (h != std::string::npos) {
-        path_ = url_.substr(h);
+    if (offset2 != std::string::npos) {
+        path_ = url_.substr(offset2);
     }
 
     valid_ = true;

src/lib/http/url.h

-// Copyright (C) 2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2017-2018 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -13,7 +13,7 @@
 namespace isc {
 namespace http {
 
-/// @brief Represents URL.
+/// @brief Represents an URL.
 ///
 /// It parses the provided URL and allows for retrieving the parts
 /// of it after parsing.
@@ -33,6 +33,12 @@ public:
     /// @param url URL.
     explicit Url(const std::string& url);
 
+    /// @brief compares URLs lexically.
+    ///
+    /// Both URLs are compared as text.
+    ///
+    /// @param url URL to be compared with
+    /// @return true if the other operator is larger (in lexical sense)
     bool operator<(const Url& url) const;
 
     /// @brief Checks if the URL is valid.