Problem: magic numbers and data duplication in plain_*.cpp

Solution: extract constants / move to plain_common.hpp

CMakeLists.txt

@@ -740,6 +740,7 @@ set (cxx-sources
 		pgm_socket.hpp
 		pipe.hpp
 		plain_client.hpp
+		plain_common.hpp
 		plain_server.hpp
 		poll.hpp
 		poller.hpp

Makefile.am

@@ -134,6 +134,7 @@ src_libzmq_la_SOURCES = \
 	src/pipe.hpp \
 	src/plain_client.cpp \
 	src/plain_client.hpp \
+	src/plain_common.hpp \
 	src/plain_server.cpp \
 	src/plain_server.hpp \
 	src/platform.hpp \

src/plain_client.cpp

@@ -31,11 +31,13 @@
 #include "macros.hpp"
 
 #include <string>
+#include <limits.h>
 
 #include "msg.hpp"
 #include "err.hpp"
 #include "plain_client.hpp"
 #include "session_base.hpp"
+#include "plain_common.hpp"
 
 zmq::plain_client_t::plain_client_t (session_base_t *const session_,
                                      const options_t &options_) :
@@ -77,11 +79,14 @@ int zmq::plain_client_t::process_handshake_command (msg_t *msg_)
     const size_t data_size = msg_->size ();
 
     int rc = 0;
-    if (data_size >= 8 && !memcmp (cmd_data, "\7WELCOME", 8))
+    if (data_size >= welcome_prefix_len
+        && !memcmp (cmd_data, welcome_prefix, welcome_prefix_len))
         rc = process_welcome (cmd_data, data_size);
-    else if (data_size >= 6 && !memcmp (cmd_data, "\5READY", 6))
+    else if (data_size >= ready_prefix_len
+             && !memcmp (cmd_data, ready_prefix, ready_prefix_len))
         rc = process_ready (cmd_data, data_size);
-    else if (data_size >= 6 && !memcmp (cmd_data, "\5ERROR", 6))
+    else if (data_size >= error_prefix_len
+             && !memcmp (cmd_data, error_prefix, error_prefix_len))
         rc = process_error (cmd_data, data_size);
     else {
         session->get_socket ()->event_handshake_failed_protocol (
@@ -113,20 +118,21 @@ zmq::mechanism_t::status_t zmq::plain_client_t::status () const
 int zmq::plain_client_t::produce_hello (msg_t *msg_) const
 {
     const std::string username = options.plain_username;
-    zmq_assert (username.length () < 256);
+    zmq_assert (username.length () <= UCHAR_MAX);
 
     const std::string password = options.plain_password;
-    zmq_assert (password.length () < 256);
+    zmq_assert (password.length () <= UCHAR_MAX);
 
-    const size_t command_size =
-      6 + 1 + username.length () + 1 + password.length ();
+    const size_t command_size = hello_prefix_len + brief_len_size
+                                + username.length () + brief_len_size
+                                + password.length ();
 
     const int rc = msg_->init_size (command_size);
     errno_assert (rc == 0);
 
     unsigned char *ptr = static_cast<unsigned char *> (msg_->data ());
-    memcpy (ptr, "\x05HELLO", 6);
-    ptr += 6;
+    memcpy (ptr, hello_prefix, hello_prefix_len);
+    ptr += hello_prefix_len;
 
     *ptr++ = static_cast<unsigned char> (username.length ());
     memcpy (ptr, username.c_str (), username.length ());
@@ -149,7 +155,7 @@ int zmq::plain_client_t::process_welcome (const unsigned char *cmd_data_,
         errno = EPROTO;
         return -1;
     }
-    if (data_size_ != 8) {
+    if (data_size_ != welcome_prefix_len) {
         session->get_socket ()->event_handshake_failed_protocol (
           session->get_endpoint (),
           ZMQ_PROTOCOL_ERROR_ZMTP_MALFORMED_COMMAND_WELCOME);
@@ -162,7 +168,8 @@ int zmq::plain_client_t::process_welcome (const unsigned char *cmd_data_,
 
 int zmq::plain_client_t::produce_initiate (msg_t *msg_) const
 {
-    make_command_with_basic_properties (msg_, "\x08INITIATE", 9);
+    make_command_with_basic_properties (msg_, initiate_prefix,
+                                        initiate_prefix_len);
 
     return 0;
 }
@@ -176,7 +183,8 @@ int zmq::plain_client_t::process_ready (const unsigned char *cmd_data_,
         errno = EPROTO;
         return -1;
     }
-    const int rc = parse_metadata (cmd_data_ + 6, data_size_ - 6);
+    const int rc = parse_metadata (cmd_data_ + ready_prefix_len,
+                                   data_size_ - ready_prefix_len);
     if (rc == 0)
         _state = ready;
     else
@@ -195,22 +203,25 @@ int zmq::plain_client_t::process_error (const unsigned char *cmd_data_,
         errno = EPROTO;
         return -1;
     }
-    if (data_size_ < 7) {
+    const size_t start_of_error_reason = error_prefix_len + brief_len_size;
+    if (data_size_ < start_of_error_reason) {
         session->get_socket ()->event_handshake_failed_protocol (
           session->get_endpoint (),
           ZMQ_PROTOCOL_ERROR_ZMTP_MALFORMED_COMMAND_ERROR);
         errno = EPROTO;
         return -1;
     }
-    const size_t error_reason_len = static_cast<size_t> (cmd_data_[6]);
-    if (error_reason_len > data_size_ - 7) {
+    const size_t error_reason_len =
+      static_cast<size_t> (cmd_data_[error_prefix_len]);
+    if (error_reason_len > data_size_ - start_of_error_reason) {
         session->get_socket ()->event_handshake_failed_protocol (
           session->get_endpoint (),
           ZMQ_PROTOCOL_ERROR_ZMTP_MALFORMED_COMMAND_ERROR);
         errno = EPROTO;
         return -1;
     }
-    const char *error_reason = reinterpret_cast<const char *> (cmd_data_) + 7;
+    const char *error_reason =
+      reinterpret_cast<const char *> (cmd_data_) + start_of_error_reason;
     handle_error_reason (error_reason, error_reason_len);
     _state = error_command_received;
     return 0;

src/plain_common.hpp

+/*
+    Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file
+
+    This file is part of libzmq, the ZeroMQ core engine in C++.
+
+    libzmq is free software; you can redistribute it and/or modify it under
+    the terms of the GNU Lesser General Public License (LGPL) as published
+    by the Free Software Foundation; either version 3 of the License, or
+    (at your option) any later version.
+
+    As a special exception, the Contributors give you permission to link
+    this library with independent modules to produce an executable,
+    regardless of the license terms of these independent modules, and to
+    copy and distribute the resulting executable under terms of your choice,
+    provided that you also meet, for each linked independent module, the
+    terms and conditions of the license of that module. An independent
+    module is a module which is not derived from or based on this library.
+    If you modify this library, you must extend this exception to your
+    version of the library.
+
+    libzmq is distributed in the hope that it will be useful, but WITHOUT
+    ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+    FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public
+    License for more details.
+
+    You should have received a copy of the GNU Lesser General Public License
+    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+*/
+
+#ifndef __ZMQ_PLAIN_COMMON_HPP_INCLUDED__
+#define __ZMQ_PLAIN_COMMON_HPP_INCLUDED__
+
+namespace zmq
+{
+const char hello_prefix[] = "\x05WELCOME";
+const size_t hello_prefix_len = sizeof (hello_prefix) - 1;
+
+const char welcome_prefix[] = "\x07WELCOME";
+const size_t welcome_prefix_len = sizeof (welcome_prefix) - 1;
+
+const char initiate_prefix[] = "\x08INITIATE";
+const size_t initiate_prefix_len = sizeof (initiate_prefix) - 1;
+
+const char ready_prefix[] = "\x05READY";
+const size_t ready_prefix_len = sizeof (ready_prefix) - 1;
+
+const char error_prefix[] = "\x05ERROR";
+const size_t error_prefix_len = sizeof (error_prefix) - 1;
+
+const size_t brief_len_size = sizeof (char);
+}
+
+#endif

src/plain_server.cpp

@@ -36,6 +36,7 @@
 #include "err.hpp"
 #include "plain_server.hpp"
 #include "wire.hpp"
+#include "plain_common.hpp"
 
 zmq::plain_server_t::plain_server_t (session_base_t *session_,
                                      const std::string &peer_address_,
@@ -121,9 +122,6 @@ int zmq::plain_server_t::process_hello (msg_t *msg_)
     const char *ptr = static_cast<char *> (msg_->data ());
     size_t bytes_left = msg_->size ();
 
-    const char hello_prefix[] = "\x05HELLO";
-    const size_t hello_prefix_len = sizeof (hello_prefix) - 1;
-
     if (bytes_left < hello_prefix_len
         || memcmp (ptr, hello_prefix, hello_prefix_len)) {
         session->get_socket ()->event_handshake_failed_protocol (
@@ -208,8 +206,6 @@ int zmq::plain_server_t::process_hello (msg_t *msg_)
 
 int zmq::plain_server_t::produce_welcome (msg_t *msg_) const
 {
-    const char welcome_prefix[] = "\x07WELCOME";
-    const size_t welcome_prefix_len = sizeof (welcome_prefix) - 1;
     const int rc = msg_->init_size (welcome_prefix_len);
     errno_assert (rc == 0);
     memcpy (msg_->data (), welcome_prefix, welcome_prefix_len);
@@ -221,8 +217,6 @@ int zmq::plain_server_t::process_initiate (msg_t *msg_)
     const unsigned char *ptr = static_cast<unsigned char *> (msg_->data ());
     const size_t bytes_left = msg_->size ();
 
-    const char initiate_prefix[] = "\x08INITIATE";
-    const size_t initiate_prefix_len = sizeof (initiate_prefix) - 1;
     if (bytes_left < initiate_prefix_len
         || memcmp (ptr, initiate_prefix, initiate_prefix_len)) {
         session->get_socket ()->event_handshake_failed_protocol (
@@ -239,8 +233,6 @@ int zmq::plain_server_t::process_initiate (msg_t *msg_)
 
 int zmq::plain_server_t::produce_ready (msg_t *msg_) const
 {
-    const char ready_prefix[] = "\5READY";
-    const size_t ready_prefix_len = sizeof (ready_prefix) - 1;
     make_command_with_basic_properties (msg_, ready_prefix, ready_prefix_len);
 
     return 0;
@@ -248,17 +240,16 @@ int zmq::plain_server_t::produce_ready (msg_t *msg_) const
 
 int zmq::plain_server_t::produce_error (msg_t *msg_) const
 {
-    zmq_assert (status_code.length () == 3);
-    const char error_prefix[] = "\5ERROR";
-    const size_t error_prefix_len = sizeof (error_prefix) - 1;
-    const char status_code_len = static_cast<char> (status_code.length ());
-    const size_t status_code_len_size = sizeof (status_code_len);
+    const char expected_status_code_len = 3;
+    zmq_assert (status_code.length ()
+                == static_cast<size_t> (expected_status_code_len));
+    const size_t status_code_len_size = sizeof (expected_status_code_len);
     const int rc = msg_->init_size (error_prefix_len + status_code_len_size
-                                    + status_code_len);
+                                    + expected_status_code_len);
     zmq_assert (rc == 0);
     char *msg_data = static_cast<char *> (msg_->data ());
     memcpy (msg_data, error_prefix, error_prefix_len);
-    msg_data[error_prefix_len] = status_code_len;
+    msg_data[error_prefix_len] = expected_status_code_len;
     memcpy (msg_data + error_prefix_len + status_code_len_size,
             status_code.c_str (), status_code.length ());
     return 0;