<URL: http://bugs.freeciv.org/Ticket/Display.html?id=39960 >

Implement part of the bug fix proposal in PR#39578.

There are more than 80 calls.  Concentrating on those sent initially.

Because send_player_info() is hidden inside certain functions, remove and
scatter to various code branches instead to reveal code flow.  This also
fixes several potential bugs.

Moreover, in most places that send_all_info() was called, it was immediately
followed by send_player_info(NULL, NULL), duplicated to the connection!

Generally, send_conn_info() after send_game_info() and send_player_info()
because of player number.  This also fixes several potential bugs.

server/connecthand.c
   establish_new_connection()
     repeat calls in various code branches to avoid sending redundant packets.
     bug fix: send_conn_info() after send_game_info() and send_player_info()

   attach_connection_to_player()
     remove redundant calls, scatter to various code branches instead:
       send_game_info(NULL)
       send_player_info(pplayer, NULL)

   lost_connection_to_client()
     send_player_info(pplayer, NULL) was only sent in S_S_RUNNING,
     but also sent sometimes by toggle_ai_player_direct();
     now always send for consistency.

server/srv_main.c
   send_all_info()
     remove redundant call after each call, include within function:
       send_player_info(NULL, NULL)

server/stdinhand.c
   toggle_ai_player_direct()
     remove redundant call, scatter to various code branches instead:
       send_player_info(pplayer, NULL)
     (allows the calls to be moved ahead of sending *_info)

   detach_command()
     bug fix? detaching observer should be sent rulesets (like /take)?




Index: server/srv_main.c
===================================================================
--- server/srv_main.c   (revision 14150)
+++ server/srv_main.c   (working copy)
@@ -352,7 +352,8 @@
 
   send_game_info(dest);
   send_map_info(dest);
-  send_player_info_c(NULL, dest);
+  /* avoid redundancy, send to everybody */
+  send_player_info_c(NULL, game.est_connections);
   send_conn_info(game.est_connections, dest);
   send_spaceship_info(NULL, dest);
   send_all_known_tiles(dest);
Index: server/srv_main.h
===================================================================
--- server/srv_main.h   (revision 14150)
+++ server/srv_main.h   (working copy)
@@ -59,6 +59,7 @@
 enum server_states server_state(void);
 void set_server_state(enum server_states newstate);
 
+void check_for_full_turn_done(void);
 bool check_for_game_over(void);
 
 bool server_packet_input(struct connection *pconn, void *packet, int type);
@@ -67,14 +68,12 @@
 void pick_random_player_name(const struct nation_type *pnation,
                             char *newname);
 void send_all_info(struct conn_list *dest);
-void check_for_full_turn_done(void);
 
 void dealloc_id(int id);
 void alloc_id(int id);
 int get_next_id_number(void);
 void server_game_init(void);
 void server_game_free(void);
-void check_for_full_turn_done(void);
 void aifill(int amount);
 
 extern struct server_arguments srvarg;
Index: server/connecthand.c
===================================================================
--- server/connecthand.c        (revision 14150)
+++ server/connecthand.c        (working copy)
@@ -112,33 +112,50 @@
   if ((pplayer = find_player_by_user(pconn->username))) {
     attach_connection_to_player(pconn, pplayer);
 
+    if (game.info.auto_ai_toggle && pplayer->ai.control) {
+      toggle_ai_player_direct(NULL, pplayer);
+    }
+
     if (S_S_RUNNING == server_state()) {
       /* Player and other info is only updated when the game is running.
        * See the comment in lost_connection_to_client(). */
       send_packet_freeze_hint(pconn);
       send_all_info(dest);
-      send_player_info(NULL,NULL);
       send_diplomatic_meetings(pconn);
       send_packet_thaw_hint(pconn);
       dsend_packet_start_phase(pconn, game.info.phase);
+    } else {
+      /* send new player connection to everybody */
+      send_game_info(game.est_connections);
+      send_player_info_c(NULL, game.est_connections);
+      send_conn_info(game.est_connections, dest);
     }
+  } else {
+    if (S_S_INITIAL == server_state() && game.info.is_new_game) {
+      if (attach_connection_to_player(pconn, NULL)) {
+        sz_strlcpy(pconn->player->name, pconn->username);
 
-    /* This must be done after the above info is sent, because it will
-     * generate a player-packet which can't be sent until (at least)
-     * rulesets are sent. */
-    if (game.info.auto_ai_toggle && pplayer->ai.control) {
-      toggle_ai_player_direct(NULL, pplayer);
-    }
+        /* send new player connection to everybody */
+        send_game_info(game.est_connections);
+        send_player_info_c(NULL, game.est_connections);
+      } else {
+        notify_conn(dest, NULL, E_CONNECTION,
+                    _("Couldn't attach your connection to new player."));
+        freelog(LOG_VERBOSE, "%s is not attached to a player", 
pconn->username);
 
-  } else if (S_S_INITIAL == server_state() && game.info.is_new_game) {
-    if (!attach_connection_to_player(pconn, NULL)) {
-      notify_conn(dest, NULL, E_CONNECTION,
-                 _("Couldn't attach your connection to new player."));
-      freelog(LOG_VERBOSE, "%s is not attached to a player", pconn->username);
+        /* send old player connections to self */
+        send_game_info(dest);
+        send_player_info_c(NULL, dest);
+      }
     } else {
-      sz_strlcpy(pconn->player->name, pconn->username);
+      /* send old player connections to self */
+      send_game_info(dest);
+      send_player_info_c(NULL, dest);
     }
+    send_conn_info(game.est_connections, dest);
   }
+  /* redundant self to self cannot be avoided */
+  send_conn_info(dest, game.est_connections);
 
   /* remind the connection who he is */
   if (!pconn->player) {
@@ -176,10 +193,6 @@
     show_players(pconn);
   }
 
-  send_conn_info(dest, game.est_connections);
-  send_conn_info(game.est_connections, dest);
-  send_game_info(dest);
-  send_player_info_c(NULL, dest);
   reset_all_start_commands();
   (void) send_server_info_to_metaserver(META_INFO);
 }
