Robustified code with respect to insertion failures.

10 files changed, 114 insertions, 38 deletions

diff --git a/ChangeLog b/ChangeLog
index cfb7e89a..617837dc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -6,6 +6,14 @@
 	src/game-server/state.cpp, src/game-server/spawnarea.cpp,
 	src/game-server/state.hpp, src/game-server/monster.cpp,
 	src/game-server/trigger.cpp: Updated to new interface.
+	* src/account-server/accounthandler.cpp, src/defines.h,
+	src/chat-server/chathandler.cpp: Merged time-out messages with response
+	to connection messages.
+	* src/game-server/state.cpp, src/game-server/state.hpp: Modified
+	interface to deal with failure to insert objects.
+	* src/scripting/lua.cpp, src/game-server/testing.cpp,
+	src/game-server/command.cpp, src/game-server/gamehandler.cpp: Taken
+	insertion failures into account.
 
 2007-09-22  Guillaume Melquiond  <guillaume.melquiond@gmail.com>
 
diff --git a/src/account-server/accounthandler.cpp b/src/account-server/accounthandler.cpp
index 77e66660..2a046de6 100644
--- a/src/account-server/accounthandler.cpp
+++ b/src/account-server/accounthandler.cpp
@@ -201,7 +201,7 @@ AccountHandler::handleLoginMessage(AccountClient &computer, MessageIn &msg)
 
     if (getClientNumber() >= MAX_CLIENTS )
     {
-        reply.writeByte(LOGIN_SERVER_FULL);
+        reply.writeByte(ERRMSG_SERVER_FULL);
         computer.send(reply);
         return;
     }
@@ -731,7 +731,8 @@ AccountHandler::tokenMatched(AccountClient *computer, int accountID)
 void
 AccountHandler::deletePendingClient(AccountClient* computer)
 {
-    MessageOut msg(APMSG_CONNECTION_TIMEDOUT);
+    MessageOut msg(APMSG_RECONNECT_RESPONSE);
+    msg.writeByte(ERRMSG_TIME_OUT);
     computer->disconnect(msg);
     // The computer will be deleted when the disconnect event is processed
 }
diff --git a/src/chat-server/chathandler.cpp b/src/chat-server/chathandler.cpp
index efa6129a..0cc91acc 100644
--- a/src/chat-server/chathandler.cpp
+++ b/src/chat-server/chathandler.cpp
@@ -61,7 +61,8 @@ bool ChatHandler::startListen(enet_uint16 port)
 
 void ChatHandler::deletePendingClient(ChatClient *c)
 {
-    MessageOut msg(GPMSG_CONNECTION_TIMEDOUT);
+    MessageOut msg(CPMSG_CONNECT_RESPONSE);
+    msg.writeByte(ERRMSG_TIME_OUT);
 
     // The computer will be deleted when the disconnect event is processed
     c->disconnect(msg);
diff --git a/src/defines.h b/src/defines.h
index d4f44b06..9668bb7c 100644
--- a/src/defines.h
+++ b/src/defines.h
@@ -128,10 +128,6 @@ enum {
     PAMSG_RECONNECT                = 0x0065, // B*32 token
     APMSG_RECONNECT_RESPONSE       = 0x0066, // B error
 
-    APMSG_CONNECTION_TIMEDOUT      = 0x0070, // -
-    GPMSG_CONNECTION_TIMEDOUT      = 0x0071, // -
-    CPMSG_CONNECTION_TIMEDOUT      = 0x0072, // -
-
     // Game
     GPMSG_PLAYER_MAP_CHANGE        = 0x0100, // S filename, W x, W y
     GPMSG_PLAYER_SERVER_CHANGE     = 0x0101, // B*32 token, S game address, W game port
@@ -266,13 +262,14 @@ enum {
     ERRMSG_NO_CHARACTER_SELECTED,       // the user needs a character
     ERRMSG_INSUFFICIENT_RIGHTS,         // the user is not privileged
     ERRMSG_INVALID_ARGUMENT,            // part of the received message was invalid
-    ERRMSG_ALREADY_TAKEN                // name used was already taken
+    ERRMSG_ALREADY_TAKEN,               // name used was already taken
+    ERRMSG_SERVER_FULL,                 // the server is overloaded
+    ERRMSG_TIME_OUT                     // data failed to arrive in due time
 };
 
 // Login specific return values
 enum {
     LOGIN_INVALID_VERSION = 0x40,       // the user is using an incompatible protocol
-    LOGIN_SERVER_FULL,                  // the server is overloaded
     LOGIN_BANNED                        // the user is currently banned
 };
 
diff --git a/src/game-server/command.cpp b/src/game-server/command.cpp
index 55ef4d10..a77c2c02 100644
--- a/src/game-server/command.cpp
+++ b/src/game-server/command.cpp
@@ -197,7 +197,7 @@ static void drop(Character *from, ItemClass *it, int nb)
     Item *item = new Item(it, nb);
     item->setMap(from->getMap());
     item->setPosition(from->getPosition());
-    GameState::insert(item);
+    GameState::insertSafe(item);
 }
 
 static void spawn(Character *from, MonsterClass *specy, int nb)
@@ -211,7 +211,11 @@ static void spawn(Character *from, MonsterClass *specy, int nb)
         monster->setMap(map);
         monster->setPosition(pos);
         monster->clearDestination();
-        GameState::insert(monster);
+        if (!GameState::insertSafe(monster))
+        {
+            // The map is full. Break out.
+            break;
+        }
     }
 }
 
