[PATCH] libosmocore[master]: ctrl: fix deferred commands (and hence fix osmo-bts-sysmo 'c...
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/7618
to look at the new patch set (#3).
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
---
M include/osmocom/ctrl/control_cmd.h
M src/ctrl/control_cmd.c
M src/ctrl/control_if.c
M tests/ctrl/ctrl_test.c
M tests/ctrl/ctrl_test.ok
5 files changed, 13 insertions(+), 5 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/18/7618/3
diff --git a/include/osmocom/ctrl/control_cmd.h
b/include/osmocom/ctrl/control_cmd.h
index 865b006..a5df753 100644
--- a/include/osmocom/ctrl/control_cmd.h
+++ b/include/osmocom/ctrl/control_cmd.h
@@ -56,6 +56,8 @@
struct llist_head def_cmds;
};
+struct ctrl_cmd_def;
+
struct ctrl_cmd {
struct ctrl_connection *ccon;
enum ctrl_type type;
@@ -64,6 +66,7 @@
char *variable;
char *value;
char *reply;
+ struct ctrl_cmd_def *defer;
};
#define ctrl_cmd_reply_printf(cmd, fmt, args ...) \
diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c
index c747e84..fb0cd2b 100644
--- a/src/ctrl/control_cmd.c
+++ b/src/ctrl/control_cmd.c
@@ -566,6 +566,7 @@
cd = talloc_zero(ctx, struct ctrl_cmd_def);
+ cmd->defer = cd;
cd->cmd = cmd;
cd->data = data;
diff --git a/src/ctrl/control_if.c b/src/ctrl/control_if.c
index 07de0d4..df8abbc 100644
--- a/src/ctrl/control_if.c
+++ b/src/ctrl/control_if.c
@@ -401,6 +401,13 @@
if (cmd->type != CTRL_TYPE_ERROR) {
cmd->ccon = ccon;
if (ctrl_cmd_handle(ctrl, cmd, ctrl->data) == CTRL_CMD_HANDLED)
{
+
+ if (cmd->defer) {
+ /* The command is still stored as
ctrl_cmd_def.cmd, in the def_cmds list.
+* Just leave hanging for deferred handling.
Reply will happen later. */
+ return 0;
+ }
+
/* On CTRL_CMD_HANDLED, no reply needs to be sent back.
*/
talloc_free(cmd);
cmd = NULL;
diff --git a/tests/ctrl/ctrl_test.c b/tests/ctrl/ctrl_test.c
index f20f534..b7b30c3 100644
--- a/tests/ctrl/ctrl_test.c
+++ b/tests/ctrl/ctrl_test.c
@@ -401,8 +401,6 @@
/* Expecting a ctrl_cmd_def as well as the cmd to still be allocated */
if (talloc_total_size(ctx) <= ctx_size_before_defer) {
printf("deferred command apparently deallocated too soon\n");
- /* ERROR -- showing current bug in handling deallocated cmds,
hence exiting early */
- goto exit_early;
talloc_report_full(ctx, stdout);
OSMO_ASSERT(false);
}
@@ -416,8 +414,6 @@
talloc_report_full(ctx, stdout);
OSMO_ASSERT(false);
}
-
-exit_early:
talloc_free(ccon);
talloc_free(ctrl);
diff --git a/tests/ctrl/ctrl_test.ok b/tests/ctrl/ctrl_test.ok
index 6a902be..07f4aac 100644
--- a/tests/ctrl/ctrl_test.ok
+++ b/tests/ctrl/ctrl_test.ok
@@ -176,5 +176,6 @@
test_deferred_cmd
get_test_defer called
ctrl_handle_msg() returned 0
-deferred command apparently deallocated too soon
+invoking ctrl_test_defer_cb() asynchronously
+ctrl_test_defer_cb called
success
--
To view, visit https://gerrit.osmocom.org/7618
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24232be7dcf7be79f4def91ddc8b8f8005b56318
Gerrit-PatchSet: 3
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
[PATCH] libosmocore[master]: ctrl: fix deferred commands (and hence fix osmo-bts-sysmo 'c...
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/7618
to look at the new patch set (#2).
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
---
M include/osmocom/ctrl/control_cmd.h
M src/ctrl/control_cmd.c
M src/ctrl/control_if.c
M tests/ctrl/ctrl_test.c
M tests/ctrl/ctrl_test.ok
5 files changed, 14 insertions(+), 5 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/18/7618/2
diff --git a/include/osmocom/ctrl/control_cmd.h
b/include/osmocom/ctrl/control_cmd.h
index 865b006..a5df753 100644
--- a/include/osmocom/ctrl/control_cmd.h
+++ b/include/osmocom/ctrl/control_cmd.h
@@ -56,6 +56,8 @@
struct llist_head def_cmds;
};
+struct ctrl_cmd_def;
+
struct ctrl_cmd {
struct ctrl_connection *ccon;
enum ctrl_type type;
@@ -64,6 +66,7 @@
char *variable;
char *value;
char *reply;
+ struct ctrl_cmd_def *defer;
};
#define ctrl_cmd_reply_printf(cmd, fmt, args ...) \
diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c
index c747e84..fb0cd2b 100644
--- a/src/ctrl/control_cmd.c
+++ b/src/ctrl/control_cmd.c
@@ -566,6 +566,7 @@
cd = talloc_zero(ctx, struct ctrl_cmd_def);
+ cmd->defer = cd;
cd->cmd = cmd;
cd->data = data;
diff --git a/src/ctrl/control_if.c b/src/ctrl/control_if.c
index 07de0d4..df8abbc 100644
--- a/src/ctrl/control_if.c
+++ b/src/ctrl/control_if.c
@@ -401,6 +401,13 @@
if (cmd->type != CTRL_TYPE_ERROR) {
cmd->ccon = ccon;
if (ctrl_cmd_handle(ctrl, cmd, ctrl->data) == CTRL_CMD_HANDLED)
{
+
+ if (cmd->defer) {
+ /* The command is still stored as
ctrl_cmd_def.cmd, in the def_cmds list.
+* Just leave hanging for deferred handling.
Reply will happen later. */
+ return 0;
+ }
+
/* On CTRL_CMD_HANDLED, no reply needs to be sent back.
*/
talloc_free(cmd);
cmd = NULL;
diff --git a/tests/ctrl/ctrl_test.c b/tests/ctrl/ctrl_test.c
index 86c6a78..0c55bf0 100644
--- a/tests/ctrl/ctrl_test.c
+++ b/tests/ctrl/ctrl_test.c
@@ -402,8 +402,6 @@
/* Expecting a ctrl_cmd_def as well as the cmd to still be allocated */
if (talloc_total_size(ctx) <= ctx_size_before_defer) {
printf("deferred command apparently deallocated too soon\n");
- /* ERROR -- showing current bug in handling deallocated cmds,
hence exiting early */
- goto exit_early;
talloc_report_full(ctx, stdout);
OSMO_ASSERT(false);
}
@@ -419,8 +417,6 @@
}
printf("success\n");
-
-exit_early:
talloc_free(ccon);
talloc_free(ctrl);
diff --git a/tests/ctrl/ctrl_test.ok b/tests/ctrl/ctrl_test.ok
index 2af708a..07f4aac 100644
--- a/tests/ctrl/ctrl_test.ok
+++ b/tests/ctrl/ctrl_test.ok
@@ -176,4 +176,6 @@
test_deferred_cmd
get_test_defer called
ctrl_handle_msg() returned 0
-deferred command apparently deallocated too soon
+invoking ctrl_test_defer_cb() asynchronously
+ctrl_test_defer_cb called
+success
--
To view, visit https://gerrit.osmocom.org/7618
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I24232be7dcf7be79f4def91ddc8b8f8005b56318
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr
Gerrit-Reviewer: Harald Welte
Gerrit-Reviewer: Jenkins Builder
[PATCH] libosmocore[master]: ctrl: fix deferred commands (and hence fix osmo-bts-sysmo 'c...
Review at https://gerrit.osmocom.org/7618
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.)
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
---
M include/osmocom/ctrl/control_cmd.h
M src/ctrl/control_cmd.c
M src/ctrl/control_if.c
3 files changed, 11 insertions(+), 0 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/18/7618/1
diff --git a/include/osmocom/ctrl/control_cmd.h
b/include/osmocom/ctrl/control_cmd.h
index 865b006..a5df753 100644
--- a/include/osmocom/ctrl/control_cmd.h
+++ b/include/osmocom/ctrl/control_cmd.h
@@ -56,6 +56,8 @@
struct llist_head def_cmds;
};
+struct ctrl_cmd_def;
+
struct ctrl_cmd {
struct ctrl_connection *ccon;
enum ctrl_type type;
@@ -64,6 +66,7 @@
char *variable;
char *value;
char *reply;
+ struct ctrl_cmd_def *defer;
};
#define ctrl_cmd_reply_printf(cmd, fmt, args ...) \
diff --git a/src/ctrl/control_cmd.c b/src/ctrl/control_cmd.c
index c747e84..fb0cd2b 100644
--- a/src/ctrl/control_cmd.c
+++ b/src/ctrl/control_cmd.c
@@ -566,6 +566,7 @@
cd = talloc_zero(ctx, struct ctrl_cmd_def);
+ cmd->defer = cd;
cd->cmd = cmd;
cd->data = data;
diff --git a/src/ctrl/control_if.c b/src/ctrl/control_if.c
index 07de0d4..df8abbc 100644
--- a/src/ctrl/control_if.c
+++ b/src/ctrl/control_if.c
@@ -401,6 +401,13 @@
if (cmd->type != CTRL_TYPE_ERROR) {
cmd->ccon = ccon;
if (ctrl_cmd_handle(ctrl, cmd, ctrl->data) == CTRL_CMD_HANDLED)
{
+
+ if (cmd->defer) {
+ /* The command is still stored as
ctrl_cmd_def.cmd, in the def_cmds list.
+* Just leave hanging for deferred handling.
Reply will happen later. */
+ return 0;
+ }
+
/* On CTRL_CMD_HANDLED, no reply needs to be sent back.
*/
talloc_free(cmd);
cmd = NULL;
--
To view, visit https://gerrit.osmocom.org/7618
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I24232be7dcf7be79f4def91ddc8b8f8005b56318
Gerrit-PatchSet: 1
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr
