January 2023 - gerrit-log - lists.osmocom.org

Change in osmo-upf[master]: manual: some tweaks in overview

by neels

neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-upf/+/30836 ) Change subject: manual: some tweaks in overview ...................................................................... manual: some tweaks in overview Change-Id: I5a672d24eb12bd29d8684117b2658ad4cd89d682 --- M doc/manuals/chapters/overview.adoc 1 file changed, 13 insertions(+), 10 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-upf refs/changes/36/30836/1 diff --git a/doc/manuals/chapters/overview.adoc b/doc/manuals/chapters/overview.adoc index fa01939..80d5982 100644 --- a/doc/manuals/chapters/overview.adoc +++ b/doc/manuals/chapters/overview.adoc @@ -117,11 +117,14 @@ Header Creation for GTP. - A second PDR+FAR pair like above, with Access and Core swapped. +Access and Core must be indicated by the Source Interface IE (PDR) and +Destination Interface IE (FAR) in PFCP. + Any set of rules only partially or not at all matching the above PDR and FAR rules will not result in any actions on the GTP user plane, but will still return a successful outcome in the PFCP messages. -For example, a rule set using an interface other than "Access" or "Core" results +For example, a rule set using a Source Interface other than "Access" or "Core" results in a PFCP no-op, returning PFCP responses with successful outcome, but not providing any GTP-U service. @@ -132,11 +135,11 @@ - OsmoUPF using Linux kernel features for the GTP user plane, where there is either a full bidirectional GTP tunnel in place or none at all. -For example, a typical CPF will first establish a PFCP session with the UPF -before passing on a data service request from Access to Core, because the UPF -must first choose the GTP TEID that needs to be forwarded towards Core. When the -Core side has responded with its GTP details, the UPF is updated, to form a -complete PFCP rule set. +For example, for `tunmap`, a typical CPF will establish a PFCP session in two +steps: first request a local F-TEID from the UPF before passing on a data +service request from Access to Core. When the Core side has responded with its +GTP details, the PFCP session at the UPF is updated (Session Modifification), +to form a complete PFCP rule set. .Typical sequence of establishing a GTP-U tunnel relay ["mscgen"] @@ -156,16 +159,16 @@ |||; - sgwc <= sgwu [label="PFCP Session Establishment Response\n\n2x Created PDR\nwith chosen F-TEID"]; + sgwc <= sgwu [label="PFCP Session Establishment Response\n\n2x Created PDR\nwith chosen local F-TEID"]; |||; - sgwc => pgwc [label="GTP Create Session Request"]; - sgwc <= pgwc [label="GTP Create Session Response"]; + sgwc => pgwc [label="GTP Create Session Request\nwith chosen local F-TEID towards Core"]; + sgwc <= pgwc [label="GTP Create Session Response\nwith remote F-TEID at Core"]; |||; - sgwc => sgwu [label="PFCP Session Modification Request\n\nUpdate FAR\nwith F-TEID from Core"]; + sgwc => sgwu [label="PFCP Session Modification Request\n\nUpdate FAR\nwith remote F-TEID at Core"]; |||; -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/30836 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: I5a672d24eb12bd29d8684117b2658ad4cd89d682 Gerrit-Change-Number: 30836 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: newchange

2 years, 5 months

1
0
0 0

Change in osmo-upf[master]: tunend: choose local GTP addr by Network Instance IEs

by neels

neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-upf/+/30831 ) Change subject: tunend: choose local GTP addr by Network Instance IEs ...................................................................... tunend: choose local GTP addr by Network Instance IEs Implement handling of the Network Instance IEs from PFCP for tunend, like already done for tunmap. In 'tunend' cfg, allow indicating a local GTP address for both 'dev create' and 'dev use'. Select a GTP device by the local address the Network Instance IE in PFCP PDR indicates. Change-Id: I376c09bfc1844df1e61d2efac17561fac614858b --- M include/osmocom/upf/upf_gtp.h M src/osmo-upf/up_gtp_action.c M src/osmo-upf/upf_gtp.c M src/osmo-upf/upf_vty.c M tests/upf.vty 5 files changed, 64 insertions(+), 27 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-upf refs/changes/31/30831/1 diff --git a/include/osmocom/upf/upf_gtp.h b/include/osmocom/upf/upf_gtp.h index 1a358ba..e3db918 100644 --- a/include/osmocom/upf/upf_gtp.h +++ b/include/osmocom/upf/upf_gtp.h @@ -83,6 +83,7 @@ int upf_gtp_dev_open(const char *name, bool create_gtp_dev, const char *local_addr, bool listen_for_gtpv0, bool sgsn_mode); struct upf_gtp_dev *upf_gtp_dev_find_by_name(const char *name); +struct upf_gtp_dev *upf_gtp_dev_find_by_local_addr(const struct osmo_sockaddr *local_addr); struct upf_gtp_dev *upf_gtp_dev_first(); int upf_gtp_dev_tunend_add(struct upf_gtp_dev *dev, const struct upf_gtp_tunend_desc *t); diff --git a/src/osmo-upf/up_gtp_action.c b/src/osmo-upf/up_gtp_action.c index 8ce8a6f..3b3da86 100644 --- a/src/osmo-upf/up_gtp_action.c +++ b/src/osmo-upf/up_gtp_action.c @@ -79,6 +79,7 @@ static int up_gtp_action_enable_disable(struct up_gtp_action *a, bool enable) { struct upf_gtp_dev *gtp_dev; + const struct osmo_sockaddr *gtp_addr; int rc; switch (a->kind) { @@ -89,12 +90,15 @@ return 0; } - /* use the first available GTP device. - * TODO: select by interface name? - */ - gtp_dev = upf_gtp_dev_first(); + /* Pick GTP device matching the local F-TEID set up for the GTP tunnel (it is on the Access side) */ + gtp_addr = &a->tunend.access.gtp_local_addr; + gtp_dev = upf_gtp_dev_find_by_local_addr(gtp_addr); if (!gtp_dev) { - LOG_UP_GTP_ACTION(a, LOGL_ERROR, "No GTP device open, cannot %s\n", enable ? "enable" : "disable"); + LOG_UP_GTP_ACTION(a, LOGL_ERROR, "No GTP device open for local address %s, cannot %s" + " -- consider configuring 'tunend' / 'dev (create|use) foo %s'\n", + osmo_sockaddr_to_str_c(OTC_SELECT, gtp_addr), + enable ? "enable" : "disable", + osmo_sockaddr_to_str_c(OTC_SELECT, gtp_addr)); return -EIO; } @@ -107,7 +111,8 @@ enable ? "enable" : "disable", rc, strerror(-rc)); return rc; } - LOG_UP_GTP_ACTION(a, LOGL_NOTICE, "%s GTP tunnel\n", enable ? "Enabled" : "Disabled"); + LOG_UP_GTP_ACTION(a, LOGL_NOTICE, "%s GTP tunnel on dev %s\n", enable ? "Enabled" : "Disabled", + gtp_dev->name); return 0; case UP_GTP_U_TUNMAP: diff --git a/src/osmo-upf/upf_gtp.c b/src/osmo-upf/upf_gtp.c index adac836..2c37bc4 100644 --- a/src/osmo-upf/upf_gtp.c +++ b/src/osmo-upf/upf_gtp.c @@ -73,6 +73,26 @@ return NULL; } +struct upf_gtp_dev *upf_gtp_dev_find_by_local_addr(const struct osmo_sockaddr *local_addr) +{ + struct upf_gtp_dev *dev; + struct upf_gtp_dev *dev_any = NULL; + struct osmo_sockaddr needle = *local_addr; + + llist_for_each_entry(dev, &g_upf->gtp.devs, entry) { + /* To leave the port number out of the cmp, set the needle's port to match */ + osmo_sockaddr_set_port(&needle.u.sa, osmo_sockaddr_port(&dev->gtpv1.local_addr.u.sa)); + + if (!osmo_sockaddr_cmp(&needle, &dev->gtpv1.local_addr)) + return dev; + if (osmo_sockaddr_is_any(&dev->gtpv1.local_addr) == 1) + dev_any = dev; + } + /* No 1:1 match found, but there is a dev listening on ANY? Return that. + * If there is no such dev, return NULL. */ + return dev_any; +} + struct upf_gtp_dev *upf_gtp_dev_first() { return llist_first_entry_or_null(&g_upf->gtp.devs, struct upf_gtp_dev, entry); diff --git a/src/osmo-upf/upf_vty.c b/src/osmo-upf/upf_vty.c index 4c683c8..4332902 100644 --- a/src/osmo-upf/upf_vty.c +++ b/src/osmo-upf/upf_vty.c @@ -137,37 +137,44 @@ return CMD_SUCCESS; } +static struct tunend_vty_cfg_dev *tunend_dev_add(int argc, const char **argv, bool create) +{ + struct tunend_vty_cfg_dev *d = talloc_zero(g_upf, struct tunend_vty_cfg_dev); + d->create = create; + d->dev_name = talloc_strdup(d, argv[0]); + if (argc > 1) + d->local_addr = talloc_strdup(d, argv[1]); + llist_add(&d->entry, &tunend_vty.devs); + return d; +} + DEFUN(cfg_tunend_dev_create, cfg_tunend_dev_create_cmd, "dev create DEVNAME [LISTEN_ADDR]", DEV_STR "Add GTP device, creating a new Linux kernel GTP device. Will listen on GTPv1 port " OSMO_STRINGIFY_VAL(PORT_GTP1_U) - " and GTPv0 port " OSMO_STRINGIFY_VAL(PORT_GTP0_U) " on the specified interface, or on ANY if LISTEN_ADDR is" - " omitted.\n" + " and GTPv0 port " OSMO_STRINGIFY_VAL(PORT_GTP0_U) " on the specified LISTEN_ADDR\n" "device name, e.g. 'apn0'\n" - "IPv4 or IPv6 address to listen on, omit for ANY\n") + "IPv4 or IPv6 address to listen on, omit for ANY. LISTEN_ADDR is used to pick a GTP device matching the local" + " address for a PFCP Network Instance," " which are configured in the 'netinst' node.\n") { - struct tunend_vty_cfg_dev *d = talloc_zero(g_upf, struct tunend_vty_cfg_dev); - d->create = true; - d->dev_name = talloc_strdup(d, argv[0]); - if (argc > 1) - d->local_addr = talloc_strdup(d, argv[1]); - llist_add(&d->entry, &tunend_vty.devs); - vty_out(vty, "Added GTP device %s (create new)%s", d->dev_name, VTY_NEWLINE); + struct tunend_vty_cfg_dev *d = tunend_dev_add(argc, argv, true); + vty_out(vty, "Added GTP device %s on %s (create new)%s", d->dev_name, d->local_addr ? : "0.0.0.0", VTY_NEWLINE); return CMD_SUCCESS; } DEFUN(cfg_tunend_dev_use, cfg_tunend_dev_use_cmd, - "dev use DEVNAME", + "dev use DEVNAME [LOCAL_ADDR]", DEV_STR "Add GTP device, using an existing Linux kernel GTP device, e.g. created by 'gtp-link'\n" - "device name, e.g. 'apn0'\n") + "device name, e.g. 'apn0'\n" + "The local GTP address this device listens on. It is assumed to be ANY when omitted." + " LOCAL_ADDR is used to pick a GTP device matching the local address for a PFCP Network Instance," + " which are configured in the 'netinst' node.\n") { - struct tunend_vty_cfg_dev *d = talloc_zero(g_upf, struct tunend_vty_cfg_dev); - d->create = false; - d->dev_name = talloc_strdup(d, argv[0]); - llist_add(&d->entry, &tunend_vty.devs); - vty_out(vty, "Added GTP device %s (use existing)%s", d->dev_name, VTY_NEWLINE); + struct tunend_vty_cfg_dev *d = tunend_dev_add(argc, argv, false); + vty_out(vty, "Added GTP device %s on %s (use existing)%s", d->dev_name, d->local_addr ? : "0.0.0.0", + VTY_NEWLINE); return CMD_SUCCESS; } diff --git a/tests/upf.vty b/tests/upf.vty index 51ebeb3..bfaa0cc 100644 --- a/tests/upf.vty +++ b/tests/upf.vty @@ -20,7 +20,7 @@ mockup no mockup dev create DEVNAME [LISTEN_ADDR] - dev use DEVNAME + dev use DEVNAME [LOCAL_ADDR] dev delete DEVNAME OsmoUPF(config-tunend)# exit @@ -28,19 +28,23 @@ OsmoUPF(config-tunend)# list ... dev create DEVNAME [LISTEN_ADDR] - dev use DEVNAME + dev use DEVNAME [LOCAL_ADDR] dev delete DEVNAME OsmoUPF(config-tunend)# dev? dev Configure the GTP device to use for encaps/decaps. OsmoUPF(config-tunend)# dev ? - create Add GTP device, creating a new Linux kernel GTP device. Will listen on GTPv1 port 2152 and GTPv0 port 3386 on the specified interface, or on ANY if LISTEN_ADDR is omitted. + create Add GTP device, creating a new Linux kernel GTP device. Will listen on GTPv1 port 2152 and GTPv0 port 3386 on the specified LISTEN_ADDR use Add GTP device, using an existing Linux kernel GTP device, e.g. created by 'gtp-link' delete Remove a GTP device from the configuration, and delete the Linux kernel GTP device if it was created here. OsmoUPF(config-tunend)# dev create ? DEVNAME device name, e.g. 'apn0' OsmoUPF(config-tunend)# dev create foo ? - [LISTEN_ADDR] IPv4 or IPv6 address to listen on, omit for ANY + [LISTEN_ADDR] IPv4 or IPv6 address to listen on, omit for ANY. LISTEN_ADDR is used to pick a GTP device matching the local address for a PFCP Network Instance, which are configured in the 'netinst' node. +OsmoUPF(config-tunend)# dev use ? + DEVNAME device name, e.g. 'apn0' +OsmoUPF(config-tunend)# dev use foo ? + [LOCAL_ADDR] The local GTP address this device listens on. It is assumed to be ANY when omitted. LOCAL_ADDR is used to pick a GTP device matching the local address for a PFCP Network Instance, which are configured in the 'netinst' node. OsmoUPF(config-tunend)# dev delete ? DEVNAME device name, e.g. 'apn0' OsmoUPF(config-tunend)# exit -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/30831 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: I376c09bfc1844df1e61d2efac17561fac614858b Gerrit-Change-Number: 30831 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: newchange

