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

2008/4/27 Marko Lindqvist:
>
>   Observing autogame player:
>   handle_city_info() 3 citizens not equal 2 city size in "Nagasaki".

 For debugging the problem I added check to the server side to get
breakpoint when it is about to send such inconsistent packet. Turned
out that these inconsistent packets are usually sent as part of
transfer_city(), but problem itself is not (necessarily) there. Cities
are already inconsistent before they are transfered. I tried also
adding sanity check for this, but I had to take it off simply because
it was failing constantly and producing far too much warnings. It sees
that cities are in inconsistent state in server side most of the time,
but usually their consistency is restored before sending them to
client (so client side warning has not been failing constantly)

 Attached patch fails to fix the bug (at least all of it), but makes
future debugging easier:
 - Adds server side consistency check before sending city packet to
client. If it fails, error message is printed and city consistency
restored with refresh_city()
 - Adds sanity check function against this problem. This function is
not called anywhere in the current code as it causes too much warnings
spam
 - Fixes two of the more obvious cases where city gets inconsistent
state. This requires calling refresh_city() resulting in inefficient
looking code. Some optimization rearrangements are probably needed
later - once we've got the actual bug fixed


 - ML

diff -Nurd -X.diff_ignore freeciv/server/citytools.c freeciv/server/citytools.c
--- freeciv/server/citytools.c	2008-04-05 01:48:20.000000000 +0300
+++ freeciv/server/citytools.c	2008-04-29 02:19:24.000000000 +0300
@@ -1053,7 +1053,10 @@
 
   if (NULL != pwork) {
     /* was previously worked by another city */
+    /* Turn citizen into specialist. */
     pwork->specialists[DEFAULT_SPECIALIST]++;
+    /* One less citizen. Update feelings counters */
+    city_refresh(pwork);
     pwork->server.synced = FALSE;
     city_freeze_workers_queue(pwork);
   }
@@ -1691,6 +1694,7 @@
 		  bool dipl_invest)
 {
   int i;
+  int ppl = 0;
 
   packet->id=pcity->id;
   packet->owner = player_number(city_owner(pcity));
@@ -1704,13 +1708,43 @@
     packet->ppl_content[i] = pcity->feel[CITIZEN_CONTENT][i];
     packet->ppl_unhappy[i] = pcity->feel[CITIZEN_UNHAPPY][i];
     packet->ppl_angry[i] = pcity->feel[CITIZEN_ANGRY][i];
+    if (i == 0) {
+      ppl += packet->ppl_happy[i];
+      ppl += packet->ppl_content[i];
+      ppl += packet->ppl_unhappy[i];
+      ppl += packet->ppl_angry[i];
+    }
   }
   /* The number of data in specialists[] array */
   packet->specialists_size = specialist_count();
   specialist_type_iterate(sp) {
     packet->specialists[sp] = pcity->specialists[sp];
+    ppl += packet->specialists[sp];
   } specialist_type_iterate_end;
 
