Attention is currently required from: laforge, daniel, lynxis lazus.

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/32512 )

Change subject: Add support for multiple APN profiles for subscriber data ......................................................................

Patch Set 7:

(9 comments)

Commit Message:

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/081a1a7a_ff75e2bd PS7, Line 11: This violates the spec spec reference please?

let's also mention where exactly: HLR User manual 13.7.3 PDP Info / Access Point Name right?

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/4fb978ea_15d56a20 PS7, Line 17: premium premium?

Patchset:

PS7: It would be really cool to see all new VTY commands tested in the *.vty transcript test, because that would also be a lot easier to understand and review than just reading the vty.c code.

File include/osmocom/hlr/hlr_ps.h:

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/b73e89ef_b08aff54 PS7, Line 2: #pragma once

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/d912453f_051fb209 PS7, Line 28: (extra blank line)

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/6c7e8b40_01b242dd PS7, Line 38: (extra blank line)

File src/gsup_server.c:

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/66e5226d_83778dd4 PS7, Line 480: if (g_hlr->ps.pdp_profile.enabled) { maybe include these checks somehow?

if (g_hlr->ps.pdp_profile.num_pdp_info)

OSMO_ASSERT(g_hlr->ps.pdp_profile.num_pdp_infos <= ARRAY_SIZE(g_hlr->ps.pdp_profile.pdp_infos));

Could 'pdp_profile.enabled' be dropped because num_pdp_info == 0 means the same?

File src/hlr_vty.c:

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/09328a10_c247f1aa PS5, Line 133: Define a PDP profile set.\n" : "Unique identifier for this PDP profile set.\n")

I think the confusion is that there's only a pdp-profiles "default", but the help for default states […]

In the future, this may look like this:

DEFUN(cfg_ps_pdp_profiles, cfg_ps_pdp_profiles_cmd, "pdp-profiles (default|NAME)", "Define a PDP profile set.\n" "Define the global default profile.\n" "Define a custom profile with this unique identifier.\n")

does that make sense? if yes, i would do this now:

DEFUN(cfg_ps_pdp_profiles, cfg_ps_pdp_profiles_cmd, "pdp-profiles default", "Define a PDP profile set.\n" "Define the global default profile.\n")

File tests/test_nodes.vty:

https://gerrit.osmocom.org/c/osmo-hlr/+/32512/comment/b50ea6ff_bd9aed01 PS7, Line 56: ps please also add tests for the command doc strings, to verify all new docs, like

# ps? # ps (ps)# list (ps)# pdp-profiles ?

etc

and also add tests for write-back like

# show running-config ... hlr ... ps pdp-profiles default ...