[Freeciv-Dev] Re: (PR#6735) Server crashed upgrading a frigate

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

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

On Thu, Nov 06, 2003 at 10:23:39AM -0800, Jason Short wrote:
> Raimar Falke wrote:
> > On Wed, Nov 05, 2003 at 05:27:59AM -0800, Per I. Mathisen wrote:
> > 
> >>On Wed, 5 Nov 2003, Raimar Falke wrote:
> >>
> >>>>I think the simplest solution to this problem is simply to ban upgrades of
> >>>>transporters with passengers where the result is a unit with less
> >>>>transport capacity than the number of passengers transported.
> >>>
> >>>I agree. What changes to the ruleset are required?
> >>
> >>No ruleset changes. We just check, when you want to upgrade, if transport
> >>is carrying more passengers than we could carry if we upgraded - if that
> >>is the case, no upgrade for you (for now).
> > 
> > 
> > So this patch is a 4-liner?!
> 
> Changes should be done in the client to disable the upgrade (disable the 
> menu, and probably some changes to the units dialog) and to the server 
> (with an error message).
> 
> Attached patch (untested) mmakes the server change.  However I'd suggest 
> a can_upgrade_unit() be added to common/unit.[ch] for use by the clients 
> and one of the server functions (but not the other, which gives a 
> specific error message).

I don't like the idea to add conditions to various code parts. The
solution is one unified function. The attached patch implementes this.

To reproduce load the game and upgrade the ship in Falmouth and press
turn done.

The patch doesn't work like expected in the client case (it should see
the problem (ship carries units) at the client and not send the
request to the server) but transported_by is not valid in the client.

If you approve the design I will convert the other clients and add
docu.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "If at first you don't succeed... well so much for skydiving."

6735.gz
Description: application/gunzip

Index: client/climisc.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/climisc.c,v
retrieving revision 1.121
diff -u -u -r1.121 climisc.c
--- client/climisc.c    2003/10/21 21:49:55     1.121
+++ client/climisc.c    2003/11/10 17:23:20
@@ -1170,3 +1170,8 @@
     append_output_window(buf);
   }
 }
