add comment, review the code

zjpanghao · Oct 24, 2012 · 481125c · 481125c
1 parent c22a810
commit 481125c
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 58 deletions.
diff --git a/src/client.cpp b/src/client.cpp
@@ -17,24 +17,53 @@ Client::~Client()
 {
     if (m_event) 
     {
-        close(m_sockfd);
         event_free(m_event);
+        close(m_sockfd);
         Server::ClientMapIter iter = m_server->m_client_map.find(m_sockfd);
         m_server->m_client_map.erase(iter);
         DelPluginDataSlots();
     }
 }
 
+bool Client::InitClient(Server *server) 
+{
+    m_server = server;
+
+    m_intemp.reserve(10 * 1024 * 1024);
+    m_inbuf.reserve(10 * 1024 * 1024);
+    m_outbuf.reserve(10 * 1024 * 1024);
+
+    evutil_make_socket_nonblocking(m_sockfd);
+    m_event = event_new(m_server->m_server_base, m_sockfd, EV_PERSIST, Client::ClientEventCallback, this);
+    event_add(m_event, NULL); 
+
+    if (!MakePluginDataSlots())
+    {
+        return false;
+    }
+
+    SetStatus(BEFORE_REQUEST);
+
+    // check to make sure plugin works well
+    if (!StatusMachine())
+    {
+        return false;
+    }
+
+    return true;
+}
+
 bool Client::MakePluginDataSlots()
 {    
     m_plugin_count = m_server->m_plugin_count;
 
+    //there's no plugin at all, return true directly
     if (!m_plugin_count)
     {
         return true;
     }
 
-    m_plugin_data_slots = new void*[m_plugin_count];
+    m_plugin_data_slots = new void*[m_plugin_count]; //no check for NULL, just keep it easy
 
     int i;
 
@@ -47,7 +76,7 @@ bool Client::MakePluginDataSlots()
 
     for (i = 0; i < m_plugin_count; ++ i)
     {
-        if (!plugins[i]->OnInit(this, i))
+        if (!plugins[i]->OnInit(this, i)) 
         {
             return false;
         }
@@ -75,34 +104,6 @@ void Client::DelPluginDataSlots()
     }
 }
 
-bool Client::InitClient(Server *server) 
-{
-    m_server = server;
-
-    m_intemp.reserve(10 * 1024 * 1024);
-    m_inbuf.reserve(10 * 1024 * 1024);
-    m_outbuf.reserve(10 * 1024 * 1024);
-
-    evutil_make_socket_nonblocking(m_sockfd);
-    m_event = event_new(m_server->m_server_base, m_sockfd, EV_PERSIST, Client::ClientEventCallback, this);
-    event_add(m_event, NULL); 
-
-    if (!MakePluginDataSlots())
-    {
-        return false;
-    }
-
-    SetStatus(BEFORE_REQUEST);
-
-    // make sure plugin works well
-    if (!StatusMachine())
-    {
-        return false;
-    }
-
-    return true;
-}
-
 void Client::WantRead()
 {
     m_want_read = true;
@@ -121,6 +122,8 @@ void Client::NotWantRead()
     event_add(m_event, NULL); 
 }
 
