| Message ID | 1399649136-15337-1-git-send-email-jerlbeck@sysmocom.de |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, May 09, 2014 at 05:25:36PM +0200, Jacob Erlbeck wrote: Good Morning, > Currently, if a CTRL method does not set the reply, an error is > logged ("cmd->reply has not been set"). hmm. I would like to see some more reflection here. The "reply" not being set being an ERROR was done by you, now the NAT might have a legetimate use-case for not setting a reply. I don't see this being reflected in the commit message. > if (!cmd->reply) { > - LOGP(DCTRL, LOGL_ERROR, "cmd->reply has not been set.\n"); > - if (ret == CTRL_CMD_ERROR) > + if (ret == CTRL_CMD_ERROR) { > cmd->reply = "An error has occured."; > - else > + LOGP(DCTRL, LOGL_NOTICE, > + "cmd->reply has not been set (ERROR).\n"); > + } else if (cmd->type == CTRL_TYPE_GET) { > + LOGP(DCTRL, LOGL_NOTICE, > + "cmd->reply has not been set (GET).\n"); > + cmd->reply = ""; > + } else > cmd->reply = "Command has been handled."; I am not convinced. The NAT will still generate these messages on the stdout but there is nothing to be fixed?! To make the actual log message more useful one should output the name of command. :) > static int set_msc_connection_status(struct ctrl_cmd *cmd, void *data) > { > + cmd->reply = "Set is unimplemented"; > return CTRL_CMD_ERROR; > } Hmm, "unimplemented" somehow indicates that we would know what to implement. E.g. if we take a look at verify_msc_connection_status I claim that "msc_connection_status" is just a read-only attribute. > > @@ -128,6 +129,7 @@ static int get_bts_connection_status(struct ctrl_cmd *cmd, void *data) > > static int set_bts_connection_status(struct ctrl_cmd *cmd, void *data) > { > + cmd->reply = "Set is unimplemented"; > return CTRL_CMD_ERROR; > } The same here. It is read-only. > > @@ -522,7 +524,7 @@ static int get_bts_rf_state(struct ctrl_cmd *cmd, void *data) > > static int set_bts_rf_state(struct ctrl_cmd *cmd, void *data) > { > - cmd->reply = "set is unimplemented"; > + cmd->reply = "Set is unimplemented"; > return CTRL_CMD_ERROR; > } It should probably be read-only. I will introduce a CTRL_CMD_DEFINE for read-only commands but the actual issue is still there. cheers holger
diff --git a/openbsc/src/libbsc/bsc_ctrl_lookup.c b/openbsc/src/libbsc/bsc_ctrl_lookup.c index 338fb11..dced8bd 100644 --- a/openbsc/src/libbsc/bsc_ctrl_lookup.c +++ b/openbsc/src/libbsc/bsc_ctrl_lookup.c @@ -150,10 +150,15 @@ int bsc_ctrl_cmd_handle(struct ctrl_cmd *cmd, void *data) err: if (!cmd->reply) { - LOGP(DCTRL, LOGL_ERROR, "cmd->reply has not been set.\n"); - if (ret == CTRL_CMD_ERROR) + if (ret == CTRL_CMD_ERROR) { cmd->reply = "An error has occured."; - else + LOGP(DCTRL, LOGL_NOTICE, + "cmd->reply has not been set (ERROR).\n"); + } else if (cmd->type == CTRL_TYPE_GET) { + LOGP(DCTRL, LOGL_NOTICE, + "cmd->reply has not been set (GET).\n"); + cmd->reply = ""; + } else cmd->reply = "Command has been handled."; } diff --git a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c index e32218d..3cc704b 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c @@ -72,6 +72,7 @@ static int get_msc_connection_status(struct ctrl_cmd *cmd, void *data) static int set_msc_connection_status(struct ctrl_cmd *cmd, void *data) { + cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR; } @@ -128,6 +129,7 @@ static int get_bts_connection_status(struct ctrl_cmd *cmd, void *data) static int set_bts_connection_status(struct ctrl_cmd *cmd, void *data) { + cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR; } @@ -522,7 +524,7 @@ static int get_bts_rf_state(struct ctrl_cmd *cmd, void *data) static int set_bts_rf_state(struct ctrl_cmd *cmd, void *data) { - cmd->reply = "set is unimplemented"; + cmd->reply = "Set is unimplemented"; return CTRL_CMD_ERROR; }
Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set"). This patch changes the logging level from ERROR to INFO. The logging is now only done, when the retry has not been set and the implementation returns CTRL_CMD_ERROR or the GET has been used. This means, every time a error is signalled or GET is used, the retry field shall be set. Some missing reply texts in error cases are also added. Ticket: OW#1177 Sponsored-by: On-Waves ehf --- openbsc/src/libbsc/bsc_ctrl_lookup.c | 11 ++++++++--- openbsc/src/osmo-bsc/osmo_bsc_ctrl.c | 4 +++- 2 files changed, 11 insertions(+), 4 deletions(-)