+
+void upgrade_unit(struct unit *punit, Unit_Type_id to_unit)
+{
+  die("The client don't upgrade units.");
+}
Index: client/gui-gtk/citydlg.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/citydlg.c,v
retrieving revision 1.173
diff -u -u -r1.173 citydlg.c
--- client/gui-gtk/citydlg.c    2003/11/01 00:53:05     1.173
+++ client/gui-gtk/citydlg.c    2003/11/10 17:23:23
@@ -2688,14 +2688,13 @@
 *****************************************************************/
 static void unit_upgrade_callback(gpointer data)
 {
-  struct unit *punit;
+  struct unit *punit =
+      player_find_unit_by_id(game.player_ptr, (size_t) data);
   char buf[512];
-  int ut1, ut2;
-  int value;
 
-  if ((punit = player_find_unit_by_id(game.player_ptr, (size_t) data))) {
-    ut1 = punit->type;
-    ut2 = can_upgrade_unittype(game.player_ptr, ut1);
+  if (punit) {
+    Unit_Type_id ut1 = punit->type;
+    Unit_Type_id ut2 = can_upgrade_unittype(game.player_ptr, ut1);
 
     if (ut2 == -1) {
       /* this shouldn't generally happen, but it is conceivable */
@@ -2703,12 +2702,23 @@
                  _("Sorry: cannot upgrade %s."), unit_types[ut1].name);
       popup_message_dialog(top_vbox,
                           _("Upgrade Unit!"), buf,
-                          dummy_close_callback, NULL, 
+                          dummy_close_callback, NULL,
                           _("Darn"), NULL, 0, NULL);
     } else {
-      value = unit_upgrade_price(game.player_ptr, ut1, ut2);
+      enum unit_upgrade_result result =
+         unit_upgrade(punit, ut2, TRUE, TRUE, TRUE);
+      int value = unit_upgrade_price(game.player_ptr, ut1, ut2);
 
-      if (game.player_ptr->economic.gold >= value) {
+      if (result == UR_NO_MONEY) {
+       my_snprintf(buf, sizeof(buf),
+                   _("Upgrading %s to %s costs %d gold.\n"
+                     "Treasury contains %d gold."), unit_types[ut1].name,
+                   unit_types[ut2].name, value,
+                   game.player_ptr->economic.gold);
+       popup_message_dialog(top_vbox, _("Upgrade Unit!"), buf,
+                            dummy_close_callback, NULL,
+                            _("Darn"), NULL, 0, NULL);
+      } else if (result == UR_OK) {
        my_snprintf(buf, sizeof(buf), _("Upgrade %s to %s for %d gold?\n"
                                        "Treasury contains %d gold."),
                    unit_types[ut1].name, unit_types[ut2].name,
@@ -2721,12 +2731,10 @@
                             NULL, 0, NULL);
       } else {
        my_snprintf(buf, sizeof(buf),
-                   _("Upgrading %s to %s costs %d gold.\n"
-                     "Treasury contains %d gold."), unit_types[ut1].name,
-                   unit_types[ut2].name, value,
-                   game.player_ptr->economic.gold);
-       popup_message_dialog(top_vbox,_("Upgrade Unit!"),buf,
-                            dummy_close_callback, NULL, 
+                   _("Sorry: cannot upgrade %s."), unit_types[ut1].name);
+       popup_message_dialog(top_vbox,
+                            _("Upgrade Unit!"), buf,
+                            dummy_close_callback, NULL,
                             _("Darn"), NULL, 0, NULL);
       }
     }
Index: common/unit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.c,v
retrieving revision 1.187
diff -u -u -r1.187 unit.c
--- common/unit.c       2003/10/29 08:00:39     1.187
+++ common/unit.c       2003/11/10 17:23:24
@@ -1512,3 +1512,111 @@
     punit->pgr = NULL;
   }
 }
+
+/****************************************************************************
+  Expensive function to check how many units are in the transport.
+****************************************************************************/
+int get_transporter_occupancy(struct unit *ptrans)
+{
+  int occupied = 0;
+
+  unit_list_iterate(map_get_tile(ptrans->x, ptrans->y)->units, pcargo) {
+    if (pcargo->transported_by == ptrans->id) {
+      occupied++;
+    }
+  } unit_list_iterate_end;
+
+  return occupied;
+}
+
+/***************************************************************************
+ Return 1 if upgrading this unit could cause passengers to
+ get stranded at sea, due to transport capacity changes
+ caused by the upgrade.
+ Return 0 if ok to upgrade (as far as stranding goes).
+***************************************************************************/
+static bool upgrade_would_strand(struct unit *punit, Unit_Type_id upgrade_type)
+{
+  int old_cap, new_cap, tile_cap, tile_ncargo;
+  
+  if (!is_sailing_unit(punit))
+    return FALSE;
+  if (!is_ocean(map_get_terrain(punit->x, punit->y))) {
+    return FALSE;
+  }
+
+  /* With weird non-standard unit types, upgrading these could
+     cause air units to run out of fuel; too bad. */
+  if (unit_flag(punit, F_CARRIER)
+      || unit_flag(punit, F_MISSILE_CARRIER))
+    return FALSE;
+
+  old_cap = get_transporter_capacity(punit);
+  new_cap = unit_types[upgrade_type].transport_capacity;
+
+  if (new_cap >= old_cap)
+    return FALSE;
+
+  tile_cap = 0;
+  tile_ncargo = 0;
+  unit_list_iterate(map_get_tile(punit->x, punit->y)->units, punit2) {
+    if (punit2->owner == punit->owner
+        || pplayers_allied(unit_owner(punit2), unit_owner(punit))) {
+      if (is_sailing_unit(punit2) && is_ground_units_transport(punit2)) { 
+       tile_cap += get_transporter_capacity(punit2);
+      } else if (is_ground_unit(punit2)) {
+       tile_ncargo++;
+      }
+    }
+  }
+  unit_list_iterate_end;
+
+  if (tile_ncargo <= tile_cap - old_cap + new_cap)
+    return FALSE;
+
+  freelog(LOG_VERBOSE, "Can't upgrade %s at (%d,%d)"
+         " because would strand passenger(s)",
+         unit_type(punit)->name, punit->x, punit->y);
+  return TRUE;
+}
+
+/***************************************************************************
+ Return 1 if upgrading this unit could cause passengers to
+ get stranded at sea, due to transport capacity changes
+ caused by the upgrade.
+ Return 0 if ok to upgrade (as far as stranding goes).
+***************************************************************************/
+enum unit_upgrade_result unit_upgrade(struct unit *punit,
+                                     Unit_Type_id to_unittype,
+                                     bool test_only, bool need_money,
+                                     bool need_city)
+{
+  struct player *pplayer = unit_owner(punit);
+  struct city *pcity = map_get_city(punit->x, punit->y);
+  const int cost = unit_upgrade_price(pplayer, punit->type, to_unittype);
+
+  if (need_money && pplayer->economic.gold < cost) {
+    return UR_NO_MONEY;
+  }
+
+  if (need_city && !pcity) {
+    return UR_NOT_IN_CITY;
+  }
+  if (need_city && pcity && city_owner(pcity) != pplayer) {
+    return UR_NOT_CITY_OWNER;
+  }
+  if (get_transporter_occupancy(punit) >
+      unit_types[to_unittype].transport_capacity
+      || upgrade_would_strand(punit, to_unittype)) {
+    return UR_NOT_ENOUGH_ROOM;
+  }
+
+  if (test_only) {
+    return UR_OK;
+  }
+  upgrade_unit(punit, to_unittype);
+  if (need_money) {
+    pplayer->economic.gold -= cost;
+  }
+  return UR_OK;
+}
Index: common/unit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.h,v
retrieving revision 1.103
diff -u -u -r1.103 unit.h
--- common/unit.h       2003/10/29 08:00:39     1.103
+++ common/unit.h       2003/11/10 17:23:25
@@ -95,6 +95,14 @@
                                   sewer but city has no sewer */
 };
 
