diff mbox

Export names for gsm_chan_t and gsm48_chan_mode

Message ID 1458816899-30771-1-git-send-email-msuraev@sysmocom.de
State Superseded
Headers show

Commit Message

Max March 24, 2016, 10:54 a.m. UTC
From: Max <msuraev@sysmocom.de>

This can be used with get_value_string() to improve debugging output.
---
 include/osmocom/gsm/gsm_utils.h          | 13 +++++++++++++
 include/osmocom/gsm/protocol/gsm_04_08.h | 14 ++++++++++++++
 2 files changed, 27 insertions(+)

Comments

Harald Welte March 27, 2016, 8:26 a.m. UTC | #1
On Thu, Mar 24, 2016 at 11:54:59AM +0100, msuraev@sysmocom.de wrote:
> This can be used with get_value_string() to improve debugging output.

Great, but...

> +static const struct value_string gsm_chan_t_names[] = {

a static symbol can certainly not be used by get_value_string.

Oh, you put it in the header file, no that's not how we do things,
sorry.  What you're doing means that every application that links the library
will end up having it's own copy of the data structures, somewhat defeating the
purpose of a shared library.  Data structures like this have to go into a .c
file, and you need a forward-declaration in the header.

So either you 
a) have the value_string non-static and exported in the symbol table, and
   directly add a forward-declaration of the 'const struct value_string' array
   like in th example of 'abis_nm_msg_disc_names' or
b) you have a static value_string array but an exported accessor functions and
   those functions are forward-declared in the header (like your original patch)

theoretically, there could also be a 'c' where the accessor function is an
inline function or a macro, as it basicall is just a one-liner.

Sorry,
	Harald
Neels Hofmeyr March 29, 2016, 10:05 a.m. UTC | #2
On Sun, Mar 27, 2016 at 10:26:53AM +0200, Harald Welte wrote:
> theoretically, there could also be a 'c' where the accessor function is an
> inline function or a macro, as it basicall is just a one-liner.

I tend to use static inline functions with a switch(){} (I recently
submitted a patch like that in "[PATCH 1/7] Add MM Auth test; add
auth_action_str() function").

Is the get_value_string() way preferred to a switch?

Thanks,
~Neels
Harald Welte March 29, 2016, 10:21 a.m. UTC | #3
On Tue, Mar 29, 2016 at 12:05:19PM +0200, Neels Hofmeyr wrote:
> On Sun, Mar 27, 2016 at 10:26:53AM +0200, Harald Welte wrote:
> > theoretically, there could also be a 'c' where the accessor function is an
> > inline function or a macro, as it basicall is just a one-liner.
> 
> I tend to use static inline functions with a switch(){} (I recently
> submitted a patch like that in "[PATCH 1/7] Add MM Auth test; add
> auth_action_str() function").

Yes, and AFAIR I already provided feedback about this particular issues.

> Is the get_value_string() way preferred to a switch?

yes, it is the general way how we map numbers to ascii strings and
vice-versa.  It is not as effcient as it iterates the list, but 
* as it is used for debug statements and from VTY, performance is not
  the utmost concern
* I prefer to have one way that all code uses over everyone inventing
  their own methods.  That helps in terms of reducing code duplication
  and also helps if we ever want to migrate to something more efficient
  (wireshark is the source of value_string, and they have a hashed
  version now, AFAIK).

Regards,
	Harald
diff mbox

Patch

diff --git a/include/osmocom/gsm/gsm_utils.h b/include/osmocom/gsm/gsm_utils.h
index 6458447..dcbee36 100644
--- a/include/osmocom/gsm/gsm_utils.h
+++ b/include/osmocom/gsm/gsm_utils.h
@@ -28,6 +28,7 @@ 
 #include <stdint.h>
 
 #include <osmocom/core/defs.h>
+#include <osmocom/core/utils.h>
 
 #define ADD_MODULO(sum, delta, modulo) do {	\
 	if ((sum += delta) >= modulo)		\
@@ -199,6 +200,18 @@  enum gsm_chan_t {
 	_GSM_LCHAN_MAX
 };
 
+static const struct value_string gsm_chan_t_names[] = {
+	{ GSM_LCHAN_NONE,	"NONE" },
+	{ GSM_LCHAN_SDCCH,	"SDCCH" },
+	{ GSM_LCHAN_TCH_F,	"TCH_F" },
+	{ GSM_LCHAN_TCH_H,	"TCH_H" },
+	{ GSM_LCHAN_UNKNOWN,	"UNKNOWN" },
+	{ GSM_LCHAN_CCCH,	"CCCH" },
+	{ GSM_LCHAN_PDTCH,	"PDTCH" },
+	{ GSM_LCHAN_CBCH,	"CBCH" },
+	{ 0,			NULL },
+};
+
 /* Deprectated functions */
 /* Limit encoding and decoding to use no more than this amount of buffer bytes */
 #define GSM_7BIT_LEGACY_MAX_BUFFER_SIZE  0x10000
diff --git a/include/osmocom/gsm/protocol/gsm_04_08.h b/include/osmocom/gsm/protocol/gsm_04_08.h
index d49b77f..604c86d 100644
--- a/include/osmocom/gsm/protocol/gsm_04_08.h
+++ b/include/osmocom/gsm/protocol/gsm_04_08.h
@@ -2,6 +2,8 @@ 
 
 #include <stdint.h>
 
+#include <osmocom/core/utils.h>
+
 /* GSM TS 04.08  definitions */
 struct gsm_lchan;
 
@@ -347,6 +349,18 @@  enum gsm48_chan_mode {
 	GSM48_CMODE_DATA_3k6	= 0x13,
 };
 
+static const struct value_string gsm48_chan_mode_names[] = {
+	{ GSM48_CMODE_SIGN,		"SIGNALLING" },
+	{ GSM48_CMODE_SPEECH_V1,	"SPEECH_V1" },
+	{ GSM48_CMODE_SPEECH_EFR,	"SPEECH_EFR" },
+	{ GSM48_CMODE_SPEECH_AMR,	"SPEECH_AMR" },
+	{ GSM48_CMODE_DATA_14k5,	"DATA_14k5" },
+	{ GSM48_CMODE_DATA_12k0,	"DATA_12k0" },
+	{ GSM48_CMODE_DATA_6k0,		"DATA_6k0" },
+	{ GSM48_CMODE_DATA_3k6,		"DATA_3k6" },
+	{ 0,				NULL },
+};
+
 /* Chapter 9.1.2 */
 struct gsm48_ass_cmd {
 	/* Semantic is from 10.5.2.5a */