+// client really want write, 
+// which means we should keep the write event whether or not there's anything to write right now.
 void Client::WantWrite() 
 {
     m_want_write = true;
@@ -129,8 +132,10 @@ void Client::WantWrite()
 
 void Client::NotWantWrite()
 {
-    m_want_write = false;
-    if (!m_outbuf.size())
+    m_want_write = false; //we really don't want to write any more.
+
+    //but if there's anything left, just keep the write event to send off the left bytes.
+    if (!m_outbuf.size())     
     {
         UnsetWriteEvent();
     }
@@ -171,7 +176,8 @@ void Client::ClientEventCallback(evutil_socket_t sockfd, short event, void *user
         }
         else if (ret == 0)
         {
-            delete client;
+            delete client; //client wants to leave, 
+                           //so let it leave whether or not there's still any data waiting to send to it.
             return;
         }
         else
@@ -196,16 +202,18 @@ void Client::ClientEventCallback(evutil_socket_t sockfd, short event, void *user
         {
             client->m_outbuf.erase(client->m_outbuf.begin(), client->m_outbuf.begin() + ret);
 
+            //if client really want to keep write event, we can't unregister write event, even though there's nothing to send.
             if (client->m_outbuf.size() == 0 && !client->m_want_write)
             {
                 client->UnsetWriteEvent();
             }
         }
     }
 
+    // core logic, just call status machine to handle the whole thing, it's so easy :)
     if (!client->StatusMachine())
     {
-        delete client;
+        delete client; //some error happend, kick out the client
     }
 }
 

diff --git a/src/server.cpp b/src/server.cpp
@@ -53,15 +53,15 @@ bool Server::SetupPlugins()
     {
         plugin_path = plugin_config[plugin_index];
 
-        void *so = dlopen(plugin_path, RTLD_LAZY);
+        void *so    = dlopen(plugin_path, RTLD_LAZY);
 
         if (!so)
         {
             std::cerr << dlerror() << std::endl;
             return false;
         }
 
-        Plugin::SetupPlugin  sp = (Plugin::SetupPlugin)dlsym(so, "SetupPlugin");
+        Plugin::SetupPlugin  sp = (Plugin::SetupPlugin )dlsym(so, "SetupPlugin");
         Plugin::RemovePlugin rp = (Plugin::RemovePlugin)dlsym(so, "RemovePlugin");
 
         if (!sp || !rp)
@@ -79,13 +79,12 @@ bool Server::SetupPlugins()
             return false;
         }
 
-        plugin->m_setup_plugin = sp;
+        plugin->m_setup_plugin  = sp;
         plugin->m_remove_plugin = rp;
 
-        m_plugins = (Plugin* *)realloc(m_plugins, sizeof(*m_plugins) * (m_plugin_count + 1));
+        m_plugins = (Plugin* *) realloc(m_plugins, sizeof(*m_plugins) * (m_plugin_count + 1));
         m_plugins[m_plugin_count++] = plugin;
 
-        plugin->m_plugin_data  = NULL;
         plugin->m_plugin_so    = so;
         plugin->m_plugin_index = plugin_index;
     }
@@ -100,33 +99,17 @@ void Server::RemovePlugins()
     for (i = 0; i < m_plugin_count; ++ i)
     {
         Plugin *plugin = m_plugins[i];
-        Plugin::RemovePlugin rp = plugin->m_remove_plugin;
+        Plugin::RemovePlugin rp = plugin->m_remove_plugin; //must get a func copy, because plugin will be deleted
         rp(plugin);
         dlclose(plugin->m_plugin_so);
     }
 
     free(m_plugins);
 }
 
-void Server::UnloadPlugins()
-{
-    Plugin *plugin;
-    int     i;
-
-    for (i = 0; i < m_plugin_count; ++ i)
-    {
-        plugin = m_plugins[i];
-
-        if (plugin->m_is_loaded)
-        {
-            plugin->OnDestroy(this, i);
-        }
-    }
-}
-
 bool Server::LoadPlugins()
 {
-    int i;
+    int     i;
     Plugin *plugin;
 
     for (i = 0; i < m_plugin_count; ++ i)
@@ -146,6 +129,22 @@ bool Server::LoadPlugins()
     return true;
 }
 
+void Server::UnloadPlugins()
+{
+    Plugin *plugin;
+    int     i;
+
+    for (i = 0; i < m_plugin_count; ++ i)
+    {
+        plugin = m_plugins[i];
+
+        if (plugin->m_is_loaded)
+        {
+            plugin->OnDestroy(this, i);
+        }
+    }
+}
+
 bool Server::StartServer()
 {
     m_server_base = event_base_new();