+enum unit_upgrade_result {
+  UR_OK,
+  UR_NO_MONEY,
+  UR_NOT_IN_CITY,
+  UR_NOT_CITY_OWNER,
+  UR_NOT_ENOUGH_ROOM
+};
+    
 struct unit_ai {
   bool control; /* 0: not automated    1: automated */
   enum ai_unit_task ai_role;
@@ -296,5 +304,15 @@
                                  Unit_Type_id type, bool make_veteran);
 void destroy_unit_virtual(struct unit *punit);
 void free_unit_goto_route(struct unit *punit);
+
+int get_transporter_occupancy(struct unit *ptrans);
+
+/* Implemented in server/unittools.c and client/climisc.c */
+void upgrade_unit(struct unit *punit, Unit_Type_id to_unit);
+
+enum unit_upgrade_result unit_upgrade(struct unit *punit,
+                                     Unit_Type_id to_unittype,
+                                     bool test_only, bool need_money,
+                                     bool need_city);
 
 #endif  /* FC__UNIT_H */
Index: server/unithand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unithand.c,v
retrieving revision 1.277
diff -u -u -r1.277 unithand.c
--- server/unithand.c   2003/10/29 08:00:39     1.277
+++ server/unithand.c   2003/11/10 17:23:27
@@ -153,55 +153,49 @@
 /**************************************************************************
  Upgrade all units of a given type.
 **************************************************************************/