@@ -289,7 +302,7 @@
 /**************************************************************************
   High-level server stuff when connection to client is closed or lost.
   Reports loss to log, and to other players if the connection was a
-  player.  Also removes player if in pregame, applies auto_toggle, and
+  player.  Also removes player in pregame, applies auto_toggle, and
   does check for turn done (since can depend on connection/ai status).
   Note caller should also call close_connection() after this, to do
   lower-level close stuff.
@@ -317,14 +330,6 @@
 
   unattach_connection_from_player(pconn);
   send_conn_info_remove(pconn->self, game.est_connections);
-
-  if (S_S_RUNNING == server_state()) {
-    /* Player info is only updated when the game is running; this must be
-     * done consistently or the client will end up with inconsistent errors.
-     * At other times, the conn info (send_conn_info) is used by the client
-     * to display player information.  See establish_new_connection(). */
-    send_player_info(pplayer, NULL);
-  }
   notify_if_first_access_level_is_available();
 
   if (game.info.is_new_game
@@ -341,6 +346,14 @@
 
     check_for_full_turn_done();
   }
+  /* send_player_info() was formerly updated by toggle_ai_player_direct(),
+   * so it must be safe to send here now?
+   *
+   * At other times, data from send_conn_info() is used by the client to
+   * display player information.  See establish_new_connection().
+   */
+  send_player_info_c(pplayer, game.est_connections);
+
   reset_all_start_commands();
 
   delayed_disconnect--;
@@ -439,9 +452,6 @@
   pconn->player = pplayer;
   conn_list_append(pplayer->connections, pconn);
 
-  send_game_info(NULL);
-  send_player_info(pplayer, NULL);
-
   aifill(game.info.aifill);
 
   return TRUE;
Index: server/settings.c
===================================================================
--- server/settings.c   (revision 14150)
+++ server/settings.c   (working copy)
@@ -23,6 +23,7 @@
 #include "map.h"
 
 #include "ggzserver.h"
+#include "plrhand.h"
 #include "report.h"
 #include "settings.h"
 #include "srv_main.h"
@@ -60,6 +61,7 @@
   players_iterate(pplayer) {
     if (!pplayer->ai.control && !pplayer->is_connected) {
       toggle_ai_player_direct(NULL, pplayer);
+      send_player_info_c(pplayer, game.est_connections);
     }
   } players_iterate_end;
 
Index: server/stdinhand.c
===================================================================
--- server/stdinhand.c  (revision 14150)
+++ server/stdinhand.c  (working copy)
@@ -791,14 +791,12 @@
      * new meeting */
     cancel_all_meetings(pplayer);
   }
-
-  send_player_info(pplayer, NULL);
 }
 
 /**************************************************************************
 ...
 **************************************************************************/
-static bool toggle_ai_player(struct connection *caller, char *arg, bool check)
+static bool toggle_ai_command(struct connection *caller, char *arg, bool check)
 {
   enum m_pre_result match_result;
   struct player *pplayer;
@@ -810,6 +808,7 @@
     return FALSE;
   } else if (!check) {
     toggle_ai_player_direct(caller, pplayer);
+    send_player_info_c(pplayer, game.est_connections);
   }
   return TRUE;
 }
@@ -2837,16 +2836,21 @@
   } else {
     unattach_connection_from_player(pconn);
   }
