[Freeciv-Dev] Re: (PR#6816) Re: Re: (PR#6819) server crashes when loaded trireme is lost

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=6816 >

Per I. Mathisen wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=6816 >
> 
> On Mon, 10 Nov 2003, Raimar Falke wrote:
> 
>>but Jason doesn't know why "wipe_unit is inherently safe". I have
>>searched the changes of unittools.c and genlist.c but none indicated
>>that something was changed in them that will make removing in an item
>>in a list safe while you iterate over it.
>>
>>So Per, Jason can you shed more light on this?
> 
> 
> Use unit_list_iterate_safe for places where you iterate units while you
> pull the rug underneat your own unit list by deleting units at random in
> the list (wipe_unit can do this due to cargo).

The attached patch does this for all callers (that I could find) of 
wipe_unit, as well as its wrappers bounce_unit and kill_unit. 
(Bounce_unit _very_ rarely wipes a unit, but it does happen.)  My tests 
on autogames show no measurable slowdown.

Wipe_unit does the work that wipe_unit_safe used to do.  Thus it is 
inherently "safe" on its own.  What isn't safe is unit_list_iterate. 
There used to be a wipe_unit_spec_safe() that passed on the iterator 
pointer.  This was removed a little while ago (it is a gross hack).

I think we're all agreed that this whole situation stinks.  I would like 
to see the end of the need for _safe iterators by making them all safe.

One alternative is to rename the current unit_list_iterate_safe as 
unit_list_iterate.  The argument has been made that 
unit_list_iterate_safe is excessively slow; however, I'm not sure that 
this is the case.  It does have an extra malloc, but this can be 
replaced with a variable-sized stack array.

Another alternative is to change genlists so that they are all safe 
(i.e., calling ***_list_remove within a ***_list_iterate is safe).  The 
proposed method is that when _remove is called it doesn't actually free 
the entry from the array, but instead marks it for deletion.  The actual 
deletion occurrs later - the most transparent way is to do it when the 
top-level ***_iterate call is exited or when this element is encountered 
in a top-level ***_iterate finds it.  (Note that iterators may be 
nested, which complicates the problem.  But it is easy for the iterator 
to track its own nesting.)  I suspect a patch for this isn't too hard; 
I'll look into it.

jason

Index: common/unit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.h,v
retrieving revision 1.103
diff -u -r1.103 unit.h
--- common/unit.h       2003/10/29 08:00:39     1.103
+++ common/unit.h       2003/11/10 23:54:27
@@ -13,6 +13,7 @@
 #ifndef FC__UNIT_H
 #define FC__UNIT_H
 
+#include "mem.h"               /* unit_list_iterate_safe */
 #include "terrain.h"           /* enum tile_special_type */
 #include "unittype.h"
 
Index: server/barbarian.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/barbarian.c,v
retrieving revision 1.70
diff -u -r1.70 barbarian.c
--- server/barbarian.c  2003/10/08 16:56:07     1.70
+++ server/barbarian.c  2003/11/10 23:54:28
@@ -195,9 +195,9 @@
   bool alive = TRUE;     /* explorer survived */
 
   if (game.barbarianrate == 0 || (game.year < game.onsetbarbarian)) {
-    unit_list_iterate(map_get_tile(x, y)->units, punit) {
+    unit_list_iterate_safe(map_get_tile(x, y)->units, punit) {
       wipe_unit(punit);
-    } unit_list_iterate_end;
+    } unit_list_iterate_safe_end;
     return FALSE;
   }
 
@@ -256,14 +256,14 @@
         }
       } unit_list_iterate_end;
     } else {             /* The village is surrounded! Kill the explorer. */
-      unit_list_iterate(map_get_tile(x, y)->units, punit2) {
+      unit_list_iterate_safe(map_get_tile(x, y)->units, punit2) {
         if (punit2->owner != me) {
           wipe_unit(punit2);
           alive = FALSE;
         } else {
           send_unit_info(NULL, punit2);
         }
-      } unit_list_iterate_end;
+      } unit_list_iterate_safe_end;
     }
   }
 
Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.240
diff -u -r1.240 citytools.c
--- server/citytools.c  2003/11/07 09:45:03     1.240
+++ server/citytools.c  2003/11/10 23:54:28
@@ -639,7 +639,7 @@
   /* Transfer enemy units in the city to the new owner.
    * Only relevant if we are transferring to another player. */
   if (pplayer != pvictim) {
-    unit_list_iterate(map_get_tile(x, y)->units, vunit)  {
+    unit_list_iterate_safe(map_get_tile(x, y)->units, vunit)  {
       /* Don't transfer units already owned by new city-owner --wegge */ 
       if (unit_owner(vunit) == pvictim) {
        transfer_unit(vunit, pcity, verbose);
@@ -649,7 +649,7 @@
         /* the owner of vunit is allied to pvictim but not to pplayer */
         bounce_unit(vunit, verbose);
       }
-    } unit_list_iterate_end;
+    } unit_list_iterate_safe_end;
   }
 
   /* Any remaining units supported by the city are either given new home
@@ -1095,7 +1095,7 @@
 
   /* This is cutpasted with modifications from transfer_city_units. Yes, it is 
ugly.
      But I couldn't see a nice way to make them use the same code */
