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

On 09/09/07, Marko Lindqvist  wrote:
>
> On 08/09/07, Marko Lindqvist  wrote:
> >
> >  move_unit()
>
>  ...
>
> > Now some
> > callers are ready to segfault when ever moving unit dies due to
> > autoattack or by entering barbarian hut (or some other reason?).

 - Cleanup
 - Also S2_0 and trunk versions


 - ML

diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2007-08-25 15:36:15.000000000 +0300
+++ freeciv/server/citytools.c	2007-09-09 17:03:40.000000000 +0300
@@ -616,8 +616,14 @@
    * Only relevant if we are transferring to another player. */
   if (pplayer != pvictim) {
     unit_list_iterate_safe((ptile)->units, vunit)  {
-      /* Don't transfer units already owned by new city-owner --wegge */ 
+      /* Don't transfer units already owned by new city-owner --wegge */
       if (unit_owner(vunit) == pvictim) {
+        /* vunit may die during transfer_unit().
+         * unit_list_unlink() is still safe using vunit pointer, as
+         * pointer is not used for dereferencing, only as value.
+         * Not sure if it would be safe to unlink first and transfer only
+         * after that. Not sure if it is correct to unlink at all in
+         * some cases, depending which list 'units' points to. */
 	transfer_unit(vunit, pcity, verbose);
 	unit_list_unlink(units, vunit);
         wipe_unit(vunit);
@@ -643,7 +649,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_name(vunit->type),
 	      vunit->tile->x, vunit->tile->y, pcity->name);
diff -Nurd -X.diff_ignore freeciv/server/unithand.c freeciv/server/unithand.c
--- freeciv/server/unithand.c	2007-08-04 04:27:24.000000000 +0300
+++ freeciv/server/unithand.c	2007-09-09 17:07:00.000000000 +0300
@@ -1094,8 +1094,10 @@
       assert(is_enemy_city_tile(pdesttile, pplayer) != NULL);
 
       if (unit_flag(punit, F_NUCLEAR)) {
-        move_unit(punit, pcity->tile, 0);
-        handle_unit_attack_request(punit, punit); /* Boom! */
+        if (move_unit(punit, pcity->tile, 0)) {
+          /* Survived dangers of moving */
+          handle_unit_attack_request(punit, punit); /* Boom! */
+        }
         return TRUE;
       }
 
diff -Nurd -X.diff_ignore freeciv/server/unittools.c freeciv/server/unittools.c
--- freeciv/server/unittools.c	2007-08-06 16:42:30.000000000 +0300
+++ freeciv/server/unittools.c	2007-09-09 17:19:37.000000000 +0300
@@ -846,7 +846,9 @@
     break; /* nothing */
 
   case OLC_LAND_TO_OCEAN:
-    unit_list_iterate(ptile->units, punit2) {
+    unit_list_iterate_safe(ptile->units, punit2) {
+      bool unit_alive = TRUE;
+
       if (is_ground_unit(punit2)) {
 	/* look for nearby land */
 	adjc_iterate(punit->tile, ptile2) {
@@ -862,19 +864,20 @@
 			     punit2->tile, E_UNIT_RELOCATED,
 			     _("Game: Moved your %s due to changing"
 			       " land to sea."), unit_name(punit2->type));
-	    (void) move_unit(punit2, ptile2, 0);
-	    if (punit2->activity == ACTIVITY_SENTRY)
+	    unit_alive = move_unit(punit2, ptile2, 0);
+	    if (unit_alive && punit2->activity == ACTIVITY_SENTRY) {
 	      handle_unit_activity_request(punit2, ACTIVITY_IDLE);
+            }
 	    goto START;
 	  }
 	} adjc_iterate_end;
-	/* look for nearby transport */
-	adjc_iterate(punit->tile, ptile2) {
-	  if (is_ocean(ptile2->terrain)
-	      && ground_unit_transporter_capacity(ptile2,
-						  unit_owner(punit2)) > 0) {
-	    if (get_transporter_capacity(punit2) > 0)
-	      sentry_transported_idle_units(punit2);
+        /* look for nearby transport */
+        adjc_iterate(punit->tile, ptile2) {
+          if (is_ocean(ptile2->terrain)
+              && ground_unit_transporter_capacity(ptile2,
+                                                  unit_owner(punit2)) > 0) {
+            if (get_transporter_capacity(punit2) > 0)
+              sentry_transported_idle_units(punit2);
 	    freelog(LOG_VERBOSE,
 		    "Embarked %s's %s due to changing land to sea at (%d, %d).",
 		    unit_owner(punit2)->name, unit_name(punit2->type),
@@ -883,9 +886,10 @@
 			     punit2->tile, E_UNIT_RELOCATED,
 			     _("Game: Embarked your %s due to changing"
 			       " land to sea."), unit_name(punit2->type));
-	    (void) move_unit(punit2, ptile2, 0);
-	    if (punit2->activity == ACTIVITY_SENTRY)
+	    unit_alive = move_unit(punit2, ptile2, 0);
+	    if (unit_alive && punit2->activity == ACTIVITY_SENTRY) {
 	      handle_unit_activity_request(punit2, ACTIVITY_IDLE);
+            }
 	    goto START;
 	  }
 	} adjc_iterate_end;
@@ -901,10 +905,12 @@
 	wipe_unit_spec_safe(punit2, FALSE);
 	goto START;
       }
-    } unit_list_iterate_end;
+    } unit_list_iterate_safe_end;
     break;
   case OLC_OCEAN_TO_LAND:
-    unit_list_iterate(ptile->units, punit2) {
+    unit_list_iterate_safe(ptile->units, punit2) {
+      bool unit_alive = TRUE;
+
       if (is_sailing_unit(punit2)) {
 	/* look for nearby water */
 	adjc_iterate(punit->tile, ptile2) {
@@ -920,9 +926,10 @@
 			     punit2->tile, E_UNIT_RELOCATED,
 			     _("Game: Moved your %s due to changing"
 			       " sea to land."), unit_name(punit2->type));
-	    (void) move_unit(punit2, ptile2, 0);
-	    if (punit2->activity == ACTIVITY_SENTRY)
+	    unit_alive = move_unit(punit2, ptile2, 0);
+	    if (unit_alive && punit2->activity == ACTIVITY_SENTRY) {
 	      handle_unit_activity_request(punit2, ACTIVITY_IDLE);
+            }
 	    goto START;
 	  }
 	} adjc_iterate_end;
@@ -940,9 +947,10 @@
 			     punit2->tile, E_UNIT_RELOCATED,
 			     _("Game: Docked your %s due to changing"
 			       " sea to land."), unit_name(punit2->type));
-	    (void) move_unit(punit2, ptile2, 0);
-	    if (punit2->activity == ACTIVITY_SENTRY)
+	    unit_alive = move_unit(punit2, ptile2, 0);
+	    if (unit_alive && punit2->activity == ACTIVITY_SENTRY) {
 	      handle_unit_activity_request(punit2, ACTIVITY_IDLE);
+            }
 	    goto START;
 	  }
 	} adjc_iterate_end;
