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

While poking at the memory problems, tried moving city ids to avoid the
possible overrun, and was checking how ids were allocated.  Discovered that
the running game never marks the used id numbers!  And never checks they've
run out of id numbers!

Problem goes back as far as 2.0 (didn't look any farther back).

When savegames are loaded, the id used bits *are* reserved.  But the counter
starts again at 100, so reloaded games will not have the same new ids after
that point in the game.

For Leave and Load games, the id counter and id used bits were not reset, so
the new ids start where the previous game finished.

Worse, because two different games could be loaded, the various saved bits
from the previous games are merged, further confounding the issues.

Although this may not be a problem in most games, for large or long games
some units' numbers may be duplicated.  And it makes direct comparisons of
saved games absolutely impossible (unless both start at the beginning).

Redid the entire scheme (with symbols)!  Set the 0th bit in the used array,
instead of testing against 0 every call.  A tiny improvement in efficiency.

I'm now using server_game_init() properly, so the rulesets are no longer
loaded twice for --file games.  Another small improvement in efficiency....

Left in my debugging swaps of name and/or id for city and unit.

Other minor cleanup.

Index: server/srv_main.c
===================================================================
--- server/srv_main.c   (revision 14380)
+++ server/srv_main.c   (working copy)
@@ -129,10 +129,8 @@
 */
 bool force_end_of_sniff;
 
-/* this counter creates all the id numbers used */
-/* use get_next_id_number()                     */
-static unsigned short global_id_counter=100;
-static unsigned char used_ids[8192]={0};
+#define IDENTITY_NUMBER_SIZE (1+MAX_UINT16)
+static unsigned char identity_numbers_used[IDENTITY_NUMBER_SIZE/8]={0};
 
 /* server initialized flag */
 static bool has_been_srv_init = FALSE;
@@ -196,7 +194,7 @@
   init_character_encodings(FC_DEFAULT_DATA_ENCODING, FALSE);
 
   /* Initialize callbacks. */
-  game.callbacks.unit_deallocate = dealloc_id;
+  game.callbacks.unit_deallocate = identity_number_release;
 
   /* done */
   return;
@@ -1076,37 +1074,43 @@
 /**************************************************************************
 ...
 **************************************************************************/
-void dealloc_id(int id)
+void identity_number_release(int id)
 {
-  used_ids[id/8]&= 0xff ^ (1<<(id%8));
+  identity_numbers_used[id/8] &= 0xff ^ (1<<(id%8));
 }
 
 /**************************************************************************
 ...
 **************************************************************************/
-static bool is_id_allocated(int id)
+void identity_number_reserve(int id)
 {
-  return TEST_BIT(used_ids[id / 8], id % 8);
+  identity_numbers_used[id/8] |= (1<<(id%8));
 }
 
 /**************************************************************************
 ...
 **************************************************************************/
-void alloc_id(int id)
+static bool identity_number_is_used(int id)
 {
-  used_ids[id/8]|= (1<<(id%8));
+  return TEST_BIT(identity_numbers_used[id/8], id%8);
 }
 
 /**************************************************************************
-...
+  Truncation of unsigned short wraps at 65K, skipping VISION_SITE_NONE (0)
+  Setup in server_game_init()
 **************************************************************************/
+int identity_number(void)
+{
+  int retries = 0;
 
-int get_next_id_number(void)
-{
-  while (is_id_allocated(++global_id_counter) || global_id_counter == 0) {
-    /* nothing */
+  while (identity_number_is_used(++server.identity_number)) {
+    /* try again */
+    if (++retries >= IDENTITY_NUMBER_SIZE) {
+      die("exhausted city and unit numbers!");
+    }
   }
-  return global_id_counter;
+  identity_number_reserve(server.identity_number);
+  return server.identity_number;
 }
 
 /**************************************************************************
@@ -1934,9 +1938,12 @@
 #endif /* HAVE_AUTH */
 
   /* load a saved game */
-  if (srvarg.load_filename[0] != '\0') {
-    (void) load_command(NULL, srvarg.load_filename, FALSE);
-  } 
+  if ('\0' == srvarg.load_filename[0]
+   || !load_command(NULL, srvarg.load_filename, FALSE)) {
+    /* Rulesets are loaded on game initialization, but may be changed later
+     * if /load or /rulesetdir is done. */
+    load_rulesets();
+  }
 
   if(!(srvarg.metaserver_no_send)) {
     freelog(LOG_NORMAL, _("Sending info to metaserver [%s]"),
@@ -2131,13 +2138,14 @@
 **************************************************************************/
 void server_game_init(void)
 {
-  game_init();
-
+  /* was redundantly in game_load() */
   server.nbarbarians = 0;
+  server.identity_number = IDENTITY_NUMBER_START;
 
-  /* Rulesets are loaded on game initialization, but may be changed later
-   * if /load or /rulesetdir is done. */
-  load_rulesets();
+  memset(identity_numbers_used, 0, sizeof(identity_numbers_used));
+  identity_number_reserve(VISION_SITE_NONE);
+
+  game_init();
 }
 
 /**************************************************************************
@@ -2207,6 +2215,7 @@
     /* Reset server */
     server_game_free();
     server_game_init();
+    load_rulesets();
     game.info.is_new_game = TRUE;
     set_server_state(S_S_INITIAL);
   } while (TRUE);
Index: server/srv_main.h
===================================================================
--- server/srv_main.h   (revision 14380)
+++ server/srv_main.h   (working copy)
@@ -61,6 +61,13 @@
  * TODO: Lots more variables could be added here. */
 extern struct civserver {
   int nbarbarians;
+
+  /* this counter creates all the city and unit numbers used.
+   * arbitrarily starts at 100, but at 65K wraps to 1.
+   * use identity_number()
+   */
+#define IDENTITY_NUMBER_START (100)
+  unsigned short identity_number;
 } server;
 
 
@@ -83,9 +90,9 @@
                             char *newname);
 void send_all_info(struct conn_list *dest);
 
-void dealloc_id(int id);
-void alloc_id(int id);
-int get_next_id_number(void);
+void identity_number_release(int id);
+void identity_number_reserve(int id);
+int identity_number(void);
 int player_count_no_barbarians(void);
 void server_game_init(void);
 void server_game_free(void);
Index: server/citytools.c
===================================================================
--- server/citytools.c  (revision 14380)
+++ server/citytools.c  (working copy)
@@ -964,7 +964,7 @@
   freelog(LOG_DEBUG, "Creating city %s", name);
 
   pcity->ai.trade_want = TRADE_WEIGHTING;
-  pcity->id = get_next_id_number();
+  pcity->id = identity_number();
   idex_register_city(pcity);
 
   if (!pplayer->capital) {
@@ -1172,9 +1172,10 @@
 
   /* idex_unregister_city() is called in game_remove_city() below */
 
-  /* dealloc_id(pcity->id); We do NOT dealloc cityID's as the cities may still 
be
-     alive in the client. As the number of removed cities is small the leak is
-     acceptable. */
+  /* identity_number_release(pcity->id) is *NOT* done!  The cities may still be
+     alive in the client, or in the player map.  The number of removed 
+     cities is small, so the loss is acceptable.
+   */
 
   old_vision = pcity->server.vision;
   pcity->server.vision = NULL;
Index: server/maphand.c
===================================================================
--- server/maphand.c    (revision 14380)
+++ server/maphand.c    (working copy)
@@ -1204,7 +1204,7 @@
   struct vision_site *psite = map_get_player_site(ptile, pplayer);
 
   if (NULL != psite && ptile == psite->location
-   && VISION_SITE_RUIN < psite->identity) {
+   && VISION_SITE_NONE < psite->identity) {
     return psite;
   }
   return NULL;
@@ -1814,13 +1814,13 @@
     /* no borders */
     return;
   }
-  if (VISION_SITE_RUIN == psite->identity) {
+  if (VISION_SITE_NONE == psite->identity) {
     /* should never be called! */
-    freelog(LOG_VERBOSE, "Warning: border source (%d,%d) is ruin!",
+    freelog(LOG_ERROR, "Warning: border source (%d,%d) is unknown!",
             TILE_XY(ptile));
     return;
   }
-  if (VISION_SITE_RUIN < psite->identity) {
+  if (VISION_SITE_NONE < psite->identity) {
     /* city expansion */
     range = MIN(psite->size + 1, game.info.borders);
     if (psite->size > game.info.borders) {
@@ -1844,7 +1844,7 @@
       int r = sq_map_distance(dsite->location, dtile);
 
       /* border tile claimed by another */
-      if (VISION_SITE_RUIN == dsite->identity) {
+      if (VISION_SITE_NONE == dsite->identity) {
         /* ruins don't keep their borders */
         dsite->owner = powner;
         tile_set_owner(dtile, powner);
Index: server/unittools.c
===================================================================
--- server/unittools.c  (revision 14380)
+++ server/unittools.c  (working copy)
@@ -1303,7 +1303,7 @@
   struct city *pcity;
 
   /* Register unit */
-  punit->id = get_next_id_number();
+  punit->id = identity_number();
   idex_register_unit(punit);
 
   assert(ptile != NULL);
Index: server/stdinhand.c
===================================================================
--- server/stdinhand.c  (revision 14380)
+++ server/stdinhand.c  (working copy)
@@ -3309,9 +3309,8 @@
 
   /* we found it, free all structures */
   server_game_free();
+  server_game_init();
 
-  game_init();
-
   loadtimer = new_timer_start(TIMER_CPU, TIMER_ACTIVE);
   uloadtimer = new_timer_start(TIMER_USER, TIMER_ACTIVE);
 
Index: server/savegame.c
===================================================================
--- server/savegame.c   (revision 14380)
+++ server/savegame.c   (working copy)
@@ -1546,7 +1546,7 @@
     punit = create_unit_virtual(plr, NULL, type,
        secfile_lookup_int(file, "player%d.u%d.veteran", plrno, i));
     punit->id = secfile_lookup_int(file, "player%d.u%d.id", plrno, i);
-    alloc_id(punit->id);
+    identity_number_reserve(punit->id);
     idex_register_unit(punit);
 
     nat_x = secfile_lookup_int(file, "player%d.u%d.x", plrno, i);
@@ -2209,7 +2209,7 @@
     tile_set_owner(ptile, plr);
 
     pcity->id=secfile_lookup_int(file, "player%d.c%d.id", plrno, i);
-    alloc_id(pcity->id);
+    identity_number_reserve(pcity->id);
     idex_register_city(pcity);
 
     id = secfile_lookup_int_default(file, -1,
@@ -2739,7 +2739,7 @@
        struct vision_site *pdcity = fc_calloc(1, sizeof(*pdcity));
 
        pdcity->identity = secfile_lookup_int(file, "player%d.dc%d.id", plrno, 
j);
-       if (VISION_SITE_RUIN >= pdcity->identity) {
+       if (VISION_SITE_NONE >= pdcity->identity) {
          freelog(LOG_ERROR, "[player%d] dc%d has invalid id (%d); skipping.",
                  plrno, j, pdcity->identity);
          free(pdcity);
@@ -2791,7 +2791,7 @@
        }
 
        map_get_player_tile(ptile, plr)->site = pdcity;
-       alloc_id(pdcity->identity);
+       identity_number_reserve(pdcity->identity);
       }
     }
 
@@ -3804,35 +3804,42 @@
       }
     }
 
-    game.info.aqueductloss = secfile_lookup_int_default(file, 
game.info.aqueductloss,
-                                                  "game.aqueductloss");
-    game.info.killcitizen = secfile_lookup_int_default(file, 
game.info.killcitizen,
-                                                 "game.killcitizen");
-    game.info.savepalace = secfile_lookup_bool_default(file, 
game.info.savepalace,
-                                               "game.savepalace");
-    game.info.turnblock = secfile_lookup_bool_default(file, 
game.info.turnblock,
-                                               "game.turnblock");
-    game.info.fixedlength = secfile_lookup_bool_default(file, 
game.info.fixedlength,
-                                                 "game.fixedlength");
-    game.info.barbarianrate = secfile_lookup_int_default(file, 
game.info.barbarianrate,
-                                                   "game.barbarians");
-    game.info.onsetbarbarian = secfile_lookup_int_default(file, 
game.info.onsetbarbarian,
-                                                    "game.onsetbarbs");
-    game.info.revolution_length
-      = secfile_lookup_int_default(file, game.info.revolution_length,
-                                  "game.revolen");
-    server.nbarbarians = 0; /* counted in player_load for compatibility with 
-                            1.10.0 */
-    game.info.occupychance = secfile_lookup_int_default(file, 
game.info.occupychance,
-                                                  "game.occupychance");
-    game.info.autoattack = secfile_lookup_bool_default(file,
-                                                 GAME_DEFAULT_AUTOATTACK,
-                                                  "game.autoattack");
-    game.seed = secfile_lookup_int_default(file, game.seed,
-                                          "game.randseed");
+    game.info.aqueductloss =
+      secfile_lookup_int_default(file, game.info.aqueductloss,
+                                 "game.aqueductloss");
+    game.info.killcitizen =
+      secfile_lookup_int_default(file, game.info.killcitizen,
+                                 "game.killcitizen");
+    game.info.savepalace =
+      secfile_lookup_bool_default(file, game.info.savepalace,
+                                  "game.savepalace");
+    game.info.turnblock =
+      secfile_lookup_bool_default(file, game.info.turnblock,
+                                  "game.turnblock");
+    game.info.fixedlength =
+      secfile_lookup_bool_default(file, game.info.fixedlength,
+                                  "game.fixedlength");
+    game.info.barbarianrate =
+      secfile_lookup_int_default(file, game.info.barbarianrate,
+                                 "game.barbarians");
+    game.info.onsetbarbarian =
+      secfile_lookup_int_default(file, game.info.onsetbarbarian,
+                                 "game.onsetbarbs");
+    game.info.revolution_length =
+      secfile_lookup_int_default(file, game.info.revolution_length,
+                                 "game.revolen");
+    game.info.occupychance =
+      secfile_lookup_int_default(file, game.info.occupychance,
+                                 "game.occupychance");
+    game.info.autoattack =
+      secfile_lookup_bool_default(file, GAME_DEFAULT_AUTOATTACK,
+                                  "game.autoattack");
+    game.seed =
+      secfile_lookup_int_default(file, game.seed,
+                                 "game.randseed");
     game.info.allowed_city_names =
-       secfile_lookup_int_default(file, game.info.allowed_city_names,
-                                  "game.allowed_city_names"); 
+      secfile_lookup_int_default(file, game.info.allowed_city_names,
+                                 "game.allowed_city_names"); 
 
     if(civstyle == 1) {
       string = "civ1";
@@ -4061,6 +4068,10 @@
       }
     }
 
+    server.identity_number =
+      secfile_lookup_int_default(file, server.identity_number,
+                                 "game.identity_number_used");
+
     /* Initialize nations we loaded from rulesets. This has to be after
      * map loading and before we seek nations for players */
     init_available_nations();
@@ -4388,8 +4399,8 @@
   secfile_insert_int(file, game.info.diplchance, "game.diplchance");
   secfile_insert_int(file, game.info.aqueductloss, "game.aqueductloss");
   secfile_insert_int(file, game.info.killcitizen, "game.killcitizen");
+  secfile_insert_bool(file, game.info.savepalace, "game.savepalace");
   secfile_insert_bool(file, game.info.turnblock, "game.turnblock");
-  secfile_insert_bool(file, game.info.savepalace, "game.savepalace");
   secfile_insert_bool(file, game.info.fixedlength, "game.fixedlength");
   secfile_insert_int(file, game.info.barbarianrate, "game.barbarians");
   secfile_insert_int(file, game.info.onsetbarbarian, "game.onsetbarbs");
@@ -4491,6 +4502,8 @@
     temp[improvement_count()] = '\0';
     secfile_insert_str(file, temp, "game.destroyed_wonders_new");
 
+    secfile_insert_int(file, server.identity_number, 
"game.identity_number_used");
+
     calc_unit_ordering();
 
     players_iterate(pplayer) {
Index: common/unit.h
===================================================================
--- common/unit.h       (revision 14380)
+++ common/unit.h       (working copy)
@@ -138,10 +138,11 @@
 
 struct unit {
   struct unit_type *utype;
+  struct tile *tile;
+  struct player *owner; /* Cannot be NULL. */
   int id;
-  struct player *owner; /* Cannot be NULL. */
-  struct tile *tile;
   int homecity;
+
   int moves_left;
   int hp;
   int veteran;
Index: common/city.h
===================================================================
--- common/city.h       (revision 14380)
+++ common/city.h       (working copy)
@@ -255,10 +255,11 @@
 };
 
 struct city {
+  char name[MAX_LEN_NAME];
+  struct tile *tile;
+  struct player *owner; /* Cannot be NULL. */
+  struct player *original; /* Cannot be NULL. */
   int id;
-  struct player *owner; /* Cannot be NULL. */
-  struct tile *tile;
-  char name[MAX_LEN_NAME];
 
   /* the people */
   int size;
@@ -332,7 +333,7 @@
   int rapture;                /* rapture rounds count */ 
   bool was_happy;
   bool airlift;
-  struct player *original;     /* original owner - cannot be NULL */
+
   bv_city_options city_options;
 
   /* server variable. indicates if the city map is synced with the client. */
Index: common/vision.h
===================================================================
--- common/vision.h     (revision 14380)
+++ common/vision.h     (working copy)
@@ -108,9 +108,10 @@
 /* This is copied in maphand.c really_give_tile_info_from_player_to_player(),
  * so be careful with pointers!
  */
-#define VISION_SITE_RUIN (0)
+#define VISION_SITE_NONE (0)
 
 struct vision_site {
+  char name[MAX_LEN_NAME];
   struct tile *location;               /* Cannot be NULL */
   struct player *owner;                        /* May be NULL, always check! */
 
@@ -122,7 +123,6 @@
   unsigned short size;
 
   bv_imprs improvements;
-  char name[MAX_LEN_NAME];
 };
 
 #define vision_owner(v) ((v)->owner)
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to