diff mbox

ctrl: Introduce a macro for read-only attributes and use it

Message ID 1400145868-19050-1-git-send-email-holger@freyther.de
State Superseded
Headers show

Commit Message

Holger Freyther May 15, 2014, 9:24 a.m. UTC 
From: Holger Hans Peter Freyther <holger@moiji-mobile.com>

Certain attributes are read-only. Add a macro to make it more
easy to define those.
---
 openbsc/include/openbsc/control_cmd.h | 20 ++++++++++++++++++
 openbsc/src/osmo-bsc/osmo_bsc_ctrl.c  | 38 +++--------------------------------
 2 files changed, 23 insertions(+), 35 deletions(-)

Comments

Jacob Erlbeck May 15, 2014, 12:58 p.m. UTC | #1 
That's much better that adding dummy functions manually.

On 15.05.2014 11:24, Holger Freyther wrote:
> From: Holger Hans Peter Freyther <holger@moiji-mobile.com>
> 
> Certain attributes are read-only. Add a macro to make it more
> easy to define those.
> ---
> --- a/openbsc/include/openbsc/control_cmd.h
> +++ b/openbsc/include/openbsc/control_cmd.h
> +#define CTRL_CMD_DEFINE_RO(cmdname, cmdstr) \
> +static int get_##cmdname(struct ctrl_cmd *cmd, void *data);		\
> +static inline int set_##cmdname(struct ctrl_cmd *cmd, void *data)	\

Why inline? We basically need the addresses of these functions.

> +{									\
> +	cmd->reply = "Read Only attribute";				\
> +	return CTRL_CMD_ERROR;						\
> +}									\
> +static inline int verify_##cmdname(struct ctrl_cmd *cmd, const char *value, void *data) \
> +{									\
> +	cmd->reply = "Read Only attribute";				\
> +	return 1;							\
> +}									\
> +static struct ctrl_cmd_element cmd_##cmdname = { \
> +	.name = cmdstr, \
> +	.param = NULL, \
> +	.get = &get_##cmdname, \
> +	.set = &set_##cmdname, \
> +	.verify = &verify_##cmdname, \
> +}

It's quite some code duplication. What about something like the
following (even if the resulting 'forward' declarations are somewhat
senseless). This would even work with the ';' in the 'calling' code:

#define CTRL_CMD_DEFINE_RO(cmdname, cmdstr) \
static int set_##cmdname(struct ctrl_cmd *cmd, void *data)	\
{								\
	cmd->reply = "Read Only attribute";			\
	return CTRL_CMD_ERROR;					\
}									\
static int verify_##cmdname(struct ctrl_cmd *cmd, const char *value,
void *data) \
{								\
	cmd->reply = "Read Only attribute";			\
	return 1;						\
}								\
CTRL_CMD_DEFINE(cmdname, cmdstr)


Jacob
Holger Freyther May 15, 2014, 1:58 p.m. UTC | #2 
On Thu, May 15, 2014 at 02:58:28PM +0200, Jacob Erlbeck wrote:

> Why inline? We basically need the addresses of these functions.

Like it is done for most methods defined inside a header file. But
you are right, we do not need the inline here.

> It's quite some code duplication. What about something like the
> following (even if the resulting 'forward' declarations are somewhat
> senseless). This would even work with the ';' in the 'calling' code:
> 
> #define CTRL_CMD_DEFINE_RO(cmdname, cmdstr) \
...

> CTRL_CMD_DEFINE(cmdname, cmdstr)

Good idea, I have done that and cleaned it up a bit further.
diff mbox

Patch

diff --git a/openbsc/include/openbsc/control_cmd.h b/openbsc/include/openbsc/control_cmd.h
index 8aede15..df5ae23 100644
--- a/openbsc/include/openbsc/control_cmd.h
+++ b/openbsc/include/openbsc/control_cmd.h
@@ -172,6 +172,26 @@  static struct ctrl_cmd_element cmd_##cmdname = { \
 	.verify = &verify_##cmdname, \
 }
 