-  unit_list_iterate(pcity->units_supported, punit) {
+  unit_list_iterate_safe(pcity->units_supported, punit) {
     struct city *new_home_city = map_get_city(punit->x, punit->y);
     x = punit->x; y = punit->y;
     if (new_home_city
@@ -1113,14 +1113,14 @@
     }
 
     wipe_unit(punit);
-  } unit_list_iterate_end;
+  } unit_list_iterate_safe_end;
 
   x = pcity->x;
   y = pcity->y;
 
   /* make sure ships are not left on land when city is removed. */
  MOVE_SEA_UNITS:
-  unit_list_iterate(ptile->units, punit) {
+  unit_list_iterate_safe(ptile->units, punit) {
     bool moved;
     if (!punit
        || !same_pos(punit->x, punit->y, x, y)
@@ -1155,7 +1155,7 @@
     /* We just messed with the unit list. Avoid trouble by starting over.
        Note that the problem is reduced by one unit, so no loop trouble. */
     goto MOVE_SEA_UNITS;
-  } unit_list_iterate_end;
+  } unit_list_iterate_safe_end;
 
   for (o = 0; o < NUM_TRADEROUTES; o++) {
     struct city *pother_city = find_city_by_id(pcity->trade[o]);
Index: server/cityturn.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/cityturn.c,v
retrieving revision 1.230
diff -u -r1.230 cityturn.c
--- server/cityturn.c   2003/10/13 07:10:15     1.230
+++ server/cityturn.c   2003/11/10 23:54:29
@@ -528,7 +528,7 @@
      * you want to disband a unit that is draining your food
      * reserves.  Hence, I'll assume food upkeep > 0 units. -- jjm
      */
-    unit_list_iterate(pcity->units_supported, punit) {
+    unit_list_iterate_safe(pcity->units_supported, punit) {
       if (unit_type(punit)->food_cost > 0 
           && !unit_flag(punit, F_UNDISBANDABLE)) {
        char *utname = unit_type(punit)->name;
@@ -544,8 +544,7 @@
          pcity->food_stock=0;
        return;
       }
-    }
-    unit_list_iterate_end;
+    } unit_list_iterate_safe_end;
     notify_player_ex(city_owner(pcity), pcity->x, pcity->y, E_CITY_FAMINE,
                     _("Game: Famine causes population loss in %s."),
                     pcity->name);
Index: server/diplomats.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/diplomats.c,v
retrieving revision 1.44
diff -u -r1.44 diplomats.c
--- server/diplomats.c  2003/10/27 14:34:31     1.44
+++ server/diplomats.c  2003/11/10 23:54:29
@@ -1110,6 +1110,8 @@
 static bool diplomat_infiltrate_city (struct player *pplayer, struct player 
*cplayer,
                              struct unit *pdiplomat, struct city *pcity)
 {
+  /* We don't need a _safe iterate since no transported units should be
+   * destroyed. */
   unit_list_iterate ((map_get_tile (pcity->x, pcity->y))->units, punit)
     if (unit_flag(punit, F_DIPLOMAT) || unit_flag(punit, F_SUPERSPY)) {
       /* A F_SUPERSPY unit may not acutally be a spy, but a superboss which 
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.140
diff -u -r1.140 savegame.c
--- server/savegame.c   2003/10/27 14:25:24     1.140
+++ server/savegame.c   2003/11/10 23:54:29
@@ -2263,7 +2263,7 @@
 
   /* Fix ferrying sanity */
   players_iterate(pplayer) {
-    unit_list_iterate(pplayer->units, punit) {
+    unit_list_iterate_safe(pplayer->units, punit) {
       struct unit *ferry = find_unit_by_id(punit->transported_by);
 
       if (is_ocean(map_get_terrain(punit->x, punit->y))
@@ -2272,7 +2272,7 @@
                 pplayer->name, unit_name(punit->type), punit->x, punit->y);
         bounce_unit(punit, TRUE);
       }
-    } unit_list_iterate_end;
+    } unit_list_iterate_safe_end;
   } players_iterate_end;
 
   return;
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.263
diff -u -r1.263 unittools.c
--- server/unittools.c  2003/11/04 10:13:28     1.263
+++ server/unittools.c  2003/11/10 23:54:30
@@ -294,7 +294,7 @@
     potential_gold += get_improvement_type(pimpr)->build_cost;
   } built_impr_iterate_end;
 
-  unit_list_iterate(pcity->units_supported, punit) {
+  unit_list_iterate_safe(pcity->units_supported, punit) {
 
     if (pplayer->economic.gold + potential_gold < punit->upkeep_gold) {
       /* We cannot upkeep this unit any longer and selling off city
@@ -311,7 +311,7 @@
        * upkeep give gold when they are disbanded? */
       pplayer->economic.gold -= punit->upkeep_gold;
     }
-  } unit_list_iterate_end;
+  } unit_list_iterate_safe_end;
 }
 
 /***************************************************************************
@@ -416,7 +416,7 @@
   if (player_owns_active_wonder(pplayer, B_LEONARDO))
     handle_leonardo(pplayer);
 
-  unit_list_iterate(pplayer->units, punit) {
+  unit_list_iterate_safe(pplayer->units, punit) {
 
     /* 2) Modify unit hitpoints. Helicopters can even lose them. */
     unit_restore_hitpoints(pplayer, punit);
@@ -485,13 +485,13 @@
          map_has_special(punit->x, punit->y, S_AIRBASE))
        punit->fuel=unit_type(punit)->fuel;
     }
