[Freeciv-Dev] Re: (PR#10389) logging of dead explorer

["Per I. Mathisen" <per@xxxxxxxxxxx>]

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

On Wed, 29 Sep 2004, Jason Short wrote:
> but the unit is killed inside ai_manage_explorer so the second UNIT_LOG
> results in some invalid memory accesses.

But how could the unit die in the explorer code? This should not be
possible. So we probably have two bugs here.

Here is a patch for the first bug. It makes ai_manage_explorer() pass a
pointer to a unit pointer, as suggested by Jason on irc.

  - Per

Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.337
diff -u -r1.337 aiunit.c
--- ai/aiunit.c 29 Sep 2004 02:24:18 -0000      1.337
+++ ai/aiunit.c 30 Sep 2004 12:06:49 -0000
@@ -1753,7 +1753,6 @@
 static void ai_military_attack(struct player *pplayer, struct unit *punit)
 {
   struct tile *dest_tile;
-  int id = punit->id;
   int ct = 10;
   struct city *pcity = NULL;
 
@@ -1832,9 +1831,9 @@
     (void) ai_unit_goto(punit, pcity->tile);
   } else if (!is_barbarian(pplayer)) {
     /* Nothing else to do, so try exploring. */
-    if (ai_manage_explorer(punit)) {
+    if (ai_manage_explorer(&punit)) {
       UNIT_LOG(LOG_DEBUG, punit, "nothing else to do, so exploring");
-    } else {
+    } else if (punit) {
       UNIT_LOG(LOG_DEBUG, punit, "nothing to do - no more exploring either");
     }
   } else {
@@ -1857,7 +1856,7 @@
       }
     }
   }
-  if ((punit = find_unit_by_id(id)) && punit->moves_left > 0) {
+  if (punit && punit->moves_left > 0) {
     struct city *pcity = map_get_city(punit->tile);
 
     if (pcity) {
@@ -2054,7 +2053,7 @@
     handle_unit_activity_request(punit, ACTIVITY_PILLAGE);
     return; /* when you pillage, you have moves left, avoid later fortify */
   case AIUNIT_EXPLORE:
-    (void) ai_manage_explorer(punit);
+    (void) ai_manage_explorer(&punit);
     break;
   case AIUNIT_RECOVER:
     ai_manage_hitpoint_recovery(punit);
@@ -2182,10 +2181,8 @@
     ai_manage_military(pplayer,punit); 
     return;
   } else {
-    int id = punit->id;
     /* what else could this be? -- Syela */
-    if (!ai_manage_explorer(punit)
-        && find_unit_by_id(id)) {
+    if (!ai_manage_explorer(&punit) && punit) {
       ai_unit_new_role(punit, AIUNIT_DEFEND_HOME, NULL);
       ai_military_gohome(pplayer, punit);
     }
Index: ai/aiexplorer.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiexplorer.c,v
retrieving revision 1.5
diff -u -r1.5 aiexplorer.c
--- ai/aiexplorer.c     29 Sep 2004 02:24:18 -0000      1.5
+++ ai/aiexplorer.c     30 Sep 2004 12:06:50 -0000
@@ -242,8 +242,9 @@
 
   Returns whether there is any more territory to be explored.
 **************************************************************************/
-bool ai_manage_explorer(struct unit *punit)
+bool ai_manage_explorer(struct unit **explorer)
 {
+  struct unit *punit = *explorer;
   struct player *pplayer = unit_owner(punit);
   /* Loop prevention */
   struct tile *ptile = punit->tile;
@@ -313,7 +314,8 @@
     /* TODO: read the path off the map we made.  Then we can make a path 
      * which goes beside the unknown, with a good EC callback... */
     if (!ai_unit_goto(punit, best_tile)) {
-      /* Died?  Strange... */
+      /* We died.  That is strange... */
+      *explorer = NULL;
       return FALSE;
     }
     if (punit->moves_left > 0) {
@@ -321,7 +323,7 @@
       if (!same_pos(punit->tile, ptile)) {
        /* At least we moved (and maybe even got to where we wnated).  
          * Let's try again. */
-       return ai_manage_explorer(punit);          
+       return ai_manage_explorer(explorer);
       } else {
        /* Something went wrong. What to do but return?
         * Answer: if we're a trireme we could get to this point,
Index: ai/aiexplorer.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiexplorer.h,v
retrieving revision 1.2
diff -u -r1.2 aiexplorer.h
--- ai/aiexplorer.h     3 Sep 2004 04:22:36 -0000       1.2
+++ ai/aiexplorer.h     30 Sep 2004 12:06:50 -0000
@@ -23,7 +23,7 @@
  *
  * Returns whether there is any more territory to be explored.
  */
-bool ai_manage_explorer(struct unit *punit);
+bool ai_manage_explorer(struct unit **explorer);
 
 /*
  * TODO: Enable AI build new explorers.  Either (the old way) write a 
Index: ai/aiferry.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiferry.c,v
retrieving revision 1.6
diff -u -r1.6 aiferry.c
--- ai/aiferry.c        29 Sep 2004 02:24:18 -0000      1.6
+++ ai/aiferry.c        30 Sep 2004 12:06:50 -0000
@@ -842,9 +842,9 @@
   }
 
   UNIT_LOG(LOGLEVEL_FERRY, punit, "Passing control of ferry to explorer code");
-  (void) ai_manage_explorer(punit);
+  (void) ai_manage_explorer(&punit);
 
-  if (punit->moves_left > 0) {
+  if (punit && punit->moves_left > 0) {
     struct city *pcity = find_nearest_safe_city(punit);
     if (pcity) {
       punit->goto_tile = pcity->tile;
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.307
diff -u -r1.307 unithand.c
--- server/unithand.c   29 Sep 2004 02:24:24 -0000      1.307
+++ server/unithand.c   30 Sep 2004 12:06:50 -0000
@@ -514,10 +514,9 @@
      * Handling it deeper in the code leads to some tricky recursive loops -
      * see PR#2631. */
     if (punit->moves_left > 0 && activity == ACTIVITY_EXPLORE) {
-      int id = punit->id;
-      bool more_to_explore = ai_manage_explorer(punit);
+      bool more_to_explore = ai_manage_explorer(&punit);
 
-      if ((punit = find_unit_by_id(id))) {
+      if (punit) {
        assert(punit->activity == ACTIVITY_EXPLORE);
        if (!more_to_explore) {
          set_unit_activity(punit, ACTIVITY_IDLE);
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.306
diff -u -r1.306 unittools.c
--- server/unittools.c  29 Sep 2004 02:24:24 -0000      1.306
+++ server/unittools.c  30 Sep 2004 12:06:50 -0000
@@ -569,7 +569,6 @@
 static void update_unit_activity(struct unit *punit)
 {
   struct player *pplayer = unit_owner(punit);
-  int id = punit->id;
   bool unit_activity_done = FALSE;
   enum unit_activity activity = punit->activity;
   enum ocean_land_change solvency = OLC_NONE;
@@ -592,9 +591,9 @@
   unit_restore_movepoints(pplayer, punit);
 
   if (activity == ACTIVITY_EXPLORE) {
-    bool more_to_explore = ai_manage_explorer(punit);
+    bool more_to_explore = ai_manage_explorer(&punit);
 
-    if (!player_find_unit_by_id(pplayer, id)) {
+    if (!punit) {
       /* Died */
       return;
     }