-  send_conn_info(pconn->self, game.est_connections);
 
   if (S_S_RUNNING == server_state()) {
     send_packet_freeze_hint(pconn);
     send_all_info(pconn->self);
-    send_player_info(NULL, NULL);
     send_diplomatic_meetings(pconn);
     send_packet_thaw_hint(pconn);
     dsend_packet_start_phase(pconn, game.info.phase);
+  } else {
+    /* send changed player connection to everybody */
+    send_game_info(game.est_connections);
+    send_player_info_c(pplayer, game.est_connections);
+    /* we already know existing connections */
   }
+  /* redundant self to self cannot be avoided */
+  send_conn_info(pconn->self, game.est_connections);
 
   if (pplayer) {
     cmd_reply(CMD_OBSERVE, caller, C_OK, _("%s now observes %s"),
@@ -2942,13 +2946,11 @@
     goto end;
   }
 
-  /* if we want to switch players, reset the client if the game is running */
+  /* if we want to take while the game is running, reset the client */
   if (S_S_RUNNING == server_state()) {
     send_rulesets(pconn->self);
     send_server_settings(pconn->self);
-    send_game_info(pconn->self);
-    send_player_info_c(NULL, pconn->self);
-    send_conn_info(game.est_connections, pconn->self);
+    /* others are sent below */
   }
 
   /* if we're taking another player with a user attached, 
@@ -2997,7 +2999,6 @@
   /* now attach to new player */
   pconn->observer = FALSE; /* do this before attach! */
   attach_connection_to_player(pconn, pplayer);
-  send_conn_info(pconn->self, game.est_connections);
   pplayer = pconn->player; /* In case pplayer was NULL. */
  
   /* if pplayer wasn't /created, and we're still in pregame, change its name */
@@ -3005,21 +3006,26 @@
     sz_strlcpy(pplayer->name, pconn->username);
   }
 
+  /* aitoggle the player back to human as necessary. */
+  if (pplayer->ai.control && game.info.auto_ai_toggle) {
+    toggle_ai_player_direct(NULL, pplayer);
+  }
 
   if (S_S_RUNNING == server_state()) {
     send_packet_freeze_hint(pconn);
     send_all_info(pconn->self);
-    send_player_info(NULL, NULL);
     send_diplomatic_meetings(pconn);
     send_packet_thaw_hint(pconn);
     dsend_packet_start_phase(pconn, game.info.phase);
+  } else {
+    /* send changed player connection to everybody */
+    send_game_info(game.est_connections);
+    send_player_info_c(pplayer, game.est_connections);
+    /* we already know existing connections */
   }
+  /* redundant self to self cannot be avoided */
+  send_conn_info(pconn->self, game.est_connections);
 
-  /* aitoggle the player back to human if necessary. */
-  if (pplayer->ai.control && game.info.auto_ai_toggle) {
-    toggle_ai_player_direct(NULL, pplayer);
-  }
-
   cmd_reply(CMD_TAKE, caller, C_OK, _("%s now controls %s (%s, %s)"), 
             pconn->username, pplayer->name, 
             is_barbarian(pplayer) ? _("Barbarian") : pplayer->ai.control ?
@@ -3113,6 +3119,7 @@
     cmd_reply(CMD_DETACH, caller, C_COMMENT,
              _("%s no longer observing."), pconn->username);
   }
+  /* useful self to self cannot be avoided */
   send_conn_info(pconn->self, game.est_connections);
 
   /* only remove the player if the game is new and in pregame, 
@@ -3124,10 +3131,14 @@
     /* detach any observers */
     conn_list_iterate(pplayer->connections, aconn) {
       if (aconn->observer) {
+       if (S_S_RUNNING == server_state()) {
+         send_rulesets(aconn->self);
+         send_server_settings(aconn->self);
+       }
+        notify_conn(aconn->self, NULL, E_CONNECTION,
+                   _("detaching from %s."), pplayer->name);
         unattach_connection_from_player(aconn);
         send_conn_info(aconn->self, game.est_connections);
-        notify_conn(aconn->self, NULL, E_CONNECTION,
-                   _("detaching from %s."), pplayer->name);
       }
     } conn_list_iterate_end;
 
@@ -3141,6 +3152,7 @@
     /* aitoggle the player if no longer connected. */
     if (game.info.auto_ai_toggle && !pplayer->ai.control) {
       toggle_ai_player_direct(NULL, pplayer);
+      send_player_info_c(pplayer, game.est_connections);
     }
     /* reset username if in pregame. */
     if (is_newgame) {
@@ -3304,6 +3316,9 @@
       }
     } players_iterate_end;
   } conn_list_iterate_end;
+
+  /* unchanged send_game_info() is above */
+  send_player_info_c(NULL, game.est_connections);
   return TRUE;
 }
 
@@ -3566,7 +3581,7 @@
   case CMD_LIST:
     return show_list(caller, arg);
   case CMD_AITOGGLE:
-    return toggle_ai_player(caller, arg, check);
+    return toggle_ai_command(caller, arg, check);
   case CMD_TAKE:
     return take_command(caller, arg, check);
   case CMD_OBSERVE:
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to