Attention is currently required from: laforge, neels.
27 comments:
File include/osmocom/hnbgw/hnbgw.h:
Patch Set #6, Line 384: /* When rules to count traffic to and from this hNodeB are present, this reflects the state in nftables
I'm not understanding this comment. You first mention "rules", and after "nftables rules". Not sure what are the differences.
Patch Set #6, Line 394: struct osmo_sockaddr_str addr_remote;
open question: to be seen whether it makes more sense to use a osmo_sockaddr instead here. It depends on whether we really use strings all the time or whether libnft or alike uses sockaddr.
Patch Set #6, Line 402: struct {
maybe you want to declare the struct type once and use it twice here.
Maybe add it in osmocom/hnbgw/nft_kpi.h?
Patch Set #6, Line 446: bool enable;
some short comment here on what these do would be welcome.
Patch Set #6, Line 483: struct {
same here, some comment welcome.
Patch Set #6, Line 514: void hnb_persistent_connected(struct hnb_persistent *hnbp);
difficult to understand what are these exactly only by looking at the name. Some functions that get called when hnb is connected or disconnected? at which stack level?
File include/osmocom/hnbgw/nft_kpi.h:
Patch Set #6, Line 7: struct nft_kpi_handle {
Some comments docummenting these structs and fields may be welcome.
Patch Set #6, Line 19: void nft_kpi_hnb_persistent_init(struct hnb_persistent *hnbp);
These APIs seems to be counterparts, but sounds weird having init vs remove.
Patch Set #6, Line 21: int nft_kpi_hnb_start(struct hnb_persistent *hnbp, const struct osmo_sockaddr_str *gtpu_remote);
This addr is probably coming from RANAP in binary form, so feels weird having it converted to a osmo_sockaddr_str instead of keeping it as a osmo_sockaddr.
File src/osmo-hnbgw/hnbgw.c:
Patch Set #6, Line 346: hnb_persistent_disconnected(ctx->persistent);
so API is not really protocol related. More like "attach/detach" maybe?
Patch Set #6, Line 590: if (osmo_sockaddr_str_from_osa(&remote_str, &osa)) {
there we go, a sockaddr transformed into a sockaddr_str, not sure if there's a real reason to do so, maybe inside nft we use strings?
Sounds like a self-made FSM right? :P
May make sense to move to osmo_fsm.
File src/osmo-hnbgw/hnbgw_hnbap.c:
Patch Set #6, Line 571: hnb_persistent_connected(ctx->persistent);
Since this happens at registration time, maybe rename it to "hnb_persistent_registered"? I still find the "connected" here a bit confusing.
File src/osmo-hnbgw/hnbgw_vty.c:
Patch Set #6, Line 884: DEFUN(cfg_hnbgw_nft_kpi, cfg_hnbgw_nft_kpi_cmd,
I think we have VTY CMD flags to state that changes apply after a process restart, you could use them here.
Patch Set #6, Line 899: g_hnbgw->config.nft_kpi.table_name = talloc_strdup(g_hnbgw, set_table_name);
These 3 lines is basically a osmo_talloc_replace_string right?
File src/osmo-hnbgw/nft_kpi.c:
Patch Set #6, Line 18: #include <inttypes.h>
Some overall architecture comment description here would be great, to quickly figure out the interesting APIs.
I guess these will be filled up by follow-up patches adding hashtables?
Patch Set #6, Line 86: static __thread struct nft_thread *g_nft_thread = NULL;
having a thread-specific variable prefix with "g_" sounds weird, since it's not really globally available :)
Maybe "self_nft_thread".
EDIT: Ok after having a whole look at the file, I see this is actually a pointer only. But I don't really see a point in having a TLS variable for this. Simply have one var per thread, even better when you split stuff into different files.
Patch Set #6, Line 120: LOGP(DNFT, LOGL_ERROR, "error running nft cmd %s#%u: rc=%d cmd=%s\n",
if nft fails, we have no way to say so to the callback?
Patch Set #6, Line 182: enum nft_thread_req_type {
These can probably be moved up before the static functions.
Patch Set #6, Line 193: static const char * const nft_thread_req_type_name[] = {
osmo_value_string?
Patch Set #6, Line 207: struct {
IIUC these are actually the params for the different primitives right?
Patch Set #6, Line 251: OSMO_ASSERT(g_nft_thread);
why do you need the TLS g_nft_hread allocated statically? Why not allocate it using talloc here?
This way we don't reserve space for other threads which may not be using it.
Patch Set #6, Line 297: static void nft_t2m_enqueue(struct nft_thread *t, struct nft_thread_req *req)
you may want to use the concept of "worker thread" vs "main thread", because well, main is also a thread. May make it more clear. Also renaming nft_thread to nft_thread_ctx. Just ideas in case you are interested.
Patch Set #6, Line 415: static int update_ctr(struct rate_ctr_group *cg, int cgidx, uint64_t *last_val, uint64_t new_val)
I first though about cgi when reading this. maybe ctrg_idx?
Patch Set #6, Line 419: rate_ctr_add2(cg, cgidx, new_val - *last_val);
Oh I'm surprised to find this function here. Mixing functions from both threads in the same section of the file makes everything confusing imho, difficult to fiugre out what runs where.
It may even make sense to split this into 2 files, one per thread.
Actually, ideally, that would be 3 files. One for each thread type. Alternatively main thread stuff can be moved to some existing file.
Patch Set #6, Line 452: size_t want = g_nft_thread->counters_len + 64;
I'd say usually one does *2 here. It's not like this will be called lots of times, but anyway you get less fragmentation probably.
To view, visit change 36539. To unsubscribe, or for help writing mail filters, visit settings.