From 1e669472a6f9c2317b9c5d6e28574d81a9e66679 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 7 Jul 2015 09:29:43 +0200 Subject: [PATCH 1/4] add a test and fix potential crashes in SABOTAGE command. sometimes even a simple test will uncover a ton of small errors. --- src/spy.c | 37 +++++++++++++++++++++++-------------- src/spy.test.c | 27 +++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 14 deletions(-) diff --git a/src/spy.c b/src/spy.c index 7d9c6529a..73af0148f 100644 --- a/src/spy.c +++ b/src/spy.c @@ -394,7 +394,7 @@ static int try_destruction(unit * u, unit * u2, const ship * sh, int skilldiff) return 1; /* success */ } -static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur) +static void sink_ship(region * r, ship * sh, unit * saboteur) { unit **ui, *u; region *safety = r; @@ -404,6 +404,9 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur) message *sink_msg = NULL; faction *f; + assert(r); + assert(sh); + assert(saboteur); for (f = NULL, u = r->units; u; u = u->next) { /* slight optimization to avoid dereferencing u->faction each time */ if (f != u->faction) { @@ -426,7 +429,7 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur) } } } - for (ui = &r->units; *ui; ui = &(*ui)->next) { + for (ui = &r->units; *ui;) { unit *u = *ui; /* inform this faction about the sinking ship: */ @@ -471,12 +474,13 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur) add_message(&u->faction->msgs, msg); msg_release(msg); if (dead == u->number) { - /* the poor creature, she dies */ - if (remove_unit(ui, u) != 0) { - ui = &u->next; + if (remove_unit(ui, u) == 0) { + /* ui is already pointing at u->next */ + continue; } } } + ui = &u->next; } if (sink_msg) msg_release(sink_msg); @@ -487,19 +491,21 @@ static void sink_ship(region * r, ship * sh, const char *name, unit * saboteur) int sabotage_cmd(unit * u, struct order *ord) { const char *s; - int i; + param_t p; ship *sh; unit *u2; - char buffer[DISPLAYSIZE]; - region *r = u->region; - int skdiff; + region *r; + int skdiff = INT_MAX; + + assert(u); + assert(ord); init_order(ord); s = getstrtoken(); - i = findparam(s, u->faction->locale); + p = findparam(s, u->faction->locale); - switch (i) { + switch (p) { case P_SHIP: sh = u->ship; if (!sh) { @@ -507,10 +513,13 @@ int sabotage_cmd(unit * u, struct order *ord) return 0; } u2 = ship_owner(sh); - skdiff = - eff_skill(u, SK_SPY, r) - crew_skill(r, u2->faction, sh, SK_PERCEPTION); + r = u->region; + if (u2->faction != u->faction) { + skdiff = + eff_skill(u, SK_SPY, r) - crew_skill(r, u2->faction, sh, SK_PERCEPTION); + } if (try_destruction(u, u2, sh, skdiff)) { - sink_ship(r, sh, buffer, u); + sink_ship(r, sh, u); } break; default: diff --git a/src/spy.test.c b/src/spy.test.c index e0f8c2043..7e0f715c0 100644 --- a/src/spy.test.c +++ b/src/spy.test.c @@ -5,9 +5,11 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -16,6 +18,7 @@ #include "spy.h" +#include #include #include @@ -98,12 +101,36 @@ static void test_all_spy_message(CuTest *tc) { test_cleanup(); } +static void test_sabotage_self(CuTest *tc) { + unit *u; + region *r; + order *ord; + struct locale *lang; + test_cleanup(); + lang = get_or_create_locale("de"); + locale_setstring(lang, parameters[P_SHIP], "SCHIFF"); + test_create_world(); + init_locales(); + + r = test_create_region(0, 0, NULL); + assert(r); + u = test_create_unit(test_create_faction(NULL), r); + assert(u && u->faction && u->region==r); + u->ship = test_create_ship(r, test_create_shiptype("boat")); + assert(u->ship); + ord = create_order(K_SABOTAGE, lang, "SCHIFF"); + assert(ord); + CuAssertIntEquals(tc, 0, sabotage_cmd(u, ord)); + CuAssertPtrEquals(tc, 0, r->ships); + test_cleanup(); +} CuSuite *get_spy_suite(void) { CuSuite *suite = CuSuiteNew(); SUITE_ADD_TEST(suite, test_simple_spy_message); SUITE_ADD_TEST(suite, test_all_spy_message); + SUITE_ADD_TEST(suite, test_sabotage_self); return suite; } From d7e5876c62001e1848fdc7c2dc12d7e1483cdc79 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 7 Jul 2015 09:44:24 +0200 Subject: [PATCH 2/4] fix build, missing limits.h include --- src/spy.c | 3 ++- src/spy.test.c | 26 +++++++++++++------------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/spy.c b/src/spy.c index 73af0148f..a440ab3aa 100644 --- a/src/spy.c +++ b/src/spy.c @@ -53,6 +53,7 @@ OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. #include #include #include +#include /* in spy steht der Unterschied zwischen Wahrnehmung des Opfers und * Spionage des Spions */ @@ -61,7 +62,7 @@ void spy_message(int spy, const unit * u, const unit * target) const char *str = report_kampfstatus(target, u->faction->locale); ADDMSG(&u->faction->msgs, msg_message("spyreport", "spy target status", u, - target, str)); + target, str)); if (spy > 20) { sc_mage *mage = get_mage(target); /* for mages, spells and magic school */ diff --git a/src/spy.test.c b/src/spy.test.c index 7e0f715c0..46b52e32d 100644 --- a/src/spy.test.c +++ b/src/spy.test.c @@ -68,9 +68,9 @@ static void test_simple_spy_message(CuTest *tc) { static void set_factionstealth(unit *u, faction *f) { attrib *a = a_find(u->attribs, &at_otherfaction); if (!a) - a = a_add(&u->attribs, make_otherfaction(f)); + a = a_add(&u->attribs, make_otherfaction(f)); else - a->data.v = f; + a->data.v = f; } static void test_all_spy_message(CuTest *tc) { @@ -92,11 +92,11 @@ static void test_all_spy_message(CuTest *tc) { spy_message(99, fix.spy, fix.victim); assert_messages(tc, fix.spy->faction->msgs->begin, fix.msg_types, 5, true, - M_BASE, - M_MAGE, - M_FACTION, - M_SKILLS, - M_ITEMS); + M_BASE, + M_MAGE, + M_FACTION, + M_SKILLS, + M_ITEMS); test_cleanup(); } @@ -116,7 +116,7 @@ static void test_sabotage_self(CuTest *tc) { r = test_create_region(0, 0, NULL); assert(r); u = test_create_unit(test_create_faction(NULL), r); - assert(u && u->faction && u->region==r); + assert(u && u->faction && u->region == r); u->ship = test_create_ship(r, test_create_shiptype("boat")); assert(u->ship); ord = create_order(K_SABOTAGE, lang, "SCHIFF"); @@ -128,9 +128,9 @@ static void test_sabotage_self(CuTest *tc) { CuSuite *get_spy_suite(void) { - CuSuite *suite = CuSuiteNew(); - SUITE_ADD_TEST(suite, test_simple_spy_message); - SUITE_ADD_TEST(suite, test_all_spy_message); - SUITE_ADD_TEST(suite, test_sabotage_self); - return suite; + CuSuite *suite = CuSuiteNew(); + SUITE_ADD_TEST(suite, test_simple_spy_message); + SUITE_ADD_TEST(suite, test_all_spy_message); + SUITE_ADD_TEST(suite, test_sabotage_self); + return suite; } From f9130fcb38b9c22835fa369a2f97f6f59c4bba9c Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 7 Jul 2015 13:38:14 +0200 Subject: [PATCH 3/4] some more easy tests for sabotage. --- src/kernel/ship.c | 15 +++++++++++ src/kernel/ship.h | 5 ++-- src/move.c | 14 ---------- src/move.h | 1 - src/spy.test.c | 65 +++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 78 insertions(+), 22 deletions(-) diff --git a/src/kernel/ship.c b/src/kernel/ship.c index 1ae81b436..c5aded3cd 100644 --- a/src/kernel/ship.c +++ b/src/kernel/ship.c @@ -445,3 +445,18 @@ const char *ship_getname(const ship * self) { return self->name; } + +unit *get_captain(const ship * sh) +{ + const region *r = sh->region; + unit *u; + + for (u = r->units; u; u = u->next) { + if (u->ship == sh && u->number + && eff_skill(u, SK_SAILING, r) >= sh->type->cptskill) + return u; + } + + return NULL; +} + diff --git a/src/kernel/ship.h b/src/kernel/ship.h index 809e678a4..f123242df 100644 --- a/src/kernel/ship.h +++ b/src/kernel/ship.h @@ -121,9 +121,10 @@ extern "C" { extern void free_ship(struct ship *s); extern void free_ships(void); - extern const char *ship_getname(const struct ship *self); - extern void ship_setname(struct ship *self, const char *name); + const char *ship_getname(const struct ship *self); + void ship_setname(struct ship *self, const char *name); int shipspeed(const struct ship *sh, const struct unit *u); + struct unit *get_captain(const struct ship *sh); #ifdef __cplusplus } diff --git a/src/move.c b/src/move.c index b14dfb68d..f29dd08ce 100644 --- a/src/move.c +++ b/src/move.c @@ -2130,20 +2130,6 @@ sail(unit * u, order * ord, bool move_on_land, region_list ** routep) } } -unit *get_captain(const ship * sh) -{ - const region *r = sh->region; - unit *u; - - for (u = r->units; u; u = u->next) { - if (u->ship == sh && u->number - && eff_skill(u, SK_SAILING, r) >= sh->type->cptskill) - return u; - } - - return NULL; -} - /* Segeln, Wandern, Reiten * when this routine returns a non-zero value, movement for the region needs * to be done again because of followers that got new MOVE orders. diff --git a/src/move.h b/src/move.h index f1c20d622..f35012b07 100644 --- a/src/move.h +++ b/src/move.h @@ -64,7 +64,6 @@ extern "C" { int enoughsailors(const struct ship *sh, const struct region *r); bool canswim(struct unit *u); bool canfly(struct unit *u); - struct unit *get_captain(const struct ship *sh); void travelthru(const struct unit *u, struct region *r); struct ship *move_ship(struct ship *sh, struct region *from, struct region *to, struct region_list *route); diff --git a/src/spy.test.c b/src/spy.test.c index 46b52e32d..7f97eb171 100644 --- a/src/spy.test.c +++ b/src/spy.test.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -101,10 +102,7 @@ static void test_all_spy_message(CuTest *tc) { test_cleanup(); } -static void test_sabotage_self(CuTest *tc) { - unit *u; - region *r; - order *ord; +static void setup_sabotage(void) { struct locale *lang; test_cleanup(); @@ -112,25 +110,82 @@ static void test_sabotage_self(CuTest *tc) { locale_setstring(lang, parameters[P_SHIP], "SCHIFF"); test_create_world(); init_locales(); +} +static void test_sabotage_self(CuTest *tc) { + unit *u; + region *r; + order *ord; + + setup_sabotage(); r = test_create_region(0, 0, NULL); assert(r); u = test_create_unit(test_create_faction(NULL), r); assert(u && u->faction && u->region == r); u->ship = test_create_ship(r, test_create_shiptype("boat")); assert(u->ship); - ord = create_order(K_SABOTAGE, lang, "SCHIFF"); + ord = create_order(K_SABOTAGE, u->faction->locale, "SCHIFF"); assert(ord); CuAssertIntEquals(tc, 0, sabotage_cmd(u, ord)); CuAssertPtrEquals(tc, 0, r->ships); test_cleanup(); } + +static void test_sabotage_other_fail(CuTest *tc) { + unit *u, *u2; + region *r; + order *ord; + + setup_sabotage(); + r = test_create_region(0, 0, NULL); + assert(r); + u = test_create_unit(test_create_faction(NULL), r); + u2 = test_create_unit(test_create_faction(NULL), r); + assert(u && u2); + u2->ship = test_create_ship(r, test_create_shiptype("boat")); + assert(u2->ship); + u->ship = u2->ship; + ship_update_owner(u->ship); + assert(ship_owner(u->ship) == u); + ord = create_order(K_SABOTAGE, u->faction->locale, "SCHIFF"); + assert(ord); + CuAssertIntEquals(tc, 0, sabotage_cmd(u2, ord)); + CuAssertPtrNotNull(tc, r->ships); + test_cleanup(); +} + + +static void test_sabotage_other_success(CuTest *tc) { + unit *u, *u2; + region *r; + order *ord; + + setup_sabotage(); + r = test_create_region(0, 0, NULL); + assert(r); + u = test_create_unit(test_create_faction(NULL), r); + u2 = test_create_unit(test_create_faction(NULL), r); + assert(u && u2); + u2->ship = test_create_ship(r, test_create_shiptype("boat")); + assert(u2->ship); + u->ship = u2->ship; + ship_update_owner(u->ship); + assert(ship_owner(u->ship) == u); + ord = create_order(K_SABOTAGE, u->faction->locale, "SCHIFF"); + assert(ord); + set_level(u2, SK_SPY, 1); + CuAssertIntEquals(tc, 0, sabotage_cmd(u2, ord)); + CuAssertPtrEquals(tc, 0, r->ships); + test_cleanup(); +} CuSuite *get_spy_suite(void) { CuSuite *suite = CuSuiteNew(); SUITE_ADD_TEST(suite, test_simple_spy_message); SUITE_ADD_TEST(suite, test_all_spy_message); SUITE_ADD_TEST(suite, test_sabotage_self); + SUITE_ADD_TEST(suite, test_sabotage_other_fail); + SUITE_ADD_TEST(suite, test_sabotage_other_success); return suite; } From 391a123a3ed3a1eeac5885b9dda7c762e9c58d88 Mon Sep 17 00:00:00 2001 From: Enno Rehling Date: Tue, 7 Jul 2015 14:42:07 +0200 Subject: [PATCH 4/4] test that correct messages are sent to correct factions. --- src/kernel/ship.c | 5 +---- src/spy.test.c | 6 ++++++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/kernel/ship.c b/src/kernel/ship.c index c5aded3cd..a01326bdd 100644 --- a/src/kernel/ship.c +++ b/src/kernel/ship.c @@ -435,10 +435,7 @@ void write_ship_reference(const struct ship *sh, struct storage *store) void ship_setname(ship * self, const char *name) { free(self->name); - if (name) - self->name = _strdup(name); - else - self->name = NULL; + self->name = name ? _strdup(name) : 0; } const char *ship_getname(const ship * self) diff --git a/src/spy.test.c b/src/spy.test.c index 7f97eb171..085cc5675 100644 --- a/src/spy.test.c +++ b/src/spy.test.c @@ -136,6 +136,7 @@ static void test_sabotage_other_fail(CuTest *tc) { unit *u, *u2; region *r; order *ord; + message *msg; setup_sabotage(); r = test_create_region(0, 0, NULL); @@ -151,6 +152,10 @@ static void test_sabotage_other_fail(CuTest *tc) { ord = create_order(K_SABOTAGE, u->faction->locale, "SCHIFF"); assert(ord); CuAssertIntEquals(tc, 0, sabotage_cmd(u2, ord)); + msg = test_get_last_message(u2->faction->msgs); + CuAssertStrEquals(tc, "destroy_ship_1", test_get_messagetype(msg)); + msg = test_get_last_message(u->faction->msgs); + CuAssertStrEquals(tc, "destroy_ship_3", test_get_messagetype(msg)); CuAssertPtrNotNull(tc, r->ships); test_cleanup(); } @@ -179,6 +184,7 @@ static void test_sabotage_other_success(CuTest *tc) { CuAssertPtrEquals(tc, 0, r->ships); test_cleanup(); } + CuSuite *get_spy_suite(void) { CuSuite *suite = CuSuiteNew();