@@ -958,7 +966,7 @@
 	wipe_unit_spec_safe(punit2, FALSE);
 	goto START;
       }
-    } unit_list_iterate_end;
+    } unit_list_iterate_safe_end;
     break;
   }
 }
@@ -2659,6 +2667,8 @@
   the transported_by unit field correctly. take_from_land is only relevant 
   if you have set transport_units. Note that the src and dest need not be 
   adjacent.
+
+  Returns TRUE iff unit still alive.
 **************************************************************************/
 bool move_unit(struct unit *punit, struct tile *pdesttile, int move_cost)
 {
diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2007-09-02 15:37:58.000000000 +0300
+++ freeciv/server/citytools.c	2007-09-09 15:56:12.000000000 +0300
@@ -578,8 +578,14 @@
    * Only relevant if we are transferring to another player. */
   if (pplayer != pvictim) {
     unit_list_iterate_safe((ptile)->units, vunit)  {
-      /* Don't transfer units already owned by new city-owner --wegge */ 
+      /* Don't transfer units already owned by new city-owner --wegge */
       if (unit_owner(vunit) == pvictim) {
+        /* vunit may die during transfer_unit().
+         * unit_list_unlink() is still safe using vunit pointer, as
+         * pointer is not used for dereferencing, only as value.
+         * Not sure if it would be safe to unlink first and transfer only
+         * after that. Not sure if it is correct to unlink at all in
+         * some cases, depending which list 'units' points to. */
 	transfer_unit(vunit, pcity, verbose);
 	unit_list_unlink(units, vunit);
       } else if (!pplayers_allied(pplayer, unit_owner(vunit))) {
@@ -604,7 +610,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),
diff -Nurd -X.diff_ignore freeciv/server/maphand.c freeciv/server/maphand.c
--- freeciv/server/maphand.c	2007-08-31 21:24:51.000000000 +0300
+++ freeciv/server/maphand.c	2007-09-09 16:14:46.000000000 +0300
@@ -1360,6 +1360,8 @@
 static void bounce_units_on_terrain_change(struct tile *ptile)
 {
   unit_list_iterate_safe(ptile->units, punit) {
+    bool unit_alive = TRUE;
+
     if (punit->tile == ptile
 	&& punit->transported_by == -1
 	&& !can_unit_exist_at_tile(punit, ptile)) {
@@ -1376,14 +1378,14 @@
 			   punit->tile, E_UNIT_RELOCATED,
 			   _("Moved your %s due to changing terrain."),
 			   unit_name_translation(punit));
-	  (void) move_unit(punit, ptile2, 0);
-	  if (punit->activity == ACTIVITY_SENTRY) {
-	    handle_unit_activity_request(punit, ACTIVITY_IDLE);
-	  }
+	  unit_alive = move_unit(punit, ptile2, 0);
+          if (unit_alive && punit->activity == ACTIVITY_SENTRY) {
+            handle_unit_activity_request(punit, ACTIVITY_IDLE);
+          }
 	  break;
 	}
       } adjc_iterate_end;
-      if (punit->tile == ptile) {
+      if (unit_alive && punit->tile == ptile) {
 	/* if we get here we could not move punit */
 	freelog(LOG_VERBOSE,
 		"Disbanded %s's %s due to changing land to sea at (%d,%d).",
diff -Nurd -X.diff_ignore freeciv/server/unithand.c freeciv/server/unithand.c
--- freeciv/server/unithand.c	2007-08-01 19:18:41.000000000 +0300
+++ freeciv/server/unithand.c	2007-09-09 16:15:24.000000000 +0300
@@ -273,12 +273,15 @@
   Transfer a unit from one homecity to another. If unit is not presently
   in its new homecity, it will be moved there. This new homecity must
   be valid for this unit.
+
+  Note that unit may die in the process.
 **************************************************************************/
 void real_unit_change_homecity(struct unit *punit, struct city *new_pcity)
 {
   struct city *old_pcity = find_city_by_id(punit->homecity);
   struct player *old_owner = unit_owner(punit);
   struct player *new_owner = city_owner(new_pcity);
+  bool unit_alive = TRUE;
 
   if (old_owner != new_owner) {
     vision_clear_sight(punit->server.vision);
@@ -296,16 +299,21 @@
 
   if (!same_pos(punit->tile, new_pcity->tile)) {
     assert(can_unit_exist_at_tile(punit, new_pcity->tile));
-    move_unit(punit, new_pcity->tile, 0); /* teleport to location */
+    /* Teleport to location */
+    unit_alive = move_unit(punit, new_pcity->tile, 0);
   }
-  
-  unit_list_prepend(new_pcity->units_supported, punit);
+
   if (old_pcity) {
+    /* Even if unit is dead, we have to unlink unit pointer (punit). */
     unit_list_unlink(old_pcity->units_supported, punit);
   }
 
-  punit->homecity = new_pcity->id;
-  send_unit_info(unit_owner(punit), punit);
+  if (unit_alive) {
+    unit_list_prepend(new_pcity->units_supported, punit);
+
+    punit->homecity = new_pcity->id;
+    send_unit_info(unit_owner(punit), punit);
+  }
 
   city_refresh(new_pcity);
   send_city_info(new_owner, new_pcity);
@@ -315,7 +323,8 @@
     city_refresh(old_pcity);
     send_city_info(old_owner, old_pcity);
   }
-  assert(unit_owner(punit) == city_owner(new_pcity));
+
+  assert(!unit_alive || unit_owner(punit) == city_owner(new_pcity));
 }
 
 /**************************************************************************
@@ -1121,8 +1130,10 @@
       assert(is_enemy_city_tile(pdesttile, pplayer) != NULL);
 
       if (unit_has_type_flag(punit, F_NUCLEAR)) {
-        move_unit(punit, pcity->tile, 0);
-        handle_unit_attack_request(punit, punit); /* Boom! */
+        if (move_unit(punit, pcity->tile, 0)) {
+          /* Survived dangers of moving */
+          handle_unit_attack_request(punit, punit); /* Boom! */
+        }
         return TRUE;
       }
 
diff -Nurd -X.diff_ignore freeciv/server/unittools.c freeciv/server/unittools.c
--- freeciv/server/unittools.c	2007-09-02 15:37:58.000000000 +0300
+++ freeciv/server/unittools.c	2007-09-09 16:17:58.000000000 +0300
@@ -2715,6 +2715,8 @@
   the transported_by unit field correctly. take_from_land is only relevant 
   if you have set transport_units. Note that the src and dest need not be 
   adjacent.
+
+  Returns TRUE iff unit still alive.
 **************************************************************************/
 bool move_unit(struct unit *punit, struct tile *pdesttile, int move_cost)
 {
diff -Nurd -X.diff_ignore freeciv/common/unit.c freeciv/common/unit.c
--- freeciv/common/unit.c	2007-08-13 20:51:03.000000000 +0300
+++ freeciv/common/unit.c	2007-09-09 16:22:12.000000000 +0300
@@ -1528,3 +1528,18 @@
     < (punittype->hp *
        utype_class(punittype)->hp_loss_pct / 100);
 }
+
+/**************************************************************************
+  Check if unit with given id is still alive. Use this before using
+  old unit pointers when unit might have dead.
+**************************************************************************/
+bool unit_alive(int id)
+{
+  /* Check if unit exist for any player
+   * (this is just one fast idex_lookup_unit() ) */
+  if (player_find_unit_by_id(NULL, id)) {
+    return TRUE;
+  }
+
+  return FALSE;
+}
diff -Nurd -X.diff_ignore freeciv/common/unit.h freeciv/common/unit.h
--- freeciv/common/unit.h	2007-08-08 05:51:50.000000000 +0300
+++ freeciv/common/unit.h	2007-09-09 16:22:39.000000000 +0300
@@ -303,4 +303,6 @@
 bool unit_type_is_losing_hp(const struct player *pplayer,
                             const struct unit_type *punittype);
 
+bool unit_alive(int id);
+
 #endif  /* FC__UNIT_H */
diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2007-09-02 15:38:29.000000000 +0300
+++ freeciv/server/citytools.c	2007-09-09 16:09:39.000000000 +0300
@@ -581,8 +581,14 @@
    * Only relevant if we are transferring to another player. */
   if (pplayer != pvictim) {
     unit_list_iterate_safe((ptile)->units, vunit)  {
-      /* Don't transfer units already owned by new city-owner --wegge */ 
+      /* Don't transfer units already owned by new city-owner --wegge */
       if (unit_owner(vunit) == pvictim) {
+        /* vunit may die during transfer_unit().
+         * unit_list_unlink() is still safe using vunit pointer, as
+         * pointer is not used for dereferencing, only as value.
+         * Not sure if it would be safe to unlink first and transfer only
+         * after that. Not sure if it is correct to unlink at all in
+         * some cases, depending which list 'units' points to. */
 	transfer_unit(vunit, pcity, verbose);
 	unit_list_unlink(units, vunit);
       } else if (!pplayers_allied(pplayer, unit_owner(vunit))) {
diff -Nurd -X.diff_ignore freeciv/server/maphand.c freeciv/server/maphand.c
--- freeciv/server/maphand.c	2007-08-31 21:25:10.000000000 +0300
+++ freeciv/server/maphand.c	2007-09-09 16:11:01.000000000 +0300
@@ -1510,6 +1510,8 @@
 static void bounce_units_on_terrain_change(struct tile *ptile)
 {
   unit_list_iterate_safe(ptile->units, punit) {
+    bool unit_alive = TRUE;
+
     if (punit->tile == ptile
 	&& punit->transported_by == -1
 	&& !can_unit_exist_at_tile(punit, ptile)) {
@@ -1526,14 +1528,14 @@
 			   punit->tile, E_UNIT_RELOCATED,
 			   _("Moved your %s due to changing terrain."),
 			   unit_name_translation(punit));
-	  move_unit(punit, ptile2, 0);
-	  if (punit->activity == ACTIVITY_SENTRY) {
+	  unit_alive = move_unit(punit, ptile2, 0);
+	  if (unit_alive && punit->activity == ACTIVITY_SENTRY) {
 	    handle_unit_activity_request(punit, ACTIVITY_IDLE);
 	  }
 	  break;
 	}
       } adjc_iterate_end;
-      if (punit->tile == ptile) {
+      if (unit_alive && punit->tile == ptile) {
 	/* if we get here we could not move punit */
 	freelog(LOG_VERBOSE,
 		"Disbanded %s's %s due to changing land to sea at (%d,%d).",
diff -Nurd -X.diff_ignore freeciv/server/unithand.c freeciv/server/unithand.c
--- freeciv/server/unithand.c	2007-08-14 02:36:43.000000000 +0300
+++ freeciv/server/unithand.c	2007-09-09 16:14:13.000000000 +0300
@@ -275,12 +275,15 @@
   Transfer a unit from one homecity to another. If unit is not presently
   in its new homecity, it will be moved there. This new homecity must
   be valid for this unit.
+
+  Note that unit may die in the process.
 **************************************************************************/
 void real_unit_change_homecity(struct unit *punit, struct city *new_pcity)
 {
   struct city *old_pcity = game_find_city_by_number(punit->homecity);
   struct player *old_owner = unit_owner(punit);
   struct player *new_owner = city_owner(new_pcity);
+  bool unit_alive = TRUE;
 
   /* Calling this function when new_pcity is same as old_pcity should
    * be safe with current implementation, but it is not meant to
@@ -303,7 +306,8 @@
 
   if (!same_pos(punit->tile, new_pcity->tile)) {
     assert(can_unit_exist_at_tile(punit, new_pcity->tile));
-    move_unit(punit, new_pcity->tile, 0); /* teleport to location */
+    /* Teleport to location */
+    unit_alive = move_unit(punit, new_pcity->tile, 0);
   }
 
   /* Remove from old city first and add to new city only after that.
@@ -311,12 +315,16 @@
    * prohibited by assert in the beginning of the function).
    */
   if (old_pcity) {
+    /* Even if unit is dead, we have to unlink unit pointer (punit). */
     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);
+  if (unit_alive) {
+    unit_list_prepend(new_pcity->units_supported, punit);
+
+    punit->homecity = new_pcity->id;
+    send_unit_info(unit_owner(punit), punit);
+  }
 
   city_refresh(new_pcity);
   send_city_info(new_owner, new_pcity);
@@ -326,7 +334,8 @@
     city_refresh(old_pcity);
     send_city_info(old_owner, old_pcity);
   }
-  assert(unit_owner(punit) == city_owner(new_pcity));
+
+  assert(!unit_alive || unit_owner(punit) == city_owner(new_pcity));
 }
 
 /**************************************************************************
@@ -1151,8 +1160,10 @@
       assert(is_enemy_city_tile(pdesttile, pplayer) != NULL);
 
       if (unit_has_type_flag(punit, F_NUCLEAR)) {
-        move_unit(punit, pcity->tile, 0);
-        handle_unit_attack_request(punit, punit); /* Boom! */
+        if (move_unit(punit, pcity->tile, 0)) {
+          /* Survived dangers of moving */
+          handle_unit_attack_request(punit, punit); /* Boom! */
+        }
         return TRUE;
       }
 
diff -Nurd -X.diff_ignore freeciv/server/unittools.c freeciv/server/unittools.c
--- freeciv/server/unittools.c	2007-09-02 15:38:29.000000000 +0300
+++ freeciv/server/unittools.c	2007-09-09 16:20:11.000000000 +0300
@@ -2646,8 +2646,10 @@
   the transported_by unit field correctly. take_from_land is only relevant 
   if you have set transport_units. Note that the src and dest need not be 
   adjacent.
+
+  Returns TRUE iff unit still alive.
 **************************************************************************/
-void move_unit(struct unit *punit, struct tile *pdesttile, int move_cost)
+bool move_unit(struct unit *punit, struct tile *pdesttile, int move_cost)
 {
   struct player *pplayer = unit_owner(punit);
   struct tile *psrctile = punit->tile;
@@ -2829,7 +2831,7 @@
   wakeup_neighbor_sentries(punit);
   if (!unit_survive_autoattack(punit)) {
     conn_list_do_unbuffer(pplayer->connections);
-    return;
+    return FALSE;
   }
   maybe_make_contact(pdesttile, unit_owner(punit));
 
@@ -2861,8 +2863,14 @@
    * right order.  This is probably not a bug. */
 
   if (tile_has_special(pdesttile, S_HUT)) {
+    int saved_id = punit->id;
+
     unit_enter_hut(punit);
+
+    return unit_alive(saved_id);
   }
+
+  return TRUE;
 }
 
 /**************************************************************************
diff -Nurd -X.diff_ignore freeciv/server/unittools.h freeciv/server/unittools.h
--- freeciv/server/unittools.h	2007-08-13 20:51:02.000000000 +0300
+++ freeciv/server/unittools.h	2007-09-09 16:16:36.000000000 +0300
@@ -85,7 +85,7 @@
 bool do_paradrop(struct unit *punit, struct tile *ptile);
 void load_unit_onto_transporter(struct unit *punit, struct unit *ptrans);
 void unload_unit_from_transporter(struct unit *punit);
-void move_unit(struct unit *punit, struct tile *ptile, int move_cost);
+bool move_unit(struct unit *punit, struct tile *ptile, int move_cost);
 bool execute_orders(struct unit *punit);
 
 #endif  /* FC__UNITTOOLS_H */
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to