<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
[email protected]
https://mail.gna.org/listinfo/freeciv-dev
