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

2008/4/29 Marko Lindqvist:
>   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

 More complete patch. With this my example autogame never tries to
send inconsistent packages.

 Santiy check still cannot be anbled by default. I don't know if
server still has some real internal problem, or if sanity checking
just runs at such point where consistency is not needed (unnecessary
refresh optimized away)


 - 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-05-01 18:45:28.000000000 +0300
@@ -117,6 +117,7 @@
   pcity->server.workers_frozen--;
   assert(pcity->server.workers_frozen >= 0);
   if (pcity->server.workers_frozen == 0 && pcity->server.needs_arrange) {
+    city_refresh(pcity); /* Citizen count sanity */
     auto_arrange_workers(pcity);
   }
 }
@@ -1053,7 +1054,10 @@
 
   if (NULL != pwork) {
     /* was previously worked by another city */
+    /* Turn citizen into specialist. */
     pwork->specialists[DEFAULT_SPECIALIST]++;
+    /* One less citizen. Citizen sanity will be handled later in
+     * city_thaw_workers_queue() */
     pwork->server.synced = FALSE;
     city_freeze_workers_queue(pwork);
   }
@@ -1691,6 +1695,7 @@
 		  bool dipl_invest)
 {
   int i;
+  int ppl = 0;
 
   packet->id=pcity->id;
   packet->owner = player_number(city_owner(pcity));
@@ -1704,13 +1709,45 @@
     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 {
+      /* Note: If you get this error and try to debug the cause, you may find
+       *       using sanity_check_feelings() in some key points useful. */
+      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];
@@ -2091,6 +2128,7 @@
     if (queued) {
       city_freeze_workers_queue(pwork); /* place the displaced later */
     } else {
+      city_refresh(pwork); /* Specialist added, keep citizen count sanity */
       auto_arrange_workers(pwork);
       send_city_info(NULL, pwork);
     }
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-05-01 18:43:17.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,10 +456,10 @@
   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. */
   }
 }
 
-
 /**************************************************************************
   Verify that the city has sane values.
 **************************************************************************/
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-05-01 14:53:14.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