| Message ID | 1400151854-2048-1-git-send-email-jerlbeck@sysmocom.de |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, May 15, 2014 at 01:04:14PM +0200, Jacob Erlbeck wrote: > Currently, if a CTRL method does not set the reply, an error is > logged ("cmd->reply has not been set"). It even complains when the > function implementing the command returns CTRL_CMD_HANDLED, where > a reply text is not needed. > > This patch changes the logging level from ERROR to NOTICE. The logging > is now only done, when the retry has not been set and the ^^^^^^ <- reply? > implementation returns either CTRL_CMD_ERROR or CTRL_CMD_REPLY. So > in these cases the reply field must be set. > - if (ret == CTRL_CMD_ERROR) > + if (ret == CTRL_CMD_ERROR) { > cmd->reply = "An error has occured."; > - else > + LOGP(DCTRL, LOGL_NOTICE, > + "%s: cmd->reply has not been set (ERROR).\n", > + cmd->variable); > + } else if (ret == CTRL_CMD_REPLY) { > + LOGP(DCTRL, LOGL_NOTICE, > + "%s: cmd->reply has not been set (type = %d).\n", > + cmd->variable, cmd->type); > + cmd->reply = ""; > + } else { > cmd->reply = "Command has been handled."; Is using a switch/case better here? So for CTRL_CMD_HANDLED the "Command has been handled" will be set?
On 15.05.2014 21:24, Holger Hans Peter Freyther wrote: > On Thu, May 15, 2014 at 01:04:14PM +0200, Jacob Erlbeck wrote: >> Currently, if a CTRL method does not set the reply, an error is >> logged ("cmd->reply has not been set"). It even complains when the >> function implementing the command returns CTRL_CMD_HANDLED, where >> a reply text is not needed. >> >> This patch changes the logging level from ERROR to NOTICE. The logging >> is now only done, when the retry has not been set and the > > ^^^^^^ <- reply? Yes, of course, thanks. > >> implementation returns either CTRL_CMD_ERROR or CTRL_CMD_REPLY. So >> in these cases the reply field must be set. > >> - if (ret == CTRL_CMD_ERROR) >> + if (ret == CTRL_CMD_ERROR) { >> cmd->reply = "An error has occured."; >> - else >> + LOGP(DCTRL, LOGL_NOTICE, >> + "%s: cmd->reply has not been set (ERROR).\n", >> + cmd->variable); >> + } else if (ret == CTRL_CMD_REPLY) { >> + LOGP(DCTRL, LOGL_NOTICE, >> + "%s: cmd->reply has not been set (type = %d).\n", >> + cmd->variable, cmd->type); >> + cmd->reply = ""; >> + } else { >> cmd->reply = "Command has been handled."; > > Is using a switch/case better here? So for CTRL_CMD_HANDLED the > "Command has been handled" will be set? Yes, I'd like to use a switch here, too. But CTRL_CMD_* are not enums and CTRL_CMD_ERROR is defined to -1, which I don't want to use as case constant. I'd rather turn CTRL_CMD_* into an enum and replace if's by switches in another patch. Jacob
diff --git a/openbsc/src/libbsc/bsc_ctrl_lookup.c b/openbsc/src/libbsc/bsc_ctrl_lookup.c index 338fb11..3a3df9c 100644 --- a/openbsc/src/libbsc/bsc_ctrl_lookup.c +++ b/openbsc/src/libbsc/bsc_ctrl_lookup.c @@ -150,11 +150,19 @@ 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, + "%s: cmd->reply has not been set (ERROR).\n", + cmd->variable); + } else if (ret == CTRL_CMD_REPLY) { + LOGP(DCTRL, LOGL_NOTICE, + "%s: cmd->reply has not been set (type = %d).\n", + cmd->variable, cmd->type); + cmd->reply = ""; + } else { cmd->reply = "Command has been handled."; + } } if (ret == CTRL_CMD_ERROR)
Currently, if a CTRL method does not set the reply, an error is logged ("cmd->reply has not been set"). It even complains when the function implementing the command returns CTRL_CMD_HANDLED, where a reply text is not needed. This patch changes the logging level from ERROR to NOTICE. The logging is now only done, when the retry has not been set and the implementation returns either CTRL_CMD_ERROR or CTRL_CMD_REPLY. So in these cases the reply field must be set. This fixes the generation of log messages when doing NAT ctrl command forwarding. Ticket: OW#1177 Sponsored-by: On-Waves ehf --- openbsc/src/libbsc/bsc_ctrl_lookup.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)