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)