-void handle_upgrade_unittype_request(struct player * const pplayer,
-                       const struct packet_unittype_info * const packet)
+void handle_upgrade_unittype_request(struct player *const pplayer,
+                                    const struct packet_unittype_info *const
+                                    packet)
 {
   const Unit_Type_id from_unittype = packet->type;
   const Unit_Type_id to_unittype = can_upgrade_unittype(pplayer,
                                                        from_unittype);
+  int number_of_upgraded_units = 0;
 
   if (to_unittype == -1) {
     notify_player(pplayer,
                  _("Game: Illegal packet, can't upgrade %s (yet)."),
-                  unit_types[from_unittype].name);
-  } else {
-    const int cost = unit_upgrade_price(pplayer, from_unittype, to_unittype);
-    int number_of_upgraded_units = 0;
-
-    if (pplayer->economic.gold >= cost) {
-      const int player_no = pplayer->player_no;
+                 unit_types[from_unittype].name);
+    return;
+  }
 
-      /* Try to upgrade units. The order we upgrade in is arbitrary (if
-       * the player really cared they should have done it manually). */
-      conn_list_do_buffer(&pplayer->connections);
-      unit_list_iterate(pplayer->units, punit) {
-        if (punit->type == from_unittype) {
-          const struct city * const pcity = map_get_city(punit->x, punit->y);
-
-         /* Only units in cities can be upgraded. */
-          if (pcity && pcity->owner == player_no) {
-            upgrade_unit(punit, to_unittype);
-           number_of_upgraded_units++;
-            if ((pplayer->economic.gold -= cost) < cost) {
-             /* We can't upgrade any more units. */
-              break;
-            }
-          }
-        }
-      } unit_list_iterate_end;
-      conn_list_do_unbuffer(&pplayer->connections);
+  /* Try to upgrade units. The order we upgrade in is arbitrary (if
+   * the player really cared they should have done it manually). */
+  conn_list_do_buffer(&pplayer->connections);
+  unit_list_iterate(pplayer->units, punit) {
+    if (punit->type == from_unittype) {
+      enum unit_upgrade_result result =
+         unit_upgrade(punit, to_unittype, FALSE, TRUE, TRUE);
+      if (result == UR_OK) {
+       number_of_upgraded_units++;
+      }
+      if (result == UR_NO_MONEY) {
+       break;
+      }
     }
+  } unit_list_iterate_end;
+  conn_list_do_unbuffer(&pplayer->connections);
 
-    /* Alert the player about what happened. */
-    if (number_of_upgraded_units > 0) {
-      notify_player(pplayer, _("Game: %d %s upgraded to %s for %d gold."),
-                    number_of_upgraded_units, unit_types[from_unittype].name,
-                    unit_types[to_unittype].name,
-                    cost * number_of_upgraded_units);
-      send_player_info(pplayer, pplayer);
-    } else {
-      notify_player(pplayer, _("Game: No units could be upgraded."));
-    }
+  /* Alert the player about what happened. */
+  if (number_of_upgraded_units > 0) {
+    const int cost = unit_upgrade_price(pplayer, from_unittype, to_unittype);
+    notify_player(pplayer, _("Game: %d %s upgraded to %s for %d gold."),
+                 number_of_upgraded_units, unit_types[from_unittype].name,
+                 unit_types[to_unittype].name,
+                 cost * number_of_upgraded_units);
+    send_player_info(pplayer, pplayer);
+  } else {
+    notify_player(pplayer, _("Game: No units could be upgraded."));
   }
 }
 