2 years, 5 months

1
0
0 0

Change in osmo-upf[master]: manual: add charts explaining tunend and tunmap

by neels

neels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-upf/+/30835 ) Change subject: manual: add charts explaining tunend and tunmap ...................................................................... manual: add charts explaining tunend and tunmap Change-Id: Ia0674c97eb0d8b5caeae988aefe523c5eee7318b --- M doc/manuals/chapters/overview.adoc 1 file changed, 24 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-upf refs/changes/35/30835/1 diff --git a/doc/manuals/chapters/overview.adoc b/doc/manuals/chapters/overview.adoc index a6dc345..fa01939 100644 --- a/doc/manuals/chapters/overview.adoc +++ b/doc/manuals/chapters/overview.adoc @@ -79,6 +79,18 @@ OsmoUPF does not support the complete PFCP feature set. It detects exactly two use cases that will provide service of actual GTP tunnels: +.tunend use case +---- +Access osmo-upf Core + | PDR1: > FAR1: | + | IP/GTP | IP | + | ------> F-TEID | -----> | + | | | + | FAR2: < PDR2: | + | IP/GTP | IP | + | F-TEID <------ | UE IP addr <----- | +---- + * `tunend`: GTP tunnel encapsulation/decapsulation: - One Packet Detection Rule (PDR) accepts a GTP tunnel from the Access side with an Outer Header Removal. @@ -86,6 +98,18 @@ - Another PDR accepts plain IP on a specific IP address from Core. - The second PDR uses a FAR towards Access with Outer Header Creation for GTP. +.tunmap use case +---- +Access osmo-upf Core + | PDR1: > FAR1: | + | IP/GTP | IP/GTP | + | ------> F-TEID | -----> F-TEID | + | | | + | FAR2: < PDR2: | + | IP/GTP | IP/GTP | + | F-TEID <------ | F-TEID <----- | +---- + * `tunmap`: GTP tunnel forwarding: - One Packet Detection Rule (PDR) accepts a GTP tunnel from the Access side with an Outer Header Removal. -- To view, visit https://gerrit.osmocom.org/c/osmo-upf/+/30835 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-upf Gerrit-Branch: master Gerrit-Change-Id: Ia0674c97eb0d8b5caeae988aefe523c5eee7318b Gerrit-Change-Number: 30835 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-MessageType: newchange

