[Freeciv-Dev] Re: package_dumb_city: Assertion failed (PR#1266)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 20, 2002 at 01:59:56PM -0800, Christian Knoke wrote:
> On Mon, Feb 18, 2002 at 03:41:15PM +0100, Raimar Falke wrote:
> > 
> > I'm lost. Infos so far: the savegame dump_city.sav.gz contains a
> > dumb_city entry for "Carncastle" for player "chris" but "Carncastle"
> > isn't anymore -> assert triggers. I suspect that this was caused by my
> > change which introduces player_has_traderoute_with_city in
> > server/citytools.c. However this change only triggers if there is a
> > traderoute with the city ("Carncastle"). And Christian didn't build
> > one. If it is an old error why was it uncovered now? Any ideas?
> 
> This bug started to annoy me, because it happens often. So I digged 
> into it.
> 
> It looks for me that savegames are corrupt in a way I can't tell,
> so you find your game not reloadable, and a lot of previous savegames
> too :-( 
> 
> I've found a way to reproduce the savegame getting corrupt :)
> 
> Try this: http://www.enter.de/~c.knoke/bugs/dumbo.sav.gz
> 
> Login as chris. Move the caravelle 3 tiles NE, discovering the
> spanish city of Valencia of size 1. Watch the barbarian musketeer
> in front of Valencia's undefended gates. Move the Caravelle *out*
> *of* *sight* of Valencia. Move the explorer SE. Save the game.
> Reload the saved game, login as chris, start, crash boom bang.

Very good debug report. The savegames aren't corrupt but the server is
buggy. Bug was introduced with this change:

+/**************************************************************************
 This fills out a package from a players dumb_city.
 **************************************************************************/
 static void package_dumb_city(struct player* pplayer, int x, int y,
                              struct packet_short_city *packet)
 {
   struct dumb_city *pdcity = map_get_player_tile(x, y, pplayer)->city;
-  struct city *pcity;
+  struct city *pcity = map_get_city(x, y);
+
+  assert(pcity);
+
   packet->id=pdcity->id;
   packet->owner=pdcity->owner;
   packet->x=x;
@@ -1285,19 +1317,23 @@
   if (map_get_known_and_seen(x, y, pplayer)) {
     /* Since the tile is visible the player can see the tile,
        and if it didn't actually have a city pdcity would be NULL */
-    pcity = map_get_tile(x,y)->city;
     packet->happy = !city_unhappy(pcity);
   } else {
     packet->happy=1;
   }

-  if ((pcity = map_get_city(x,y)) && pcity->id == pdcity->id &&
-      city_got_building(pcity,  B_PALACE))
+  if (pcity->id == pdcity->id && city_got_building(pcity, B_PALACE))
     packet->capital=1;
   else
     packet->capital=0;

   packet->walls = pdcity->has_walls;
+
+  if (player_has_traderoute_with_city(pplayer, pcity)) {
+    packet->tile_trade = pcity->tile_trade;
+  } else {
+    packet->tile_trade = 0;
+  }
 }

The assert was put a level to high. The attached patch fixes this.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 checking for the vaidity of the Maxwell laws on this machine... ok
 checking if e=mc^2... ok
 checking if we can safely swap on /dev/fd0... yes
    -- kvirc 2.0.0's configure

package_dumb_city1.diff
Description: Text document