URL:
<http://gna.org/bugs/?19175>
Summary: lookup_*() in ruleset.c look dubious in some error
cases
Project: Freeciv
Submitted by: jtn
Submitted on: Mon Dec 12 01:13:00 2011
Category: general
Severity: 2 - Minor
Priority: 5 - Normal
Status: None
Assigned to: None
Originator Email:
Open/Closed: Open
Release: S2_4 r20647
Discussion Lock: Any
Operating System: Any
Planned Release:
_______________________________________________________
Details:
lookup_tech() and friends return an advance (etc) from a secfile entry.
In S1_14 we had:
sval = secfile_lookup_str(file, "%s.%s", prefix, entry);
if (!required && strcmp(sval, "Never")==0) {
i = A_LAST;
} else {
i = find_tech_by_name(sval);
if (i==A_LAST) {
freelog((required?LOG_FATAL:LOG_ERROR), /* ...*/);
if (required) {
exit(EXIT_FAILURE);
} else {
i = A_LAST;
}
}
}
return i;
with the following possibilities:
* Entry not in secfile: *fatal* (secfile_lookup_str() kills server)
* Entry exists in secfile:
** It's the string "Never":
*** required: *fatal* (find_tech_by_name() won't find it)
*** !required: return A_LAST
** It's an unmatchable string (garbage):
*** required: *fatal*
*** !required: return A_LAST (and moan at LOG_ERROR)
** It's a matchable string (success case):
*** return its id
Advances became "struct advance *"s rather than ints by S2_0. In the process,
I think the logic got broken. Today we have:
sval = secfile_lookup_str_default(file, NULL, "%s.%s", prefix, entry);
if (!sval || (LOG_FATAL < loglevel && strcmp(sval, "Never") == 0)) {
padvance = A_NEVER;
} else {
padvance = advance_by_rule_name(sval);
if (A_NEVER == padvance) {
ruleset_error(loglevel, /* ... */);
/* ruleset_error returned only if error was not fatal. */
}
}
return padvance;
(where the old "required" is logically loglevel>LOG_FATAL (i.e., LOG_ERROR
etc)
* Entry not in secfile: return A_NEVER (==NULL)
* Entry exists in secfile:
** It's the string "Never":
*** required: *fatal* (advance_by_rule_name() won't find it, ruleset_error()
dies)
*** !required: return A_NEVER
** It's an unmatchable string (garbage):
*** required: *fatal*
*** !required: return A_NEVER (and moan at loglevel)
** It's a matchable string (success case):
*** return its id
So, the old version could never return A_LAST if "required" was set, but the
new version can return A_NEVER even in this case. I suspect callers aren't
prepared for that. Indeed, commenting out tech_req for a unit causes a
segfault.
It would seem more useful for the "required" behaviour to be never to return
A_NEVER. The details are arguable, however.
_______________________________________________________
Reply to this item at:
<http://gna.org/bugs/?19175>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev
