diff mbox

[5/6] gsup/oap: add OAP to GSUP client.

Message ID 1444643858-15149-6-git-send-email-nhofmeyr@sysmocom.de
State Accepted
Headers show

Commit Message

Neels Hofmeyr Oct. 12, 2015, 9:57 a.m. UTC 
Trigger an OAP registration upon IPA connect. Feed incoming OAP messages to
oap_handle() and send replies returned by it.

Add oap_config to sgsn_config (todo: vty).

Sponsored-by: On-Waves ehf
---
 openbsc/include/openbsc/gprs_gsup_client.h |  7 +++-
 openbsc/include/openbsc/sgsn.h             |  3 ++
 openbsc/src/gprs/gprs_gsup_client.c        | 58 ++++++++++++++++++++++++++----
 openbsc/src/gprs/gprs_subscriber.c         |  3 +-
 openbsc/tests/sgsn/Makefile.am             |  2 ++
 5 files changed, 65 insertions(+), 8 deletions(-)

Comments

Holger Freyther Nov. 2, 2015, 10:33 a.m. UTC | #1 
> On 12 Oct 2015, at 11:57, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:

Dear Neels,


> diff --git a/openbsc/src/gprs/gprs_gsup_client.c b/openbsc/src/gprs/gprs_gsup_client.c
> index 1f9e34c..a332fa7 100644
> --- a/openbsc/src/gprs/gprs_gsup_client.c
> +++ b/openbsc/src/gprs/gprs_gsup_client.c