@@ -210,38 +204,47 @@
  TODO: should upgrades in allied cities be possible?
 **************************************************************************/
 void handle_unit_upgrade_request(struct player *pplayer,
-                                 struct packet_unit_request *packet)
+                                struct packet_unit_request *packet)
 {
-  int cost;
   int from_unit, to_unit;
   struct unit *punit = player_find_unit_by_id(pplayer, packet->unit_id);
   struct city *pcity = player_find_city_by_id(pplayer, packet->city_id);
-  
+  enum unit_upgrade_result result;
+
   if (!punit || !pcity) {
     return;
   }
 
-  if(!same_pos(punit->x, punit->y, pcity->x, pcity->y))  {
+  if (!same_pos(punit->x, punit->y, pcity->x, pcity->y)) {
     notify_player(pplayer, _("Game: Illegal move, unit not in city!"));
     return;
   }
   from_unit = punit->type;
-  if((to_unit=can_upgrade_unittype(pplayer, punit->type)) == -1) {
-    notify_player(pplayer, _("Game: Illegal package, can't upgrade %s 
(yet)."), 
+  to_unit = can_upgrade_unittype(pplayer, punit->type);
+  if (to_unit == -1) {
+    notify_player(pplayer,
+                 _("Game: Illegal package, can't upgrade %s (yet)."),
                  unit_type(punit)->name);
     return;
   }
-  cost = unit_upgrade_price(pplayer, punit->type, to_unit);
-  if(cost > pplayer->economic.gold) {
+
+  result = unit_upgrade(punit, to_unit, FALSE, TRUE, TRUE);
+  if (result == UR_NO_MONEY) {
+    int cost = unit_upgrade_price(pplayer, punit->type, to_unit);
     notify_player(pplayer, _("Game: Insufficient funds, upgrade costs %d."),
                  cost);
-    return;
+  } else if (result == UR_OK) {
+    int cost = unit_upgrade_price(pplayer, punit->type, to_unit);
+    send_player_info(pplayer, pplayer);
+    notify_player(pplayer, _("Game: %s upgraded to %s for %d gold."),
+                 unit_name(from_unit), unit_name(to_unit), cost);
+  } else if (result == UR_NOT_ENOUGH_ROOM) {
+    notify_player(pplayer, _("Game: Cannot upgrade %s because it is "
+                            "transporting other units."),
+                 unit_type(punit)->name);
+  } else {
+    notify_player(pplayer, _("Game: Upgrading failed."));
   }
-  pplayer->economic.gold -= cost;
-  upgrade_unit(punit, to_unit);
-  send_player_info(pplayer, pplayer);
-  notify_player(pplayer, _("Game: %s upgraded to %s for %d gold."), 
-               unit_name(from_unit), unit_name(to_unit), cost);
 }
 
 
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.263
diff -u -u -r1.263 unittools.c
--- server/unittools.c  2003/11/04 10:13:28     1.263
+++ server/unittools.c  2003/11/10 17:23:29
@@ -59,7 +59,6 @@
 static void unit_restore_movepoints(struct player *pplayer, struct unit 
*punit);
 static void update_unit_activity(struct unit *punit);
 static void wakeup_neighbor_sentries(struct unit *punit);
-static bool upgrade_would_strand(struct unit *punit, int upgrade_type);
 static void handle_leonardo(struct player *pplayer);
 
 static void sentry_transported_idle_units(struct unit *ptrans);
@@ -158,63 +157,11 @@
 }
 
 /***************************************************************************
- Return 1 if upgrading this unit could cause passengers to
- get stranded at sea, due to transport capacity changes
- caused by the upgrade.
- Return 0 if ok to upgrade (as far as stranding goes).
-***************************************************************************/
-static bool upgrade_would_strand(struct unit *punit, int upgrade_type)
-{
-  int old_cap, new_cap, tile_cap, tile_ncargo;
-  
-  if (!is_sailing_unit(punit))
-    return FALSE;
-  if (!is_ocean(map_get_terrain(punit->x, punit->y))) {
-    return FALSE;
-  }
-
-  /* With weird non-standard unit types, upgrading these could
-     cause air units to run out of fuel; too bad. */
-  if (unit_flag(punit, F_CARRIER)
-      || unit_flag(punit, F_MISSILE_CARRIER))
-    return FALSE;
-
-  old_cap = get_transporter_capacity(punit);
-  new_cap = unit_types[upgrade_type].transport_capacity;
-
-  if (new_cap >= old_cap)
-    return FALSE;
-
-  tile_cap = 0;
-  tile_ncargo = 0;
-  unit_list_iterate(map_get_tile(punit->x, punit->y)->units, punit2) {
-    if (punit2->owner == punit->owner
-        || pplayers_allied(unit_owner(punit2), unit_owner(punit))) {
-      if (is_sailing_unit(punit2) && is_ground_units_transport(punit2)) { 
-       tile_cap += get_transporter_capacity(punit2);
-      } else if (is_ground_unit(punit2)) {
-       tile_ncargo++;
-      }
-    }
-  }
-  unit_list_iterate_end;
-
-  if (tile_ncargo <= tile_cap - old_cap + new_cap)
-    return FALSE;
-
-  freelog(LOG_VERBOSE, "Can't upgrade %s at (%d,%d)"
-         " because would strand passenger(s)",
-         unit_type(punit)->name, punit->x, punit->y);
-  return TRUE;
-}
-
-/***************************************************************************
 Do Leonardo's Workshop upgrade(s). Select unit to upgrade by random. --Zamar
 Now be careful not to strand units at sea with the Workshop. --dwp
 ****************************************************************************/
 static void handle_leonardo(struct player *pplayer)
 {
-  int upgrade_type; 
   int leonardo_variant;
        
   struct unit_list candidates;
@@ -227,9 +174,11 @@
   unit_list_init(&candidates);
        
   unit_list_iterate(pplayer->units, punit) {
-    if ((upgrade_type=can_upgrade_unittype(pplayer, punit->type))!=-1 &&
-       !upgrade_would_strand(punit, upgrade_type))
-      unit_list_insert(&candidates, punit); /* Potential candidate :) */
+    Unit_Type_id upgrade_type = can_upgrade_unittype(pplayer, punit->type);
+    if (upgrade_type != -1
+       && unit_upgrade(punit, upgrade_type, TRUE, FALSE, FALSE) == UR_OK) {
+      unit_list_insert(&candidates, punit);    /* Potential candidate :) */
+    }
   } unit_list_iterate_end;
        
   if (unit_list_size(&candidates) == 0)
@@ -241,7 +190,9 @@
   i=0; 
   unit_list_iterate(candidates, punit) {
     if (leonardo_variant != 0 || i == candidate_to_upgrade) {
-      upgrade_type=can_upgrade_unittype(pplayer, punit->type);
+      enum unit_upgrade_result result;
+      Unit_Type_id upgrade_type = can_upgrade_unittype(pplayer, punit->type);
+
       notify_player(pplayer,
             _("Game: %s has upgraded %s to %s%s."),
             improvement_types[B_LEONARDO].name,
@@ -249,7 +200,8 @@
             get_unit_type(upgrade_type)->name,
             get_location_str_in(pplayer, punit->x, punit->y));
       punit->veteran = FALSE;
-      upgrade_unit(punit, upgrade_type);
+      result = unit_upgrade(punit, upgrade_type, FALSE, FALSE, FALSE);
+      assert(result == UR_OK);
     }
     i++;
   } unit_list_iterate_end;
@@ -1398,22 +1350,6 @@
       cap++;
     return cap>0;
   }
-}
-
-/****************************************************************************
-  Expensive function to check how many units are in the transport.
-****************************************************************************/
-int get_transporter_occupancy(struct unit *ptrans)
-{
-  int occupied = 0;
-
-  unit_list_iterate(map_get_tile(ptrans->x, ptrans->y)->units, pcargo) {
-    if (pcargo->transported_by == ptrans->id) {
-      occupied++;
-    }
-  } unit_list_iterate_end;
-
-  return occupied;
 }
 
 /**************************************************************************
Index: server/unittools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.h,v
retrieving revision 1.57
diff -u -u -r1.57 unittools.h
--- server/unittools.h  2003/10/08 16:56:07     1.57
+++ server/unittools.h  2003/11/10 17:23:29
@@ -29,7 +29,6 @@
 /* move check related */
 bool is_airunit_refuel_point(int x, int y, struct player *pplayer,
                             Unit_Type_id type, bool unit_is_on_tile);
-int get_transporter_occupancy(struct unit *ptrans);
 
 /* turn update related */
 void player_restore_units(struct player *pplayer);