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") 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 ... -- To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/32512 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hlr Gerrit-Branch: master Gerrit-Change-Id: I540132ee5dcfd09f4816e02e702927e1074ca50f Gerrit-Change-Number: 32512 Gerrit-PatchSet: 7 Gerrit-Owner: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-CC: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-CC: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-CC: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Attention: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-Comment-Date: Mon, 15 May 2023 15:20:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt; Comment-In-Reply-To: daniel &lt;dwillmann(a)sysmocom.de&gt; Comment-In-Reply-To: lynxis lazus &lt;lynxis(a)fe80.eu&gt; Gerrit-MessageType: comment