diff --git a/src/game-server/gamehandler.cpp b/src/game-server/gamehandler.cpp
index 63a34689..8901a49e 100644
--- a/src/game-server/gamehandler.cpp
+++ b/src/game-server/gamehandler.cpp
@@ -251,7 +251,12 @@ void GameHandler::processMessage(NetComputer *comp, MessageIn &message)
                 Item *item = new Item(ic, amount - nb);
                 item->setMap(computer.character->getMap());
                 item->setPosition(computer.character->getPosition());
-                GameState::insert(item);
+                if (!GameState::insert(item))
+                {
+                    // The map is full. Put back into inventory.
+                    inv.insert(ic->getDatabaseID(), amount - nb);
+                    delete item;
+                }
             }
         } break;
 
@@ -510,11 +515,19 @@ GameHandler::tokenMatched(GameClient* computer, Character* character)
     character->setClient(computer);
 
     MessageOut result(GPMSG_CONNECT_RESPONSE);
+
+    if (!GameState::insert(character))
+    {
+        result.writeByte(ERRMSG_SERVER_FULL);
+        kill(character);
+        delete character;
+        computer->disconnect(result);
+        return;
+    }
+
     result.writeByte(ERRMSG_OK);
     computer->send(result);
 
-    GameState::insert(character);
-
     // Force sending the whole character to the client.
     Inventory(character).sendFull();
     for (int i = 0; i < NB_CHARACTER_ATTRIBUTES; ++i)
@@ -529,7 +542,8 @@ GameHandler::deletePendingClient(GameClient* computer)
     // Something might have changed since it was inserted
     if (computer->status != CLIENT_QUEUED) return;
 
-    MessageOut msg(GPMSG_CONNECTION_TIMEDOUT);
+    MessageOut msg(GPMSG_CONNECT_RESPONSE);
+    msg.writeByte(ERRMSG_TIME_OUT);
 
     // The computer will be deleted when the disconnect event is processed
     computer->disconnect(msg);
diff --git a/src/game-server/state.cpp b/src/game-server/state.cpp
index b9185e92..2f68d6ba 100644
--- a/src/game-server/state.cpp
+++ b/src/game-server/state.cpp
@@ -427,7 +427,7 @@ void GameState::update()
                 break;
 
             case EVENT_INSERT:
-                insert(o);
+                insertSafe(o);
                 break;
 
             case EVENT_WARP:
@@ -439,33 +439,63 @@ void GameState::update()
     delayedEvents.clear();
 }
 
