Commit Graph

12 Commits

Author SHA1 Message Date
Pau Espin b885ef88c1 tests: ctrl: Test received ERROR messages are handled correctly
Change-Id: I3c8e95aaa1ca222d4cd1395e548f8461bf9d4cd6
2018-07-16 17:56:25 +00:00
Neels Hofmeyr cdbc9afe5d ctrl: fix deferred commands (and hence fix osmo-bts-sysmo 'clock-info' cmd)
The CTRL interface has a ctrl_cmd_def_* API that allows deferring a CTRL
command reply until later. However, the command handling currently fails to
acknowledge this and deallocates the struct ctrl_cmd anyway.

Fix: in struct ctrl_cmd, add a defer pointer to be populated by
ctrl_cmd_def_make(). A cmd thus marked as deferred is not deallocated at the
end of command handling. This fix needs no change in calling code.

(Another idea was to return a different code than CTRL_CMD_HANDLED when the
command is to be deferred, but that would require adjusting each user of
ctrl_cmd_def_make(). The implicit marking is safer and easier.)

Show that handling deferred commands is fixed by adjusting the expectations of
ctrl_test.c's test_deferred_cmd() and removing the now obsolete exit_early
label.

One symptom of the breakage is that osmo-bts-sysmo crashes when asked to report
a trx's clock-info, which is aggravated by the fact that the sysmobts-mgr does
ask osmo-bts-sysmo for a clock-info.

The crash appears since Id583b413f8b8bd16e5cf92a8a9e8663903646381 -- it looked
like just fixing an obvious memory leak, which it did as shown by the unit
test, but deferred ctrl commands actually relied on that leak. Both fixed now.

Related: OS#3120
Change-Id: I24232be7dcf7be79f4def91ddc8b8f8005b56318
2018-04-05 03:11:49 +02:00
Neels Hofmeyr 6882b80d96 ctrl: test deferred cmd, show current failure
Handling a deferred command currently deallocates the struct ctrl_cmd upon
exiting the initial command handling, while it should actually stay around for
the asynchronous/deferred handling of the ctrl command.

Show the current bug by means of a ctrl test. The test will be adjusted to
expect the correct result when the bug is fixed in a subsequent commit
(I24232be7dcf7be79f4def91ddc8b8f8005b56318).

Change-Id: Ibbc847fc583bdd8e5e53a008258805e634ea12b4
2018-04-05 03:10:34 +02:00
Neels Hofmeyr 3da9aa6b67 ctrl: tighten CTRL input parsing
Validate that incoming CTRL commands...
- have decimal IDs,
- return error on trailing characters,
- have invalid characters in variable identifiers,
- send detailed error messages as reply to the requestor.

Adjust ctrl_test.{c,ok}, which best show the change in behavior.

Message handling causes log messages on stderr; previously, stderr was empty.
Add '[ignore]' in testsuite.at so that the nonempty stderr doesn't cause test
failures.

Change-Id: I96a9b6b6a3a5e0b80513aa9eaa727ae8c9c7d7a1
2017-12-20 15:50:24 +00:00
Neels Hofmeyr 1b8b152e79 ctrl_test: add two more current parsing bugs to prep for fix
Change-Id: Id11bc326be2f0bc2746a928354e416495a18baf7
2017-12-18 23:05:50 +00:00
Neels Hofmeyr f2c10f1082 ctrl: fix mem leak when handling GET_REPLY and SET_REPLY
In ctrl_handle_msg() (code recently propagated from handle_control_read()),
talloc_free() the parsed ctrl_cmd in all code paths. In particular, a free was
missing in case ctrl_cmd_handle() returns CTRL_CMD_HANDLED.

CTRL_CMD_HANDLED is triggered by GET_REPLY / SET_REPLY parsing, as show by
ctrl_test.c. With the memleak fixed, adjust expected test output and make a
detected mem leak abort the test immediately.

Change-Id: Id583b413f8b8bd16e5cf92a8a9e8663903646381
2017-12-18 23:05:50 +00:00
Neels Hofmeyr 6769ad6e0a ctrl_test: show memleak when receiving GET_REPLY and SET_REPLY commands
The "memleak!" output shows messages that lack a talloc_free() of the parsed
ctrl command buffer. The leak shall be fixed in a subsequent patch.

Change-Id: I2c3e4d08b769b9cd77593362ea36a28d681cd042
2017-12-18 23:05:50 +00:00
Neels Hofmeyr 83aee83ec6 ctrl_test: expand to test message handling and detect mem leaks
Subsequent patches that tighten CTRL input validation will make the results
more interesting.

Change-Id: Idd4cc7d193db1a7d761f72ed33ed46eea450a78f
2017-12-18 23:05:50 +00:00
Neels Hofmeyr 0ab6eca80a ctrl_test.c: replace print_escaped() with new osmo_escape_str()
Change-Id: I12d3828dcc925f97fde11c360f1d60f3bd8cad8b
2017-12-18 23:05:49 +00:00
Neels Hofmeyr 505c965e36 CTRL: add unit tests for CTRL command parsing
This uncovers some interesting behavior of the CTRL interface which we may want
to guard against in subsequent patches: trailing whitespace, ignored tokens,
special characters as cmd->id.

Change-Id: If7af06d50ca71fd528b08cd70310774d5a53f0f7
2017-09-27 14:04:23 +00:00
Neels Hofmeyr 7c1ec8c8e7 ctrl_type_vals: explicitly terminate
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
2017-03-15 13:47:20 +00:00
Max 70c7d4160d Use value_string for ctrl_type
Use value_string for enum ctrl_type instead of custom code. Add
corresponding unit tests.

Related: OS#1615
Change-Id: Icd4e96dd9f00876cb70b43cfcf42ab4f10311b28
2017-03-01 16:37:59 +00:00