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

View Change

9 comments:

• Commit Message:

  • Patch Set #7, 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?

  • Patch Set #7, Line 17: premium

    premium?

• Patchset:

  • Patch Set #7:

    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:

  • Patch Set #7, Line 2:

    #pragma once

  • Patch Set #7, Line 28:

    (extra blank line)

  • Patch Set #7, Line 38:

    (extra blank line)

• File src/gsup_server.c:

  • Patch Set #7, 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:

  • Patch Set #5, 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:

  • Patch Set #7, 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 change 32512. To unsubscribe, or for help writing mail filters, visit settings.