Attention is currently required from: laforge, pespin.

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539?usp=email )

Change subject: per-HNB GTP-U traffic counters via nft ......................................................................

Patch Set 13:

(5 comments)

Patchset:

PS6:

Actually it is the same thread running twice. […]

regarding split files, i kind of like that everything is contained in a single .c file. I thought a bit and it might be even easier to understand if the nft_thread is in a separate .c file, but then we also need another .h file and so on...

for me it's fine in one .c file (with the new comments describing what is going on), but just say the word and i can try to split to another .c file too.

File src/osmo-hnbgw/hnbgw_hnbap.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/2c4c4abc_2eb93499 PS6, Line 571: hnb_persistent_connected(ctx->persistent);

I agree pespin's wording is more clear.

Done

File src/osmo-hnbgw/hnbgw_vty.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/82819dea_b42b49ca PS12, Line 885: "nft-kpi [TABLE_NAME]",

shouldn't this be in some kind of #ifdef block to suppress offering those vty commands in case the p […]

This is a basic disagreement we encounter repeatedly.

IMHO parsing of a config file should be conceptually independent from what the binary supports.

- First read what the user intends, then error later if that is not possible. - A command that is described in the docs should not appear as a syntax error; instead as a semantic error.

I much prefer if the VTY commands are always available also for practical reasons:

- to generate automatic xml-output of VTY commands for the manuals, we don't have to specifically compile a binary with all possible features enabled. We just dump the VTY with the simplest dependencies and have all the docs. - allows simpler vty testing: a 'list' always shows the same items, etc. - allows simpler working with cfg files: if parsing fails for a missing command, it is hard to understand what is going on. The following instead is very clear: "you configured nft-kpi but this binary is built without support" - the user can pass a 'no nft-kpi' command to a binary without nft-kpi support, which would otherwise be an error.

This patch quits the program if nft-kpi is present in the cfg but there is no support for that compiled in, with a sensible error message, at program startup.

File src/osmo-hnbgw/nft_kpi.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/5a7cbe46_24696b8d PS9, Line 994: nft_kpi_get_counters_schedule();

I agree with neels that his approach implements what we discussed (for the reasons we discussed and […]

an idea: best of both worlds: measure the time it took from the last request, and if there is still some left over, wait only for the remaining time. If time is up, don't wait at all.

We can do that in a separate patch...

File src/osmo-hnbgw/nft_kpi.c:

https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/1bd57bcc_2bd5dee9 PS12, Line 694: unsigned long period_us = osmo_tdef_get(hnbgw_T_defs, -34, OSMO_TDEF_US, 1000000);

elasped = now() - prev_start_ts; […]

ah yes this is exactly what i thought