2 years, 5 months

1
0
0 0

Change in libosmocore[master]: libosmocore: Transition to use of 'telnet_init_default'

by neels

Attention is currently required from: arehbein, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/30703 ) Change subject: libosmocore: Transition to use of 'telnet_init_default' ...................................................................... Patch Set 4: Code-Review+1 (3 comments) Patchset: PS4: > Forgot to say. the title for the commit is too generic, doesn't explain what they do. What about: […] pespin, does this also need a TODO-RELEASE entry? File src/vty/telnet_interface.c: https://gerrit.osmocom.org/c/libosmocore/+/30703/comment/d47d21c4_6c64511d PS4, Line 45: \ref ('\ref' is to reference files, not functions. we've been using it wrongly for some time. Could drop it while busy editing the line anyway: ... call telnet_init_default() once https://osmocom.org/projects/cellular-infrastructure/wiki/Guidelines_for_AP… ) https://gerrit.osmocom.org/c/libosmocore/+/30703/comment/ba612e85_09edd4dd PS4, Line 96: \deprecated (interesting, i wasn't aware of the \deprecated cmd yet) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/30703 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ibd05d3bc2736256aa45e9e7ec15a98bd14a10454 Gerrit-Change-Number: 30703 Gerrit-PatchSet: 4 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 03 Jan 2023 01:29:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

