This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.orgHello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/1962 to look at the new patch set (#2). ctrl_type_vals: fix range check, explicitly terminate In ctrl_cmd_parse(), fix missing check for not parseable ctrl type. Don't use CTRL_TYPE_UNKNOWN as value_string[] terminator, use an explicit, more obvious { 0, NULL } termination. Set an explicit string for CTRL_TYPE_UNKNOWN. No other value_string[]s to date have such a "hidden" terminator. BTW, a { 0, "string" } item is not a terminator, only { 0, NULL } is, so we can set a string for CTRL_TYPE_UNKNOWN == 0. Also, having a string value for CTRL_TYPE_UNKNOWN is not harmful because all code paths explicitly check for the CTRL_TYPE_*s that are valid. Adjust the test expectation. >From the ctrl_type_vals enum, remove the = 0, because it is implicitly 0 anyway. One motivation to press this fixup: I am trying to add a script that checks whether all value_string[]s are terminated to our jenkins jobs, and to find that this one is terminated, it would need to interpret the CTRL_TYPE_UNKNOWN constant, which would make things far more complex. At this point, all of the value_string[]s have an explicit termination, and I would like to enforce this from now on -- for readable code and to not spend more time on the validator. The patch adding ctrl_type_vals (Icd4e96dd9f00876cb70b43cfcf42ab4f10311b28) was accepted by another reviewer before I could reconfirm my -1, so this is a fixup to enable the termination checking script patches. Related: I2bc93ab4781487e7685cfb63091a489cd126b1a8 (adds script to libosmocore) I7fe3678b524d602fc6aa14bc0ed06308df809a3e (uses in jenkins.sh) Icd4e96dd9f00876cb70b43cfcf42ab4f10311b28 (adds ctrl_type_vals) Change-Id: Ia99f37464c7b36b587da2cc78f52c82725f02cbc --- M include/osmocom/ctrl/control_cmd.h M src/ctrl/control_cmd.c M tests/ctrl/ctrl_test.ok 3 files changed, 5 insertions(+), 4 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/62/1962/2 diff --git a/include/osmocom/ctrl/control_cmd.h b/include/osmocom/ctrl/control_cmd.h index d82c9af..d9092f3 100644 --- a/include/osmocom/ctrl/control_cmd.h +++ b/include/osmocom/ctrl/control_cmd.h @@ -22,7 +22,7 @@ }; enum ctrl_type { - CTRL_TYPE_UNKNOWN = 0, + CTRL_TYPE_UNKNOWN, CTRL_TYPE_GET, CTRL_TYPE_SET, CTRL_TYPE_GET_REPLY, diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c index 1cc8247..d706995 100644 --- a/src/ctrl/control_cmd.c +++ b/src/ctrl/control_cmd.c @@ -40,13 +40,14 @@ extern vector ctrl_node_vec; const struct value_string ctrl_type_vals[] = { + { CTRL_TYPE_UNKNOWN, "(unknown)" }, { CTRL_TYPE_GET, "GET" }, { CTRL_TYPE_SET, "SET" }, { CTRL_TYPE_GET_REPLY, "GET_REPLY" }, { CTRL_TYPE_SET_REPLY, "SET_REPLY" }, { CTRL_TYPE_TRAP, "TRAP" }, { CTRL_TYPE_ERROR, "ERROR" }, - { CTRL_TYPE_UNKNOWN, NULL } + { 0, NULL } }; /* Functions from libosmocom */ @@ -289,7 +290,7 @@ } cmd->type = get_string_value(ctrl_type_vals, tmp); - if (cmd->type == CTRL_TYPE_UNKNOWN) { + if (cmd->type < 0 || cmd->type == CTRL_TYPE_UNKNOWN) { cmd->type = CTRL_TYPE_ERROR; cmd->id = "err"; cmd->reply = "Request type unknown"; diff --git a/tests/ctrl/ctrl_test.ok b/tests/ctrl/ctrl_test.ok index c4b05e9..8f97a27 100644 --- a/tests/ctrl/ctrl_test.ok +++ b/tests/ctrl/ctrl_test.ok @@ -1,5 +1,5 @@ Checking ctrl types... -ctrl type 0 is unknown 0x0 [PARSE FAILED] +ctrl type 0 is (unknown) -> 0 OK ctrl type 1 is GET -> 1 OK ctrl type 2 is SET -> 2 OK ctrl type 3 is GET_REPLY -> 3 OK -- To view, visit https://gerrit.osmocom.org/1962 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia99f37464c7b36b587da2cc78f52c82725f02cbc Gerrit-PatchSet: 2 Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-Reviewer: Jenkins Builder