+  if (packet->size != ppl) {
+    static bool recursion = FALSE;
+
+    if (recursion) {
+      /* Recursion didn't help. Do not enter infinite recursive loop.
+       * Package city as it is. */
+      freelog(LOG_ERROR, "Failed to fix inconsistent city size.");
+      recursion = FALSE;
+    } else {
+      freelog(LOG_ERROR, "City size %d, citizen count %d for %s",
+              packet->size, ppl, city_name(pcity));
+      /* Try to fix */
+      city_refresh(pcity);
+
+      /* And repackage */
+      recursion = TRUE;
+      package_city(pcity, packet, dipl_invest);
+      recursion = FALSE;
+
+      return;
+    }
+  }
+
   for (i = 0; i < NUM_TRADEROUTES; i++) {
     packet->trade[i]=pcity->trade[i];
     packet->trade_value[i]=pcity->trade_value[i];
diff -Nurd -X.diff_ignore freeciv/server/cityturn.c freeciv/server/cityturn.c
--- freeciv/server/cityturn.c	2008-03-12 21:57:04.000000000 +0200
+++ freeciv/server/cityturn.c	2008-04-29 03:08:47.000000000 +0300
@@ -481,6 +481,7 @@
 **************************************************************************/
 bool city_reduce_size(struct city *pcity, int pop_loss)
 {
+  int loss_remain;
   int i;
 
   if (pop_loss == 0) {
@@ -499,24 +500,28 @@
   }
 
   /* First try to kill off the specialists */
-  i = pop_loss - city_reduce_specialists(pcity, pop_loss);
+  loss_remain = pop_loss - city_reduce_specialists(pcity, pop_loss);
 
-  if (0 < i) {
+  /* Update number of people in each feelings category.
+   * This must be after new pcity->size and specialists counts
+   * have been set, and before any auto_arrange_workers() */
+  city_refresh(pcity);
+
+  if (loss_remain > 0) {
     /* Take it out on workers */
-    i -= city_reduce_workers(pcity, i);
+    loss_remain -= city_reduce_workers(pcity, loss_remain);
 
     /* Then rearrange workers */
     auto_arrange_workers(pcity);
     sync_cities();
   } else {
-    city_refresh(pcity);
     send_city_info(city_owner(pcity), pcity);
   }
 
-  if (0 != i) {
+  if (0 != loss_remain) {
     freelog(LOG_FATAL, "city_reduce_size()"
             " has remaining %d of %d for \"%s\"[%d]",
-            i, pop_loss,
+            loss_remain, pop_loss,
             city_name(pcity), pcity->size);
     assert(0);
   }
diff -Nurd -X.diff_ignore freeciv/server/diplomats.c freeciv/server/diplomats.c
--- freeciv/server/diplomats.c	2008-01-20 04:02:12.000000000 +0200
+++ freeciv/server/diplomats.c	2008-04-29 03:02:37.000000000 +0300
@@ -686,9 +686,7 @@
   maybe_cause_incident(DIPLOMAT_INCITE, pplayer, NULL, pcity);
 
   /* Transfer city and units supported by this city (that
-     are within one square of the city) to the new owner.
-     Remember that pcity is destroyed as part of the transfer,
-     Which is why we do this last */
+     are within one square of the city) to the new owner. */
   transfer_city (pplayer, pcity, 1, TRUE, TRUE, FALSE);
 
   /* Check if a spy survives her mission. Diplomats never do.
diff -Nurd -X.diff_ignore freeciv/server/sanitycheck.c freeciv/server/sanitycheck.c
--- freeciv/server/sanitycheck.c	2008-03-10 20:08:03.000000000 +0200
+++ freeciv/server/sanitycheck.c	2008-04-29 03:11:26.000000000 +0300
@@ -427,6 +427,28 @@
 }
 
 /**************************************************************************
+  Verify that the number of people with feelings + specialists equal
+  city size.
+**************************************************************************/
+void real_sanity_check_feelings(const struct city *pcity,
+                                const char *file, int line)
+{
+  int ccategory;
+  int spe = city_specialists(pcity);
+
+  for (ccategory = CITIZEN_HAPPY; ccategory < CITIZEN_LAST; ccategory++) {
+    int sum = 0;
+    int feel;
+
+    for (feel = FEELING_BASE; feel < FEELING_LAST; feel++) {
+      sum += pcity->feel[ccategory][feel];
+    }
+
+    SANITY_CITY(pcity, pcity->size - spe == sum);
+  }
+}
+
+/**************************************************************************
   Verify that the city has sane values.
 **************************************************************************/
 void real_sanity_check_city_all(struct city *pcity, const char *file, int line)
@@ -434,6 +456,7 @@
   if (check_city_good(pcity, file, line)) {
     check_city_map(pcity, file, line);
     check_city_size(pcity, file, line);
+    /* TODO: check_feelings. Currently fails far too often to be included. */
   }
 }
 
diff -Nurd -X.diff_ignore freeciv/server/sanitycheck.h freeciv/server/sanitycheck.h
--- freeciv/server/sanitycheck.h	2008-03-10 20:08:03.000000000 +0200
+++ freeciv/server/sanitycheck.h	2008-04-29 03:10:47.000000000 +0300
@@ -31,11 +31,16 @@
 #  define sanity_check() real_sanity_check(__FILE__, __LINE__)
 void real_sanity_check(const char *file, int line);
 
+#  define sanity_check_feelings(x) real_sanity_check_feelings(x, __FILE__, __LINE__)
+void real_sanity_check_feelings(const struct city *pcity,
+                                const char *file, int line);
+
 #else /* SANITY_CHECKING */
 
 #  define sanity_check_city_all(x) (void)0
 #  define sanity_check_city(x) (void)0
 #  define sanity_check() (void)0
+#  define sanity_check_feelings(x) (void)0
 
 #endif /* SANITY_CHECKING */
 
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to