+#define CTRL_CMD_DEFINE_RO(cmdname, cmdstr) \
+static int get_##cmdname(struct ctrl_cmd *cmd, void *data);		\
+static inline int set_##cmdname(struct ctrl_cmd *cmd, void *data)	\
+{									\
+	cmd->reply = "Read Only attribute";				\
+	return CTRL_CMD_ERROR;						\
+}									\
+static inline int verify_##cmdname(struct ctrl_cmd *cmd, const char *value, void *data) \
+{									\
+	cmd->reply = "Read Only attribute";				\
+	return 1;							\
+}									\
+static struct ctrl_cmd_element cmd_##cmdname = { \
+	.name = cmdstr, \
+	.param = NULL, \
+	.get = &get_##cmdname, \
+	.set = &set_##cmdname, \
+	.verify = &verify_##cmdname, \
+}
+
 struct gsm_network;
 
 #endif /* _CONTROL_CMD_H */
diff --git a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c
index e32218d..d3593de 100644
--- a/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c
+++ b/openbsc/src/osmo-bsc/osmo_bsc_ctrl.c
@@ -58,7 +58,7 @@  void osmo_bsc_send_trap(struct ctrl_cmd *cmd, struct bsc_msc_connection *msc_con
 	talloc_free(trap);
 }
 
-CTRL_CMD_DEFINE(msc_connection_status, "msc_connection_status");
+CTRL_CMD_DEFINE_RO(msc_connection_status, "msc_connection_status");
 static int msc_connection_status = 0;
 
 static int get_msc_connection_status(struct ctrl_cmd *cmd, void *data)
@@ -70,17 +70,6 @@  static int get_msc_connection_status(struct ctrl_cmd *cmd, void *data)
 	return CTRL_CMD_REPLY;
 }
 
-static int set_msc_connection_status(struct ctrl_cmd *cmd, void *data)
-{
-	return CTRL_CMD_ERROR;
-}
-
-static int verify_msc_connection_status(struct ctrl_cmd *cmd, const char *value, void *data)
-{
-	cmd->reply = "Read-only property";
-	return 1;
-}
-
 static int msc_connection_status_trap_cb(unsigned int subsys, unsigned int signal, void *handler_data, void *signal_data)
 {
 	struct ctrl_cmd *cmd;
@@ -114,7 +103,7 @@  static int msc_connection_status_trap_cb(unsigned int subsys, unsigned int signa
 	return 0;
 }
 
-CTRL_CMD_DEFINE(bts_connection_status, "bts_connection_status");
+CTRL_CMD_DEFINE_RO(bts_connection_status, "bts_connection_status");
 static int bts_connection_status = 0;
 
 static int get_bts_connection_status(struct ctrl_cmd *cmd, void *data)
@@ -126,17 +115,6 @@  static int get_bts_connection_status(struct ctrl_cmd *cmd, void *data)
 	return CTRL_CMD_REPLY;
 }
 
-static int set_bts_connection_status(struct ctrl_cmd *cmd, void *data)
-{
-	return CTRL_CMD_ERROR;
-}
-
-static int verify_bts_connection_status(struct ctrl_cmd *cmd, const char *value, void *data)
-{
-	cmd->reply = "Read-only property";
-	return 1;
-}
-
 static int bts_connection_status_trap_cb(unsigned int subsys, unsigned int signal, void *handler_data, void *signal_data)
 {
 	struct ctrl_cmd *cmd;
@@ -496,7 +474,7 @@  err:
 	return 1;
 }
 
-CTRL_CMD_DEFINE(bts_rf_state, "rf_state");
+CTRL_CMD_DEFINE_RO(bts_rf_state, "rf_state");
 static int get_bts_rf_state(struct ctrl_cmd *cmd, void *data)
 {
 	const char *oper, *admin, *policy;
@@ -520,16 +498,6 @@  static int get_bts_rf_state(struct ctrl_cmd *cmd, void *data)
 	return CTRL_CMD_REPLY;
 }
 
-static int set_bts_rf_state(struct ctrl_cmd *cmd, void *data)
-{
-	cmd->reply = "set is unimplemented";
-	return CTRL_CMD_ERROR;
-}
-
-static int verify_bts_rf_state(struct ctrl_cmd *cmd, const char *value, void *data)
-{
-	return 0;
-}
 
 CTRL_CMD_DEFINE(net_rf_lock, "rf_locked");
 static int get_net_rf_lock(struct ctrl_cmd *cmd, void *data)