-void GameState::insert(Thing *ptr)
+bool GameState::insert(Thing *ptr)
 {
     assert(!dbgLockObjects);
     MapComposite *map = ptr->getMap();
-    if (!map || !map->insert(ptr))
+    assert(map && map->isActive());
+
+    /* Non-visible objects have neither position nor public ID, so their
+       insertion cannot fail. Take care of them first. */
+    if (!ptr->isVisible())
     {
-        // TODO: Deal with failure to place Thing on the map.
-        return;
+        map->insert(ptr);
+        ptr->inserted();
+        return true;
     }
 
-    ptr->inserted();
+    // Check that coordinates are actually valid.
+    Object *obj = static_cast< Object * >(ptr);
+    Map *mp = map->getMap();
+    Point pos = obj->getPosition();
+    if (pos.x / 32 >= (unsigned)mp->getWidth() ||
+        pos.y / 32 >= (unsigned)mp->getHeight())
+    {
+        LOG_ERROR("Tried to insert an object at position " << pos.x << ','
+                  << pos.y << " outside map " << map->getID() << '.');
+        // Set an arbitrary small position.
+        pos = Point(100, 100);
+        obj->setPosition(pos);
+    }
 
-    if (ptr->isVisible())
+    if (!map->insert(obj))
     {
-        Object *obj = static_cast< Object * >(ptr);
-        obj->raiseUpdateFlags(UPDATEFLAG_NEW_ON_MAP);
-        if (obj->getType() != OBJECT_CHARACTER) return;
-
-        /* Since the character doesn't know yet where on the world he is after
-           connecting to the map server, we send him an initial change map message. */
-        MessageOut mapChangeMessage(GPMSG_PLAYER_MAP_CHANGE);
-        mapChangeMessage.writeString(map->getName());
-        Point pos = obj->getPosition();
-        mapChangeMessage.writeShort(pos.x);
-        mapChangeMessage.writeShort(pos.y);
-        gameHandler->sendTo(static_cast< Character * >(obj), mapChangeMessage);
+        // The map is overloaded. No room to add a new object.
+        LOG_ERROR("Too many objects on map " << map->getID() << '.');
+        return false;
     }
+
+    obj->inserted();
+
+    obj->raiseUpdateFlags(UPDATEFLAG_NEW_ON_MAP);
+    if (obj->getType() != OBJECT_CHARACTER) return true;
+
+    /* Since the player does not know yet where in the world its character is,
+       we send a map-change message, even if it is the first time it
+       connects to this server. */
+    MessageOut mapChangeMessage(GPMSG_PLAYER_MAP_CHANGE);
+    mapChangeMessage.writeString(map->getName());
+    mapChangeMessage.writeShort(pos.x);
+    mapChangeMessage.writeShort(pos.y);
+    gameHandler->sendTo(static_cast< Character * >(obj), mapChangeMessage);
+    return true;
+}
+
+bool GameState::insertSafe(Thing *ptr)
+{
+    if (insert(ptr)) return true;
+    delete ptr;
+    return false;
 }
 
 void GameState::remove(Thing *ptr)
@@ -529,7 +559,12 @@ void GameState::warp(Character *ptr, MapComposite *map, int x, int y)
 
     if (map->isActive())
     {
-        insert(ptr);
+        if (!insert(ptr))
+        {
+            ptr->disconnected();
+            gameHandler->kill(ptr);
+            delete ptr;
+        }
     }
     else
     {
diff --git a/src/game-server/state.hpp b/src/game-server/state.hpp
index 0765984b..67f91311 100644
--- a/src/game-server/state.hpp
+++ b/src/game-server/state.hpp
@@ -40,9 +40,22 @@ namespace GameState
 
     /**
      * Inserts an object in the game world.
+     * @return false if the insertion failed and the object is in limbo.
      * @note No update may be in progress.
      */
-    void insert(Thing *);
+    bool insert(Thing *)
+#   ifdef __GNUC__
+    __attribute__((warn_unused_result))
+#   endif
+    ;
+
+    /**
+     * Inserts an object in the game world. Deletes the object if the insertion
+     * failed.
+     * @return false if the insertion failed.
+     * @note No update may be in progress. Invalid for characters.
+     */
+    bool insertSafe(Thing *);
 
     /**
      * Removes an object from the game world.
diff --git a/src/game-server/testing.cpp b/src/game-server/testing.cpp
index cf125cad..4828df40 100644
--- a/src/game-server/testing.cpp
+++ b/src/game-server/testing.cpp
@@ -19,7 +19,7 @@ static void dropItem(MapComposite *map, int x, int y, int type)
     i->setMap(map);
     Point pos(x, y);
     i->setPosition(pos);
-    GameState::insert(i);
+    GameState::insertSafe(i);
 }
 
 void testingMap(MapComposite *map)
diff --git a/src/scripting/lua.cpp b/src/scripting/lua.cpp
index f33393a5..048bc6f0 100644
--- a/src/scripting/lua.cpp
+++ b/src/scripting/lua.cpp
@@ -178,7 +178,10 @@ static int LuaNpc_Create(lua_State *s)
     }
     q->setMap(m);
     q->setPosition(Point(lua_tointeger(s, 2), lua_tointeger(s, 3)));
-    GameState::insert(q);
+    bool b = GameState::insert(q);
+    /* Do not try to deal with a failure there. There are some serious issues
+       if an insertion failed on an almost empty map. */
+    assert(b); (void)b;
     lua_pushlightuserdata(s, q);
     return 1;
 }