osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 8: Code-Review+2

I assume we can merge it now ... otherwise I'll fix later

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 8
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[MERGED] osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: ctrl: completely replace all CTRL commands
..


ctrl: completely replace all CTRL commands

The previous commands are not conforming to how the CTRL interface is intended
to work:

  SET enable-ps 
  SET disable-ps 
  SET status-ps 

'status-ps' is a write-only command even though it returns the status.
'enable-ps' / 'disable-ps' indicate the value instead of a variable name of an
entity. The entity  takes the place of the variable value.

See also https://lists.osmocom.org/pipermail/openbsc/2017-September/011236.html

Instead, replace with

  SET subscriber.by-imsi-123456.ps-enabled {0,1}
  GET subscriber.by-imsi-123456.ps-enabled

and also provide further CTRL functions while at it:

  {SET,GET} subscriber.by-{imsi,msisdn,id}-123456.{cs,ps}-enabled {0,1}
  GET subscriber.by-{imsi,msisdn,id}-123456.{info,info-aud,info-all}

Provide CTRL tests in the form of transcripts.

Adjust tests/test_subscriber.sql to feature nonzero SQN, to see some values for
SQN in the CTRL transcript tests. (This does not affect the VTY tests, because
that creates its own subscribers, and there's no VTY command to set the SQN.)

This is the first time an application uses CTRL_NODE ids that are defined
outside of libosmocore, see 'Depends' below.

Implementation choice: the first idea was to have a '.' between the 'by-xxx'
and the value, like:

  subscriber.by-xxx.123456.function

but the difficulty with subscribers is that they are not in RAM, and I can't
just point node_data at a struct instance that is always there (like, say, a
global bts[0] struct in osmo-bsc). Instead, I want to store the selector and
later decide whether to read from the DB or whatever. With a '.' separating
things, the only way in a ctrl function to obtain both 'by-xxx' and '123456'
for picking a subscriber record would be to parse the entire variable path
string elements, including 'subscriber' and 'function', which would then also
clumsily fix at which node level we hook these commands; there could have been
separate CTRL_NODE_SUBSCR_BY_{IMSI,MSISDN,ID} parent nodes, but we cannot
introspect the current parent node dynamically within a ctrl function handler
(plus I'm not sure whether it's possible and a good idea to have the same
command under multiple parent nodes).

Rather than that, I store the 'by-foo-123' token in the node_data pointer to
have both bits of information pointed at by a single pointer; I use the
incoming command parsing to get this token pre-separated from surrounding node
names, and no need to re-allocate it, since the vector of tokens lives until
after command execution is complete. Each leaf command obtains this token from
cmd->node (aka node_data), and feeds this token to a common static function to
parse selector and value from it and to retrieve a subscriber record as needed.

(BTW, I have mentioned on the mailing list that this way might be necessary to
avoid numeric-only CTRL node names, but we don't need to, and that is not at
all related to this choice of structure.)

Depends: libosmocore I1bd62ae0d4eefde7e1517db15a2155640a1bab58
 libosmocore Ic9dba0e4a1eb5a7dc3cee2f181b9024ed4fc7005
Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
---
M src/ctrl.c
M src/ctrl.h
M tests/test_subscriber.ctrl
M tests/test_subscriber.sql
M tests/test_subscriber.vty
A tests/test_subscriber_errors.ctrl
6 files changed, 1,041 insertions(+), 73 deletions(-)

Approvals:
  Max: Looks good to me, but someone else must approve
  Neels Hofmeyr: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/ctrl.c b/src/ctrl.c
index b49765d..3e81661 100644
--- a/src/ctrl.c
+++ b/src/ctrl.c
@@ -21,77 +21,362 @@
  */
 
 #include 
+#include 
+#include 
+#include 
 
-#include 
-#include 
+#include 
 #include 
 
-#include "gsup_server.h"
-#include "logging.h"
-#include "db.h"
 #include "hlr.h"
-#include "luop.h"
 #include "ctrl.h"
+#include "db.h"
 
-static int handle_cmd_ps(struct hlr *ctx, struct ctrl_cmd *cmd, bool enable)
+#define SEL_BY "by-"
+#define SEL_BY_IMSI SEL_BY "imsi-"
+#define SEL_BY_MSISDN SEL_BY "msisdn-"
+#define SEL_BY_ID SEL_BY "id-"
+
+#define hexdump_buf(buf) osmo_hexdump_nospc((void*)buf, sizeof(buf))
+
+static bool startswith(const char *str, const char *start)
+{
+   return strncmp(str, start, strlen(start)) == 0;
+}
+
+static int _get_subscriber(struct db_context *dbc,
+  const char *by_selector,
+  struct hlr_subscriber *subscr)
+{
+   const char *val;
+   if (startswith(by_selector, SEL_BY_IMSI)) {
+   val = by_selector + strlen(SEL_BY_IMSI);
+   if (!osmo_imsi_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_imsi(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_MSISDN)) {
+   val = by_selector + 

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 8: Code-Review+1

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 8
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 7:

enough argued, here is your comment.

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 7
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Hello Max, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4311

to look at the new patch set (#7).

ctrl: completely replace all CTRL commands

The previous commands are not conforming to how the CTRL interface is intended
to work:

  SET enable-ps 
  SET disable-ps 
  SET status-ps 

'status-ps' is a write-only command even though it returns the status.
'enable-ps' / 'disable-ps' indicate the value instead of a variable name of an
entity. The entity  takes the place of the variable value.

See also https://lists.osmocom.org/pipermail/openbsc/2017-September/011236.html

Instead, replace with

  SET subscriber.by-imsi-123456.ps-enabled {0,1}
  GET subscriber.by-imsi-123456.ps-enabled

and also provide further CTRL functions while at it:

  {SET,GET} subscriber.by-{imsi,msisdn,id}-123456.{cs,ps}-enabled {0,1}
  GET subscriber.by-{imsi,msisdn,id}-123456.{info,info-aud,info-all}

Provide CTRL tests in the form of transcripts.

Adjust tests/test_subscriber.sql to feature nonzero SQN, to see some values for
SQN in the CTRL transcript tests. (This does not affect the VTY tests, because
that creates its own subscribers, and there's no VTY command to set the SQN.)

This is the first time an application uses CTRL_NODE ids that are defined
outside of libosmocore, see 'Depends' below.

Implementation choice: the first idea was to have a '.' between the 'by-xxx'
and the value, like:

  subscriber.by-xxx.123456.function

but the difficulty with subscribers is that they are not in RAM, and I can't
just point node_data at a struct instance that is always there (like, say, a
global bts[0] struct in osmo-bsc). Instead, I want to store the selector and
later decide whether to read from the DB or whatever. With a '.' separating
things, the only way in a ctrl function to obtain both 'by-xxx' and '123456'
for picking a subscriber record would be to parse the entire variable path
string elements, including 'subscriber' and 'function', which would then also
clumsily fix at which node level we hook these commands; there could have been
separate CTRL_NODE_SUBSCR_BY_{IMSI,MSISDN,ID} parent nodes, but we cannot
introspect the current parent node dynamically within a ctrl function handler
(plus I'm not sure whether it's possible and a good idea to have the same
command under multiple parent nodes).

Rather than that, I store the 'by-foo-123' token in the node_data pointer to
have both bits of information pointed at by a single pointer; I use the
incoming command parsing to get this token pre-separated from surrounding node
names, and no need to re-allocate it, since the vector of tokens lives until
after command execution is complete. Each leaf command obtains this token from
cmd->node (aka node_data), and feeds this token to a common static function to
parse selector and value from it and to retrieve a subscriber record as needed.

(BTW, I have mentioned on the mailing list that this way might be necessary to
avoid numeric-only CTRL node names, but we don't need to, and that is not at
all related to this choice of structure.)

Depends: libosmocore I1bd62ae0d4eefde7e1517db15a2155640a1bab58
 libosmocore Ic9dba0e4a1eb5a7dc3cee2f181b9024ed4fc7005
Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
---
M src/ctrl.c
M src/ctrl.h
M tests/test_subscriber.ctrl
M tests/test_subscriber.sql
M tests/test_subscriber.vty
A tests/test_subscriber_errors.ctrl
6 files changed, 1,041 insertions(+), 73 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/11/4311/7

diff --git a/src/ctrl.c b/src/ctrl.c
index b49765d..3e81661 100644
--- a/src/ctrl.c
+++ b/src/ctrl.c
@@ -21,77 +21,362 @@
  */
 
 #include 
+#include 
+#include 
+#include 
 
-#include 
-#include 
+#include 
 #include 
 
-#include "gsup_server.h"
-#include "logging.h"
-#include "db.h"
 #include "hlr.h"
-#include "luop.h"
 #include "ctrl.h"
+#include "db.h"
 
-static int handle_cmd_ps(struct hlr *ctx, struct ctrl_cmd *cmd, bool enable)
+#define SEL_BY "by-"
+#define SEL_BY_IMSI SEL_BY "imsi-"
+#define SEL_BY_MSISDN SEL_BY "msisdn-"
+#define SEL_BY_ID SEL_BY "id-"
+
+#define hexdump_buf(buf) osmo_hexdump_nospc((void*)buf, sizeof(buf))
+
+static bool startswith(const char *str, const char *start)
+{
+   return strncmp(str, start, strlen(start)) == 0;
+}
+
+static int _get_subscriber(struct db_context *dbc,
+  const char *by_selector,
+  struct hlr_subscriber *subscr)
+{
+   const char *val;
+   if (startswith(by_selector, SEL_BY_IMSI)) {
+   val = by_selector + strlen(SEL_BY_IMSI);
+   if (!osmo_imsi_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_imsi(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_MSISDN)) {
+   val = by_selector + strlen(SEL_BY_MSISDN);
+   if (!osmo_msisdn_str_valid(val))
+   return -EINVAL;

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 6:

(1 comment)

https://gerrit.osmocom.org/#/c/4311/6/src/ctrl.c
File src/ctrl.c:

Line 233:   aud3g.algo = OSMO_AUTH_ALG_NONE;
> I think it's worth putting this clarification right here next to the code r
ENOENT says it all: there is no such entry, and it's all over the db api docs 
(now it is). It goes on to print things, and each print exits if algo is none; 
but that's left up to the print function to decide. there is no mystery here at 
all.


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 6
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 6:

(1 comment)

https://gerrit.osmocom.org/#/c/4311/6/src/ctrl.c
File src/ctrl.c:

Line 233:   aud3g.algo = OSMO_AUTH_ALG_NONE;
> ah, I think that might have come from the VTY code where I used to print "2
I think it's worth putting this clarification right here next to the code 
rather than leaving it as gerrit comment only.


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 6
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 6:

see https://gerrit.osmocom.org/#/c/4415/

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 6
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 6:

(3 comments)

all in all I think this patch can stay as it is, but I need to fix the db API 
docs:

https://gerrit.osmocom.org/#/c/4311/6/src/ctrl.c
File src/ctrl.c:

Line 233:   aud3g.algo = OSMO_AUTH_ALG_NONE;
> I'm confused by this part. In case of ENOENT error we will continue with pr
ah, I think that might have come from the VTY code where I used to print "2G 
auth data: none" in some earlier state of the patches, so that I wanted to 
output absence and still want to call the printing functions below; doesn't 
apply here, that's right. Returning immediately is however just a minor 
optimization, really.

In general, ENOENT indicates that no auth data is present, which is a valid 
situation.


Line 258:   rc = db_get_auth_data(hlr->dbc, subscr.imsi, , , 
NULL);
> If someone later have to change db-get_auth_data() they got to make sure no
ENOENT rc is a concept used throughout the db API of osmo-hlr, it is imperative 
to not break that in case of changes. It is documented for all the db functions.

...at least I thought so, I see now that db_get_auth_data() was changed to the 
ENOENT rc but I forgot to adjust the API comment. Other functions also need 
fixing of their API doc, it should all read something like db_subscr_nam()'s 
doc:

 * Returns 0 on success, -ENOENT when the given IMSI does not exist, -EINVAL if
 * the SQL statement could not be composed, -ENOEXEC if running the SQL
 * statement failed, -EIO if the amount of rows modified is unexpected.


Line 262:   aud3g.algo = OSMO_AUTH_ALG_NONE;
> Same here. Maybe move it to separate inline function?
here it makes sense to continue. We gather data, then print it below. Note the 
print_subscr_info() that we still want to call even if no auth data is present. 
So I would keep this as is, so that ENOENT makes sure to mark auth data as not 
present and then go on to print whatever we got.

I think the two instances of rc evaluation don't justify effort to undup the 
code yet... but it would make sense in principle, agreed.


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 6
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 6: Code-Review-1

(3 comments)

https://gerrit.osmocom.org/#/c/4311/6/src/ctrl.c
File src/ctrl.c:

Line 233:   aud3g.algo = OSMO_AUTH_ALG_NONE;
I'm confused by this part. In case of ENOENT error we will continue with 
printing auth data to resp. but other errors will result in failure. If that's 
indeed the intention than it's worth placing a comment here which explains 
what's so special about ENOENT. Usually all the errors result in the same 
behavior.


Line 258:   rc = db_get_auth_data(hlr->dbc, subscr.imsi, , , 
NULL);
If someone later have to change db-get_auth_data() they got to make sure not to 
break this special ENOENT case. Maybe place a comment in there?


Line 262:   aud3g.algo = OSMO_AUTH_ALG_NONE;
Same here. Maybe move it to separate inline function?


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 6
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 6:

now needs G#4396

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 6
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Hello Max, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4311

to look at the new patch set (#6).

ctrl: completely replace all CTRL commands

The previous commands are not conforming to how the CTRL interface is intended
to work:

  SET enable-ps 
  SET disable-ps 
  SET status-ps 

'status-ps' is a write-only command even though it returns the status.
'enable-ps' / 'disable-ps' indicate the value instead of a variable name of an
entity. The entity  takes the place of the variable value.

See also https://lists.osmocom.org/pipermail/openbsc/2017-September/011236.html

Instead, replace with

  SET subscriber.by-imsi-123456.ps-enabled {0,1}
  GET subscriber.by-imsi-123456.ps-enabled

and also provide further CTRL functions while at it:

  {SET,GET} subscriber.by-{imsi,msisdn,id}-123456.{cs,ps}-enabled {0,1}
  GET subscriber.by-{imsi,msisdn,id}-123456.{info,info-aud,info-all}

Provide CTRL tests in the form of transcripts.

Adjust tests/test_subscriber.sql to feature nonzero SQN, to see some values for
SQN in the CTRL transcript tests. (This does not affect the VTY tests, because
that creates its own subscribers, and there's no VTY command to set the SQN.)

This is the first time an application uses CTRL_NODE ids that are defined
outside of libosmocore, see 'Depends' below.

Implementation choice: the first idea was to have a '.' between the 'by-xxx'
and the value, like:

  subscriber.by-xxx.123456.function

but the difficulty with subscribers is that they are not in RAM, and I can't
just point node_data at a struct instance that is always there (like, say, a
global bts[0] struct in osmo-bsc). Instead, I want to store the selector and
later decide whether to read from the DB or whatever. With a '.' separating
things, the only way in a ctrl function to obtain both 'by-xxx' and '123456'
for picking a subscriber record would be to parse the entire variable path
string elements, including 'subscriber' and 'function', which would then also
clumsily fix at which node level we hook these commands; there could have been
separate CTRL_NODE_SUBSCR_BY_{IMSI,MSISDN,ID} parent nodes, but we cannot
introspect the current parent node dynamically within a ctrl function handler
(plus I'm not sure whether it's possible and a good idea to have the same
command under multiple parent nodes).

Rather than that, I store the 'by-foo-123' token in the node_data pointer to
have both bits of information pointed at by a single pointer; I use the
incoming command parsing to get this token pre-separated from surrounding node
names, and no need to re-allocate it, since the vector of tokens lives until
after command execution is complete. Each leaf command obtains this token from
cmd->node (aka node_data), and feeds this token to a common static function to
parse selector and value from it and to retrieve a subscriber record as needed.

(BTW, I have mentioned on the mailing list that this way might be necessary to
avoid numeric-only CTRL node names, but we don't need to, and that is not at
all related to this choice of structure.)

Depends: libosmocore I1bd62ae0d4eefde7e1517db15a2155640a1bab58
 libosmocore Ic9dba0e4a1eb5a7dc3cee2f181b9024ed4fc7005
Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
---
M src/ctrl.c
M src/ctrl.h
M tests/test_subscriber.ctrl
M tests/test_subscriber.sql
M tests/test_subscriber.vty
A tests/test_subscriber_errors.ctrl
6 files changed, 1,039 insertions(+), 73 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/11/4311/6

diff --git a/src/ctrl.c b/src/ctrl.c
index b49765d..9297eb8 100644
--- a/src/ctrl.c
+++ b/src/ctrl.c
@@ -21,77 +21,360 @@
  */
 
 #include 
+#include 
+#include 
+#include 
 
-#include 
-#include 
+#include 
 #include 
 
-#include "gsup_server.h"
-#include "logging.h"
-#include "db.h"
 #include "hlr.h"
-#include "luop.h"
 #include "ctrl.h"
+#include "db.h"
 
-static int handle_cmd_ps(struct hlr *ctx, struct ctrl_cmd *cmd, bool enable)
+#define SEL_BY "by-"
+#define SEL_BY_IMSI SEL_BY "imsi-"
+#define SEL_BY_MSISDN SEL_BY "msisdn-"
+#define SEL_BY_ID SEL_BY "id-"
+
+#define hexdump_buf(buf) osmo_hexdump_nospc((void*)buf, sizeof(buf))
+
+static bool startswith(const char *str, const char *start)
+{
+   return strncmp(str, start, strlen(start)) == 0;
+}
+
+static int _get_subscriber(struct db_context *dbc,
+  const char *by_selector,
+  struct hlr_subscriber *subscr)
+{
+   const char *val;
+   if (startswith(by_selector, SEL_BY_IMSI)) {
+   val = by_selector + strlen(SEL_BY_IMSI);
+   if (!osmo_imsi_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_imsi(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_MSISDN)) {
+   val = by_selector + strlen(SEL_BY_MSISDN);
+   if (!osmo_msisdn_str_valid(val))
+   return -EINVAL;

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 5:

(3 comments)

https://gerrit.osmocom.org/#/c/4311/5/src/ctrl.c
File src/ctrl.c:

Line 98:cmd->reply = NULL;
> Maybe it's better to return smth like "unknown/unexpected error"? It would 
the ctrl API will insert a generic error message for us, no need to invent a 
new one.

... but now that I look at it in control_if.c:300, it actually also logs a 
notice that something failed to set an error message, so after all you're right.


Line 389: _LAST_CTRL_NODE_HLR);
> So we not only replace the commands, we also change the way we setup and us
It is required to use ..._dynip2(), otherwise I would have to add the HLR 
CTRL_NODE enum values in libosmocore. You have a point about 
hlr_controlif_setup arguments changing though; and I have an unnecessary 
cosmetic change, too.


https://gerrit.osmocom.org/#/c/4311/5/src/ctrl.h
File src/ctrl.h:

Line 30:_LAST_CTRL_NODE_HLR
> Same as ctrl.c - shouldn't this belong to a separate patch? This have nothi
no, this one belongs in this patch, I am using these new nodes to provide the 
new ctrl commands.


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 5
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 5: Code-Review-1

(3 comments)

https://gerrit.osmocom.org/#/c/4311/5/src/ctrl.c
File src/ctrl.c:

Line 98:cmd->reply = NULL;
Maybe it's better to return smth like "unknown/unexpected error"? It would be 
more descriptive than empty message.


Line 389: _LAST_CTRL_NODE_HLR);
So we not only replace the commands, we also change the way we setup and use 
ctrl interface? I think it would be better to split "change interface" part 
into separate patch.


https://gerrit.osmocom.org/#/c/4311/5/src/ctrl.h
File src/ctrl.h:

Line 30:_LAST_CTRL_NODE_HLR
Same as ctrl.c - shouldn't this belong to a separate patch? This have nothing 
to do with actual ctrl commands change - would be the same with old commands 
too?


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 5
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

[PATCH] osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Hello Max, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4311

to look at the new patch set (#4).

ctrl: completely replace all CTRL commands

The previous commands are not conforming to how the CTRL interface is intended
to work:

  SET enable-ps 
  SET disable-ps 
  SET status-ps 

'status-ps' is a write-only command even though it returns the status.
'enable-ps' / 'disable-ps' indicate the value instead of a variable name of an
entity. The entity  takes the place of the variable value.

See also https://lists.osmocom.org/pipermail/openbsc/2017-September/011236.html

Instead, replace with

  SET subscriber.by-imsi-123456.ps-enabled {0,1}
  GET subscriber.by-imsi-123456.ps-enabled

and also provide further CTRL functions while at it:

  {SET,GET} subscriber.by-{imsi,msisdn,id}-123456.{cs,ps}-enabled {0,1}
  GET subscriber.by-{imsi,msisdn,id}-123456.{info,info-aud,info-all}

Provide CTRL tests in the form of transcripts.

This is the first time an application uses CTRL_NODE ids that are defined
outside of libosmocore, see 'Depends' below.

Implementation choice: the first idea was to have a '.' between the 'by-xxx'
and the value, like:

  subscriber.by-xxx.123456.function

but the difficulty with subscribers is that they are not in RAM, and I can't
just point node_data at a struct instance that is always there (like, say, a
global bts[0] struct in osmo-bsc). Instead, I want to store the selector and
later decide whether to read from the DB or whatever. With a '.' separating
things, the only way in a ctrl function to obtain both 'by-xxx' and '123456'
for picking a subscriber record would be to parse the entire variable path
string elements, including 'subscriber' and 'function', which would then also
clumsily fix at which node level we hook these commands; there could have been
separate CTRL_NODE_SUBSCR_BY_{IMSI,MSISDN,ID} parent nodes, but we cannot
introspect the current parent node dynamically within a ctrl function handler
(plus I'm not sure whether it's possible and a good idea to have the same
command under multiple parent nodes).

Rather than that, I store the 'by-foo-123' token in the node_data pointer to
have both bits of information pointed at by a single pointer; I use the
incoming command parsing to get this token pre-separated from surrounding node
names, and no need to re-allocate it, since the vector of tokens lives until
after command execution is complete. Each leaf command obtains this token from
cmd->node (aka node_data), and feeds this token to a common static function to
parse selector and value from it and to retrieve a subscriber record as needed.

(BTW, I have mentioned on the mailing list that this way might be necessary to
avoid numeric-only CTRL node names, but we don't need to, and that is not at
all related to this choice of structure.)

Depends: libosmocore I1bd62ae0d4eefde7e1517db15a2155640a1bab58
 libosmocore Ic9dba0e4a1eb5a7dc3cee2f181b9024ed4fc7005
Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
---
M src/ctrl.c
M src/ctrl.h
M src/hlr.c
M tests/test_subscriber.ctrl
M tests/test_subscriber.sql
M tests/test_subscriber.vty
A tests/test_subscriber_errors.ctrl
7 files changed, 1,045 insertions(+), 78 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/11/4311/4

diff --git a/src/ctrl.c b/src/ctrl.c
index 3bd4d8f..05466c8 100644
--- a/src/ctrl.c
+++ b/src/ctrl.c
@@ -21,87 +21,372 @@
  */
 
 #include 
+#include 
+#include 
+#include 
 
-#include 
-#include 
+#include 
 #include 
 
-#include "gsup_server.h"
-#include "logging.h"
-#include "db.h"
 #include "hlr.h"
-#include "luop.h"
 #include "ctrl.h"
+#include "db.h"
 
-static int handle_cmd_ps(struct hlr *ctx, struct ctrl_cmd *cmd, bool enable)
+#define SEL_BY "by-"
+#define SEL_BY_IMSI SEL_BY "imsi-"
+#define SEL_BY_MSISDN SEL_BY "msisdn-"
+#define SEL_BY_ID SEL_BY "id-"
+
+#define hexdump_buf(buf) osmo_hexdump_nospc((void*)buf, sizeof(buf))
+
+static bool startswith(const char *str, const char *start)
+{
+   return strncmp(str, start, strlen(start)) == 0;
+}
+
+static int _get_subscriber(struct db_context *dbc,
+  const char *by_selector,
+  struct hlr_subscriber *subscr)
+{
+   const char *val;
+   if (startswith(by_selector, SEL_BY_IMSI)) {
+   val = by_selector + strlen(SEL_BY_IMSI);
+   if (!osmo_imsi_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_imsi(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_MSISDN)) {
+   val = by_selector + strlen(SEL_BY_MSISDN);
+   if (!osmo_msisdn_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_msisdn(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_ID)) {
+   int64_t id;
+   char *endptr;
+   val = by_selector + 

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 3:

> I really dislike the idea of including dead code: it always raises
 > the questions for any person reading this code later on, it
 > silently bitrot and it doesn't serve any particular purpose.

you're right, changing it.

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 3
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 2:

> are not of interest to any osmo-hlr user yet

than what's the point of including it?

> CTRL command 'subscriber.by-*.info'

will it break if you just omit "#if 0"?

I really dislike the idea of including dead code: it always raises the 
questions for any person reading this code later on, it silently bitrot and it 
doesn't serve any particular purpose. If we need it to illustrate some point 
than it's better to include comment which illustrate this point with words. If 
it's there for completeness sake than uncomment it and let the caller ignore it.

-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 2
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Hello Max, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/4311

to look at the new patch set (#3).

ctrl: completely replace all CTRL commands

The previous commands are not conforming to how the CTRL interface is intended
to work:

  SET enable-ps 
  SET disable-ps 
  SET status-ps 

'status-ps' is a write-only command even though it returns the status.
'enable-ps' / 'disable-ps' indicate the value instead of a variable name of an
entity. The entity  takes the place of the variable value.

See also https://lists.osmocom.org/pipermail/openbsc/2017-September/011236.html

Instead, replace with

  SET subscriber.by-imsi-123456.ps-enabled {0,1}
  GET subscriber.by-imsi-123456.ps-enabled

and also provide further CTRL functions while at it:

  {SET,GET} subscriber.by-{imsi,msisdn,id}-123456.{cs,ps}-enabled {0,1}
  GET subscriber.by-{imsi,msisdn,id}-123456.{info,info-aud,info-all}

Provide CTRL tests in the form of transcripts.

This is the first time an application uses CTRL_NODE ids that are defined
outside of libosmocore, see 'Depends' below.

Implementation choice: the first idea was to have a '.' between the 'by-xxx'
and the value, like:

  subscriber.by-xxx.123456.function

but the difficulty with subscribers is that they are not in RAM, and I can't
just point node_data at a struct instance that is always there (like, say, a
global bts[0] struct in osmo-bsc). Instead, I want to store the selector and
later decide whether to read from the DB or whatever. With a '.' separating
things, the only way in a ctrl function to obtain both 'by-xxx' and '123456'
for picking a subscriber record would be to parse the entire variable path
string elements, including 'subscriber' and 'function', which would then also
clumsily fix at which node level we hook these commands; there could have been
separate CTRL_NODE_SUBSCR_BY_{IMSI,MSISDN,ID} parent nodes, but we cannot
introspect the current parent node dynamically within a ctrl function handler
(plus I'm not sure whether it's possible and a good idea to have the same
command under multiple parent nodes).

Rather than that, I store the 'by-foo-123' token in the node_data pointer to
have both bits of information pointed at by a single pointer; I use the
incoming command parsing to get this token pre-separated from surrounding node
names, and no need to re-allocate it, since the vector of tokens lives until
after command execution is complete. Each leaf command obtains this token from
cmd->node (aka node_data), and feeds this token to a common static function to
parse selector and value from it and to retrieve a subscriber record as needed.

(BTW, I have mentioned on the mailing list that this way might be necessary to
avoid numeric-only CTRL node names, but we don't need to, and that is not at
all related to this choice of structure.)

Depends: libosmocore I1bd62ae0d4eefde7e1517db15a2155640a1bab58
 libosmocore Ic9dba0e4a1eb5a7dc3cee2f181b9024ed4fc7005
Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
---
M src/ctrl.c
M src/ctrl.h
M src/hlr.c
M tests/test_subscriber.ctrl
M tests/test_subscriber.sql
M tests/test_subscriber.vty
A tests/test_subscriber_errors.ctrl
7 files changed, 871 insertions(+), 78 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/11/4311/3

diff --git a/src/ctrl.c b/src/ctrl.c
index 3bd4d8f..6140a25 100644
--- a/src/ctrl.c
+++ b/src/ctrl.c
@@ -21,87 +21,382 @@
  */
 
 #include 
+#include 
+#include 
+#include 
 
-#include 
-#include 
+#include 
 #include 
 
-#include "gsup_server.h"
-#include "logging.h"
-#include "db.h"
 #include "hlr.h"
-#include "luop.h"
 #include "ctrl.h"
+#include "db.h"
 
-static int handle_cmd_ps(struct hlr *ctx, struct ctrl_cmd *cmd, bool enable)
+#define SEL_BY "by-"
+#define SEL_BY_IMSI SEL_BY "imsi-"
+#define SEL_BY_MSISDN SEL_BY "msisdn-"
+#define SEL_BY_ID SEL_BY "id-"
+
+#define hexdump_buf(buf) osmo_hexdump_nospc((void*)buf, sizeof(buf))
+
+static bool startswith(const char *str, const char *start)
+{
+   return strncmp(str, start, strlen(start)) == 0;
+}
+
+static int _get_subscriber(struct db_context *dbc,
+  const char *by_selector,
+  struct hlr_subscriber *subscr)
+{
+   const char *val;
+   if (startswith(by_selector, SEL_BY_IMSI)) {
+   val = by_selector + strlen(SEL_BY_IMSI);
+   if (!osmo_imsi_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_imsi(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_MSISDN)) {
+   val = by_selector + strlen(SEL_BY_MSISDN);
+   if (!osmo_msisdn_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_msisdn(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_ID)) {
+   int64_t id;
+   char *endptr;
+   val = by_selector + 

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 2:

(1 comment)

https://gerrit.osmocom.org/#/c/4311/2/src/ctrl.c
File src/ctrl.c:

Line 156: /* not used yet */
> Erm, could you clarify? Who's using the other part of the same function?
This is one large printf (to avoid reallocating string memory), in the first 
#if 0 are the format strings and in below #if 0 are the arguments corresponding 
to the format strings of the first #if 0. I'm excluding these, because they are 
variables that already exist in the struct (modeled after HLR specs I presume) 
but are not of interest to any osmo-hlr user yet, because there is no code 
actually using these features. Does this answer your question?

Or did you mean who is using print_subscr_info()? The CTRL command 
'subscriber.by-*.info'.


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 2
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 2:

(2 comments)

https://gerrit.osmocom.org/#/c/4311/2/src/ctrl.c
File src/ctrl.c:

Line 119:   do { \
> would indeed; shall we move it now?
If we won't do it now it might be easily forgotten.


Line 156: /* not used yet */
> I first wrote this and then figured that no-one is using these values anywa
Erm, could you clarify? Who's using the other part of the same function?


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 2
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Patch Set 2:

(5 comments)

https://gerrit.osmocom.org/#/c/4311/2/src/ctrl.c
File src/ctrl.c:

Line 119:   do { \
> This looks pretty generic. Would it make sense to put it into libosmoctrl i
would indeed; shall we move it now?


Line 156: /* not used yet */
> Shouldn't it belong to separate patch?
I first wrote this and then figured that no-one is using these values anyway. 
Instead of deleting and having to figure it out again later, it would be nice 
to just keep it ready. ok with you?


Line 224:   
> whitespace
thx


Line 272:   
> again
thx again :)


Line 307:   
...


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 2
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Max]

Patch Set 2: Code-Review-1

(4 comments)

https://gerrit.osmocom.org/#/c/4311/2/src/ctrl.c
File src/ctrl.c:

Line 119:   do { \
This looks pretty generic. Would it make sense to put it into libosmoctrl 
instead?


Line 156: /* not used yet */
Shouldn't it belong to separate patch?


Line 224:   
whitespace


Line 272:   
again


-- 
To view, visit https://gerrit.osmocom.org/4311
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
Gerrit-PatchSet: 2
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Max 
Gerrit-HasComments: Yes

[PATCH] osmo-hlr[master]: ctrl: completely replace all CTRL commands

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/4311

ctrl: completely replace all CTRL commands

The previous commands are not conforming to how the CTRL interface is intended
to work:

  SET enable-ps 
  SET disable-ps 
  SET status-ps 

'status-ps' is a write-only command even though it returns the status.
'enable-ps' / 'disable-ps' indicate the value instead of a variable name of an
entity. The entity  takes the place of the variable value.

See also https://lists.osmocom.org/pipermail/openbsc/2017-September/011236.html

Instead, replace with

  SET subscriber.by-imsi-123456.ps-enabled {0,1}
  GET subscriber.by-imsi-123456.ps-enabled

and also provide further CTRL functions while at it:

  {SET,GET} subscriber.by-{imsi,msisdn,id}-123456.{cs,ps}-enabled {0,1}
  GET subscriber.by-{imsi,msisdn,id}-123456.{info,info-aud,info-all}

Provide CTRL tests in the form of transcripts.

This is the first time an application uses CTRL_NODE ids that are defined
outside of libosmocore, see 'Depends' below.

Implementation choice: the first idea was to have a '.' between the 'by-xxx'
and the value, like:

  subscriber.by-xxx.123456.function

but the difficulty with subscribers is that they are not in RAM, and I can't
just point node_data at a struct instance that is always there (like, say, a
global bts[0] struct in osmo-bsc). Instead, I want to store the selector and
later decide whether to read from the DB or whatever. With a '.' separating
things, the only way in a ctrl function to obtain both 'by-xxx' and '123456'
for picking a subscriber record would be to parse the entire variable path
string elements, including 'subscriber' and 'function', which would then also
clumsily fix at which node level we hook these commands; there could have been
separate CTRL_NODE_SUBSCR_BY_{IMSI,MSISDN,ID} parent nodes, but we cannot
introspect the current parent node dynamically within a ctrl function handler
(plus I'm not sure whether it's possible and a good idea to have the same
command under multiple parent nodes).

Rather than that, I store the 'by-foo-123' token in the node_data pointer to
have both bits of information pointed at by a single pointer; I use the
incoming command parsing to get this token pre-separated from surrounding node
names, and no need to re-allocate it, since the vector of tokens lives until
after command execution is complete. Each leaf command obtains this token from
cmd->node (aka node_data), and feeds this token to a common static function to
parse selector and value from it and to retrieve a subscriber record as needed.

(BTW, I have mentioned on the mailing list that this way might be necessary to
avoid numeric-only CTRL node names, but we don't need to, and that is not at
all related to this choice of structure.)

Depends: libosmocore I1bd62ae0d4eefde7e1517db15a2155640a1bab58
Change-Id: I98ee6a06b3aa6a67adb868e0b63b0e04eb42eb50
---
M src/ctrl.c
M src/ctrl.h
M src/hlr.c
M tests/test_subscriber.ctrl
M tests/test_subscriber.sql
A tests/test_subscriber_errors.ctrl
6 files changed, 874 insertions(+), 74 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-hlr refs/changes/11/4311/1

diff --git a/src/ctrl.c b/src/ctrl.c
index 3bd4d8f..a0dbc2f 100644
--- a/src/ctrl.c
+++ b/src/ctrl.c
@@ -21,87 +21,390 @@
  */
 
 #include 
+#include 
+#include 
+#include 
 
-#include 
-#include 
+#include 
 #include 
 
-#include "gsup_server.h"
-#include "logging.h"
-#include "db.h"
 #include "hlr.h"
-#include "luop.h"
 #include "ctrl.h"
+#include "db.h"
 
-static int handle_cmd_ps(struct hlr *ctx, struct ctrl_cmd *cmd, bool enable)
+#define SEL_BY "by-"
+#define SEL_BY_IMSI SEL_BY "imsi-"
+#define SEL_BY_MSISDN SEL_BY "msisdn-"
+#define SEL_BY_ID SEL_BY "id-"
+
+#define hexdump_buf(buf) osmo_hexdump_nospc((void*)buf, sizeof(buf))
+
+static bool startswith(const char *str, const char *start)
+{
+   return strncmp(str, start, strlen(start)) == 0;
+}
+
+static int _get_subscriber(struct db_context *dbc,
+  const char *by_selector,
+  struct hlr_subscriber *subscr)
+{
+   const char *val;
+   if (startswith(by_selector, SEL_BY_IMSI)) {
+   val = by_selector + strlen(SEL_BY_IMSI);
+   if (!osmo_imsi_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_imsi(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_MSISDN)) {
+   val = by_selector + strlen(SEL_BY_MSISDN);
+   if (!osmo_msisdn_str_valid(val))
+   return -EINVAL;
+   return db_subscr_get_by_msisdn(dbc, val, subscr);
+   }
+   if (startswith(by_selector, SEL_BY_ID)) {
+   int64_t id;
+   char *endptr;
+   val = by_selector + strlen(SEL_BY_ID);
+   if (*val == '+')
+   return -EINVAL;
+   errno = 0;
+   id = strtoll(val, , 10);
+   if (errno || *endptr)
+