Change in libosmocore[master]: refactor stat_item: get rid of FIFO and "skipped" error

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/25464 )

Change subject: refactor stat_item: get rid of FIFO and "skipped" error
......................................................................


Patch Set 1:

(8 comments)

https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h 
File include/osmocom/core/stat_item.h:

https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@42 
PS1, Line 42: 	int32_t not_stats_next_id;
Since it seems nobody is using this field outside libosmocore (you are changing the time), then probably change it to something even more understandable, like "reserved0", "reserved1", or alike.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@46 
PS1, Line 46: 	struct osmo_stat_item_value not_values[0];
This one can be dropped since it adds no space, and you are changing the name so it means nobody is using it outside libosmocore.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@49 
PS1, Line 49: 	struct osmo_stat_item_period value;
I'm really not understanding fully why you leave the other fields while changing their name. Can you explain the motivation or rationale behind keeping them? Is this struct used directly (not as a pointer) or any of its fields somewhere outside libosmocore?


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@125 
PS1, Line 125: 	OSMO_DEPRECATED("Access item->value.last, item->value.min, item->value.max or osmo_stat_item_get_avg() instead");
I'd add API getter for last, min and max if they are to be used outside libosmocore (I doubt so).


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/include/osmocom/core/stat_item.h@127 
PS1, Line 127: void osmo_stat_item_flush(struct osmo_stat_item *item);
There's a new function in between deprecated ones? please move it somewhere else.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/src/stat_item.c 
File src/stat_item.c:

https://gerrit.osmocom.org/c/libosmocore/+/25464/1/src/stat_item.c@68 
PS1, Line 68:  * "virtual-trunk-0" and can be looked up by that name instead of its more or less arbitrary group index number).
Maybe add that user of this subsystem is in charge of avoiding group name collisions, ie assigning uniques names to different group instances.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/src/stat_item.c@346 
PS1, Line 346: int osmo_stat_item_get_next(const struct osmo_stat_item *item, int32_t *next_id,
next_id is not being used here, but documentation still explains old behavior. IIRC this is marked as deprecated right? Is it used by anyone anymore? maybe also write it here somewhere.


https://gerrit.osmocom.org/c/libosmocore/+/25464/1/src/stat_item.c@369 
PS1, Line 369: 	item->value.min = item->value.max = item->value.last;
Not sure whether this is really what's needed here. Do we care at all in case value n=0 anyway? Maybe call osmo_stat_item_reset?



-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/25464
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I137992a5479fc39bbceb6c6c2af9c227bd33b39b
Gerrit-Change-Number: 25464
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 15 Sep 2021 14:10:24 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210915/5888d721/attachment.htm>