-  } unit_list_iterate_end;
+  } unit_list_iterate_safe_end;
 
   /* 8) Use carriers and submarines to refuel air units */
   refuel_air_units_from_carriers(pplayer);
   
   /* 9) Check if there are air units without fuel */
-  unit_list_iterate(pplayer->units, punit) {
+  unit_list_iterate_safe(pplayer->units, punit) {
     if (is_air_unit(punit) && punit->fuel <= 0
         && unit_type(punit)->fuel > 0) {
       notify_player_ex(pplayer, punit->x, punit->y, E_UNIT_LOST, 
@@ -502,7 +502,7 @@
              unit_name(punit->type));
       wipe_unit(punit);
     } 
-  } unit_list_iterate_end;
+  } unit_list_iterate_safe_end;
 }
 
 /****************************************************************************
@@ -1362,13 +1362,13 @@
     struct tile *ptile = map_get_tile(x, y);
 
     if (is_non_allied_unit_tile(ptile, pplayer)) {
-      unit_list_iterate(ptile->units, aunit) {
+      unit_list_iterate_safe(ptile->units, aunit) {
         if (unit_owner(aunit) == pplayer
             || unit_owner(aunit) == aplayer
             || is_ocean(ptile->terrain)) {
           bounce_unit(aunit, verbose);
         }
-      } unit_list_iterate_end;
+      } unit_list_iterate_safe_end;
     }    
   } unit_list_iterate_safe_end;
 }