Change in osmo-bsc[master]: vty: Add check against sensible default value for Ny1

by neels

Attention is currently required from: arehbein, fixeria, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 16: (5 comments) Patchset: PS16: your comments suggest some things to be resolved, but they still are unchanged in the latest patch set? File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/f001c355_c2e09980 PS14, Line 1738: unsigned long T3105 = osmo_tdef_get(bts->network->T_defs, 3105, OSMO_TDEF_MS, -1); > Maybe this rule is implied by "linux kernel style". […] vars are still not declared at the top File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/ac930644_efb86ea8 PS16, Line 537: } (braces are still here) https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/66b170f2_50c23599 PS16, Line 1725: . (stray dot? "iff" isn't an abbreviation in the usual sense) File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/fd912c7f_0f42dbc8 PS16, Line 95: (void) > Done the "(void)" is still here -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 16 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 03 Jan 2023 01:16:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

Change in osmo-bsc[master]: vty: Add check against sensible default value for Ny1

by neels

Attention is currently required from: arehbein, fixeria, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 16: (2 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/fa60d1e6_d68e98eb PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; > I'm not sure I understand... […] I'm only saying this as a suggestion, in case you are aiming for a clean ceil() instead of a "custom" calculation like floor() + 1. I'm not sure what's the correct formula, but the comment sounds like you wanted a rounded up integer division, as in ceil((float)(T3124 + GSM_NY1_REQ_DELTA) / T3105) if that is true then the correct integer division here is (T3124 + GSM_NY1_REQ_DELTA + (T3105-1)) / T3105 (like in OSMO_BYTES_FOR_BITS() for example) The current patch results in floor((float)(T3124 + GSM_NY1_REQ_DELTA) / T3105) + 1 which is one too many if the numbers work out as a clean division (no fractional part). File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/cc5b2839_0eab2e72 PS14, Line 1738: unsigned long T3105 = osmo_tdef_get(bts->network->T_defs, 3105, OSMO_TDEF_MS, -1); > that was my 'reasoning' as well, didn't see it in our guidelines. […] Maybe this rule is implied by "linux kernel style". All i know is that code review from day one of submitting osmocom patches asked me to put vars at the top, the reasoning being supposedly compatibility with compilers, and i adopted that (though i personally very much prefer declaring vars at their actual first use) -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 16 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 03 Jan 2023 01:12:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

Change in osmo-bsc[master]: vty: Add check against sensible default value for Ny1

by arehbein

Attention is currently required from: neels, fixeria, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 16: (8 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/02b3622f_27b56219 PS9, Line 1742: bool ny1_satisfies_requirements = T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA; > Is this a hard rule of ours or a preference? If there is any leeway, I'd rather leave it as is. Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/1d37c437_221f9e75 PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; > the standard idiom for integer division rounding up is: […] I'm not sure I understand... if 'a', 'b' are wlog nonnegative integers and b is nonzero, then 'a/b = rounddown(a/b) + r' with 'r in [0,1)', which goes along with my earlier reply. Since we are only using unsigned types this should be alright. I can still change it if it's nonetheless important File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/05e83da7_2b401911 PS14, Line 537: } > (no need for braces when the body is a single line) Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/059d9674_7d8f6fa0 PS14, Line 1725: otherwise return a suggestion for Ny1 > this is still not true Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/d79887c1_16a8e4b6 PS14, Line 1726: the message to print > this also needs to be updated, I guess (I see no msg argument) Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/298a4763_7b98d011 PS14, Line 1729: const struct gsm_lchan *gl = gsm_bts_get_cbch(bts); > (re earlier question, 'lchan' stands for 'logical channel', a sub-division of a BTS timeslot) thanks :) https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/00ced4a7_f056287f PS14, Line 1738: unsigned long T3105 = osmo_tdef_get(bts->network->T_defs, 3105, OSMO_TDEF_MS, -1); > in osmocom style we are required to declare all variables at the top of the scope that was my 'reasoning' as well, didn't see it in our guidelines. I've changed it though File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/35711518_f3d8897a PS16, Line 95: (void) > drop the cast to void? Done -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 16 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 03 Jan 2023 00:19:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

Change in osmo-bsc[master]: vty: Add check against sensible default value for Ny1

by fixeria

Attention is currently required from: neels, arehbein, pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 16: (3 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/6787ab5e_9bb9fd01 PS14, Line 1725: otherwise return a suggestion for Ny1 > this is still not true Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/a4f75966_616bef2b PS14, Line 1726: the message to print > this also needs to be updated, I guess (I see no msg argument) Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/776c4ccc_2770d66e PS14, Line 1738: unsigned long T3105 = osmo_tdef_get(bts->network->T_defs, 3105, OSMO_TDEF_MS, -1); > in osmocom style we are required to declare all variables at the top of the scope Huh, I don't see such a requirement in https://osmocom.org/projects/cellular-infrastructure/wiki/Coding_standards#…. Likely something we agreed on verbally? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 16 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 02 Jan 2023 23:51:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de> Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

Change in osmo-bsc[master]: vty: Add check against sensible default value for Ny1

by neels

Attention is currently required from: arehbein, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 ) Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 16: (5 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/d1837a83_5f05c512 PS9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1; > Without the '+ 1', due to integer arithmetic in C, we would be setting ny1 to the largest integer sm […] the standard idiom for integer division rounding up is: (foo + (bar - 1)) / bar this avoids false incrementing when the division result was an exact integer File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/25d572f1_b728eadd PS14, Line 537: } (no need for braces when the body is a single line) https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/2449ffe2_dc6c9968 PS14, Line 1729: const struct gsm_lchan *gl = gsm_bts_get_cbch(bts); (re earlier question, 'lchan' stands for 'logical channel', a sub-division of a BTS timeslot) https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/df546584_1e43201e PS14, Line 1738: unsigned long T3105 = osmo_tdef_get(bts->network->T_defs, 3105, OSMO_TDEF_MS, -1); in osmocom style we are required to declare all variables at the top of the scope File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/a69f0cb9_077c0a65 PS16, Line 95: (void) drop the cast to void? -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 16 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: neels <nhofmeyr(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: arehbein <arehbein(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 02 Jan 2023 23:26:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <arehbein(a)sysmocom.de> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

Change in osmo-bsc[master]: vty: Add support for Ny1 configuration

by arehbein

Attention is currently required from: fixeria, pespin. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30539 ) Change subject: vty: Add support for Ny1 configuration ...................................................................... Patch Set 13: (1 comment) File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30539/comment/ef721378_cc4dd02b PS12, Line 24: osmocom/core/logging.h > you are not adding any logging in this patch, why including this header? Remainder from code I moved to the other commit, thanks! -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30539 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: I6318cceb4ebdce50005e39e2e9323c1c8433250a Gerrit-Change-Number: 30539 Gerrit-PatchSet: 13 Gerrit-Owner: arehbein <arehbein(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Mon, 02 Jan 2023 23:24:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-MessageType: comment

2 years, 5 months

1
0
0 0

2025

2024

2023

2022

gerrit-log January 2023