> 
> static int gsup_client_read_cb(struct ipa_client_conn *link, struct msgb *msg)
> {
> 
> -	OSMO_ASSERT(gsupc->read_cb != NULL);
> -	gsupc->read_cb(gsupc, msg);
> +	if (he->proto == IPAC_PROTO_EXT_GSUP) {
> +		OSMO_ASSERT(gsupc->read_cb != NULL);
> +		gsupc->read_cb(gsupc, msg);
> +		/* expecting read_cb() to free msg */
> +	}
> +	else
> +	if (he->proto == IPAC_PROTO_EXT_OAP) {
> +		return gsup_client_oap_handle(gsupc, msg);
> +		/* gsup_client_oap_handle frees msg */
> +	}
> +	else
> +		goto invalid;

the coding style would not have else and if on two different lines. I will fix this myself
right now.

cheers
	holger
Neels Hofmeyr Nov. 2, 2015, 2:06 p.m. UTC | #2 
On Mon, Nov 02, 2015 at 11:33:50AM +0100, Holger Freyther wrote:
> > static int gsup_client_read_cb(struct ipa_client_conn *link, struct msgb *msg)
> > {
> > 
> > -	OSMO_ASSERT(gsupc->read_cb != NULL);
> > -	gsupc->read_cb(gsupc, msg);
> > +	if (he->proto == IPAC_PROTO_EXT_GSUP) {
> > +		OSMO_ASSERT(gsupc->read_cb != NULL);
> > +		gsupc->read_cb(gsupc, msg);
> > +		/* expecting read_cb() to free msg */
> > +	}
> > +	else
> > +	if (he->proto == IPAC_PROTO_EXT_OAP) {
> > +		return gsup_client_oap_handle(gsupc, msg);
> > +		/* gsup_client_oap_handle frees msg */
> > +	}
> > +	else
> > +		goto invalid;
> 
> the coding style would not have else and if on two different lines. I will fix this myself
> right now.

Yes, indeed. I'm doing that on purpose... the logical idea is that the if
conditions all start on the same column.

That's my personal dialect where I'm puzzled that no-one else seems to do
it this way. IMHO it's the only way to do it nicely ;)

that said ... do I really have to lose that bit of personal dialect?

Admitted, in a bunch of code where I'm editing, I should always adhere to
the local style. But my question is also for the gtphub development. Sure
it's nice to have all styles match in osmo code ... I guess I need someone
to push me to write "else if" on the same line. Against the styles in
openggsn, this style deviation is harmless ... right?? ;)

~Neels
Neels Hofmeyr Nov. 2, 2015, 6:18 p.m. UTC | #3 
On Mon, Nov 02, 2015 at 03:06:29PM +0100, Neels Hofmeyr wrote:
> On Mon, Nov 02, 2015 at 11:33:50AM +0100, Holger Freyther wrote:
> > > static int gsup_client_read_cb(struct ipa_client_conn *link, struct msgb *msg)
> > > {
> > > 
> > > -	OSMO_ASSERT(gsupc->read_cb != NULL);
> > > -	gsupc->read_cb(gsupc, msg);
> > > +	if (he->proto == IPAC_PROTO_EXT_GSUP) {
> > > +		OSMO_ASSERT(gsupc->read_cb != NULL);
> > > +		gsupc->read_cb(gsupc, msg);
> > > +		/* expecting read_cb() to free msg */
> > > +	}
> > > +	else
> > > +	if (he->proto == IPAC_PROTO_EXT_OAP) {
> > > +		return gsup_client_oap_handle(gsupc, msg);
> > > +		/* gsup_client_oap_handle frees msg */
> > > +	}
> > > +	else
> > > +		goto invalid;
> > 
> > the coding style would not have else and if on two different lines. I will fix this myself
> > right now.
> 
> Yes, indeed. I'm doing that on purpose... the logical idea is that the if
> conditions all start on the same column.

Now that I'm actually looking at the commit, let's name it:

I wrote:

   if (a) {
           frizziply();
   }
   else 
   if (b) {
           frobnicate();
   }
   else
           goto invalid;

and you committed

   if (a) {
           frizziply();
   } else if (b) {
           frobnicate();
   } else
           goto invalid;

I personally find it mildly ugly, but let's avoid the discussion.

To reiterate, I'd like to know whether gtphub should/really must ;) be
changed to the latter style.

~Neels
Holger Freyther Nov. 2, 2015, 7:20 p.m. UTC | #4 
> On 02 Nov 2015, at 19:18, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> 
> 
> 
> I personally find it mildly ugly, but let's avoid the discussion.
> 
> To reiterate, I'd like to know whether gtphub should/really must ;) be
> changed to the latter style.

yes.
Neels Hofmeyr Nov. 4, 2015, 11:29 p.m. UTC | #5 
On Mon, Nov 02, 2015 at 08:20:39PM +0100, Holger Freyther wrote:
> 
> > On 02 Nov 2015, at 19:18, Neels Hofmeyr <nhofmeyr@sysmocom.de> wrote:
> > 
> > 
> > 
> > I personally find it mildly ugly, but let's avoid the discussion.
> > 
> > To reiterate, I'd like to know whether gtphub should/really must ;) be
> > changed to the latter style.
> 
> yes.

heh, was actually just four places in gtphub. And the script fixed five
others elsewhere while at it, sent in a separate patch mail.

~Neels
diff mbox

Patch

diff --git a/openbsc/include/openbsc/gprs_gsup_client.h b/openbsc/include/openbsc/gprs_gsup_client.h
index 9537db4..ccfa388 100644
--- a/openbsc/include/openbsc/gprs_gsup_client.h
+++ b/openbsc/include/openbsc/gprs_gsup_client.h
@@ -23,6 +23,8 @@ 
 
 #include <osmocom/core/timer.h>
 
+#include <openbsc/oap.h>
+
 #define GPRS_GSUP_RECONNECT_INTERVAL 10
 #define GPRS_GSUP_PING_INTERVAL 20
 
@@ -38,6 +40,8 @@  struct gprs_gsup_client {
 	gprs_gsup_read_cb_t	read_cb;
 	void			*data;
 
+	struct oap_state	oap_state;
+
 	struct osmo_timer_list	ping_timer;
 	struct osmo_timer_list	connect_timer;
 	int			is_connected;
@@ -46,7 +50,8 @@  struct gprs_gsup_client {
 
 struct gprs_gsup_client *gprs_gsup_client_create(const char *ip_addr,
 						 unsigned int tcp_port,
-						 gprs_gsup_read_cb_t read_cb);
+						 gprs_gsup_read_cb_t read_cb,
+						 struct oap_config *oap_config);
 
 void gprs_gsup_client_destroy(struct gprs_gsup_client *gsupc);
 int gprs_gsup_client_send(struct gprs_gsup_client *gsupc, struct msgb *msg);
diff --git a/openbsc/include/openbsc/sgsn.h b/openbsc/include/openbsc/sgsn.h
index d4f9913..2b1b97c 100644
--- a/openbsc/include/openbsc/sgsn.h
+++ b/openbsc/include/openbsc/sgsn.h
@@ -6,6 +6,7 @@ 
 
 #include <osmocom/gprs/gprs_ns.h>
 #include <openbsc/gprs_sgsn.h>
+#include <openbsc/oap.h>
 
 #include <ares.h>
 
@@ -61,6 +62,8 @@  struct sgsn_config {
 	} timers;
 
 	int dynamic_lookup;
+
+	struct oap_config oap;
 };
 
 struct sgsn_instance {
diff --git a/openbsc/src/gprs/gprs_gsup_client.c b/openbsc/src/gprs/gprs_gsup_client.c
index 1f9e34c..a332fa7 100644
--- a/openbsc/src/gprs/gprs_gsup_client.c
+++ b/openbsc/src/gprs/gprs_gsup_client.c
@@ -108,6 +108,20 @@  static void gsup_client_send(struct gprs_gsup_client *gsupc, int proto_ext, stru
 	/* msg_tx is now queued and will be freed. */
 }
 
+static void gsup_client_oap_register(struct gprs_gsup_client *gsupc)
+{
+	struct msgb *msg_tx;
+	int rc;
+	rc = oap_register(&gsupc->oap_state, &msg_tx);
+
+	if ((rc < 0) || (!msg_tx)) {
+		LOGP(DGPRS, LOGL_ERROR, "GSUP OAP set up, but cannot register.\n");
+		return;
+	}
+
+	gsup_client_send(gsupc, IPAC_PROTO_EXT_OAP, msg_tx);
+}
+
 static void gsup_client_updown_cb(struct ipa_client_conn *link, int up)
 {
 	struct gprs_gsup_client *gsupc = link->data;
@@ -120,6 +134,9 @@  static void gsup_client_updown_cb(struct ipa_client_conn *link, int up)
 	if (up) {
 		start_test_procedure(gsupc);
 
+		if (gsupc->oap_state.state == OAP_INITIALIZED)
+			gsup_client_oap_register(gsupc);
+
 		osmo_timer_del(&gsupc->connect_timer);
 	} else {
 		osmo_timer_del(&gsupc->ping_timer);
@@ -129,6 +146,22 @@  static void gsup_client_updown_cb(struct ipa_client_conn *link, int up)
 	}
 }
 
+static int gsup_client_oap_handle(struct gprs_gsup_client *gsupc, struct msgb *msg_rx)
+{
+	int rc;
+	struct msgb *msg_tx;
+
+	rc = oap_handle(&gsupc->oap_state, msg_rx, &msg_tx);
+	msgb_free(msg_rx);
+	if (rc < 0)
+		return rc;
+
+	if (msg_tx)
+		gsup_client_send(gsupc, IPAC_PROTO_EXT_OAP, msg_tx);
+
+	return 0;
+}
+
 static int gsup_client_read_cb(struct ipa_client_conn *link, struct msgb *msg)
 {
 	struct ipaccess_head *hh = (struct ipaccess_head *) msg->data;
@@ -168,16 +201,24 @@  static int gsup_client_read_cb(struct ipa_client_conn *link, struct msgb *msg)
 	if (hh->proto != IPAC_PROTO_OSMO)
 		goto invalid;
 
-	if (!he || msgb_l2len(msg) < sizeof(*he) ||
-	    he->proto != IPAC_PROTO_EXT_GSUP)
+	if (!he || msgb_l2len(msg) < sizeof(*he))
 		goto invalid;
 
 	msg->l2h = &he->data[0];
 
-	OSMO_ASSERT(gsupc->read_cb != NULL);
-	gsupc->read_cb(gsupc, msg);
+	if (he->proto == IPAC_PROTO_EXT_GSUP) {
+		OSMO_ASSERT(gsupc->read_cb != NULL);
+		gsupc->read_cb(gsupc, msg);
+		/* expecting read_cb() to free msg */
+	}
+	else
+	if (he->proto == IPAC_PROTO_EXT_OAP) {
+		return gsup_client_oap_handle(gsupc, msg);
+		/* gsup_client_oap_handle frees msg */
+	}
+	else
+		goto invalid;
 
-	/* Not freeing msg here, because that must be done by the read_cb. */
 	return 0;
 
 invalid:
@@ -222,7 +263,8 @@  static void start_test_procedure(struct gprs_gsup_client *gsupc)
 
 struct gprs_gsup_client *gprs_gsup_client_create(const char *ip_addr,
 						 unsigned int tcp_port,
-						 gprs_gsup_read_cb_t read_cb)
+						 gprs_gsup_read_cb_t read_cb,
+						 struct oap_config *oap_config)
 {
 	struct gprs_gsup_client *gsupc;
 	int rc;
@@ -230,6 +272,10 @@  struct gprs_gsup_client *gprs_gsup_client_create(const char *ip_addr,
 	gsupc = talloc_zero(tall_bsc_ctx, struct gprs_gsup_client);
 	OSMO_ASSERT(gsupc);
 
+	rc = oap_init(oap_config, &gsupc->oap_state);
+	if (rc != 0)
+		goto failed;
+
 	gsupc->link = ipa_client_conn_create(gsupc,
 					     /* no e1inp */ NULL,
 					     0,
diff --git a/openbsc/src/gprs/gprs_subscriber.c b/openbsc/src/gprs/gprs_subscriber.c
index 8231e8c..3467293 100644
--- a/openbsc/src/gprs/gprs_subscriber.c
+++ b/openbsc/src/gprs/gprs_subscriber.c
@@ -63,7 +63,8 @@  int gprs_subscr_init(struct sgsn_instance *sgi)
 
 	sgi->gsup_client = gprs_gsup_client_create(
 		addr_str, sgi->cfg.gsup_server_port,
-		&gsup_read_cb);
+		&gsup_read_cb,
+		&sgi->cfg.oap);
 
 	if (!sgi->gsup_client)
 		return -1;
diff --git a/openbsc/tests/sgsn/Makefile.am b/openbsc/tests/sgsn/Makefile.am
index 3c202dd..24504c2 100644
--- a/openbsc/tests/sgsn/Makefile.am
+++ b/openbsc/tests/sgsn/Makefile.am
@@ -28,6 +28,8 @@  sgsn_test_LDADD = \
 	$(top_builddir)/src/gprs/gprs_utils.o \
 	$(top_builddir)/src/gprs/gprs_subscriber.o \
 	$(top_builddir)/src/gprs/gsm_04_08_gprs.o \
+	$(top_builddir)/src/gprs/oap.o \
+	$(top_builddir)/src/gprs/oap_messages.o \
 	$(top_builddir)/src/libcommon/libcommon.a \
 	$(LIBOSMOABIS_LIBS) \
 	$(LIBOSMOCORE_LIBS) \