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

 I tried to track crash in transfer_city() -> transfer_city_units() ->
real_unit_change_homecity(). Never found it (cannot reproduce if
compiled with debugging symbols, or without optimization, or if there
is virtually any code change (changing where instructions are in the
memory)).
 Still, attached is some changes I did in the hunting process. Adds
comments about not-so-obvious dependencies, rearranges code to be more
robust, adds one assert.

 There is no good reason to commit this to stable branch.


 - ML

diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2007-08-11 00:20:33.000000000 +0300
+++ freeciv/server/citytools.c	2007-08-11 22:36:37.000000000 +0300
@@ -591,7 +591,7 @@
     } unit_list_iterate_safe_end;
   }
 
-  /* Any remaining units supported by the city are either given new home
+  /* Any units supported by the city are either given new home
      cities or maybe destroyed */
   unit_list_iterate_safe(units, vunit) {
     struct city *new_home_city = tile_get_city(vunit->tile);
@@ -606,7 +606,7 @@
       transfer_unit(vunit, pcity, verbose);
     } else {
       /* The unit is lost.  Call notify_player (in all other cases it is
-       * called autmatically). */
+       * called automatically). */
       freelog(LOG_VERBOSE, "Lost %s's %s at (%d,%d) when %s was lost.",
 	      unit_owner(vunit)->name,
 	      unit_rule_name(vunit),
@@ -779,8 +779,11 @@
 
   unit_list_iterate(pcity->units_supported, punit) {
     unit_list_prepend(old_city_units, punit);
-    /* otherwise we might delete the homecity from under the unit
-       in the client. */
+    /* Mark unit to have no homecity at all.
+     * 1. We remove unit from units_supported list here,
+     *    real_change_unit_homecity() should not attempt it.
+     * 2. Otherwise we might delete the homecity from under the unit
+     *    in the client */
     punit->homecity = 0;
     send_unit_info(NULL, punit);
   } unit_list_iterate_end;
diff -Nurd -X.diff_ignore freeciv/server/unithand.c freeciv/server/unithand.c
--- freeciv/server/unithand.c	2007-08-05 16:40:57.000000000 +0300
+++ freeciv/server/unithand.c	2007-08-11 22:43:57.000000000 +0300
@@ -282,6 +282,11 @@
   struct player *old_owner = unit_owner(punit);
   struct player *new_owner = city_owner(new_pcity);
 
+  /* Calling this function when new_pcity is same as old_pcity should
+   * be safe with current implementation, but it is not meant to
+   * be used that way. */
+  assert(new_pcity != old_pcity);
+
   if (old_owner != new_owner) {
     vision_clear_sight(punit->server.vision);
     vision_free(punit->server.vision);
@@ -300,11 +305,15 @@
     assert(can_unit_exist_at_tile(punit, new_pcity->tile));
     move_unit(punit, new_pcity->tile, 0); /* teleport to location */
   }
-  
-  unit_list_prepend(new_pcity->units_supported, punit);
+
+  /* Remove from old city first and add to new city only after that.
+   * This is more robust in case old_city == new_city (currently
+   * prohibited by assert in the beginning of the function).
+   */
   if (old_pcity) {
     unit_list_unlink(old_pcity->units_supported, punit);
   }
+  unit_list_prepend(new_pcity->units_supported, punit);
 
   punit->homecity = new_pcity->id;
   send_unit_info(unit_owner(punit), punit);
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to