Bug #5027
logging_gsmtap.c code not filling PID field in pkt header
100%
Description
It was recently discovered that the PID field was always sent as 0.
This first patch fixes the issue:
https://gerrit.osmocom.org/c/libosmocore/+/22920
During patch review, a discussion was started on possible improvements.
The idea is to instead of getpid(), fill it gettid() to be able to distinguish threads.
gettid() support in system is sometimes a bit flacky. osmo-trx already solves that by implemeting its own wrapper "my_gettid" based on autoconf detection in debug.c.
We should move that "my_gettid" implementation as "osmo_gettid()" and put it in some "thread_compat.h" in libosmocore, similar to what we do with "timer_compat.h" timespec operations.
Then, in libosmocore's logging_gsmtap.c, change the static global variable logging_gsmtap_pid to be __thread, and update it each time _gsmtap_raw_output() is called if value is 0.
Finally, remove the "my_gettid" from osmo-trx and use the libosmocore one.
Related issues
Associated revisions
Threads.cpp: Use already existing gettid wrapper function
A wrapper function with better support already exists in debug.c and
announced in debug.h. Let's use that one instead.
Related: OS#5027
Change-Id: I2ccf94f95a531d5873da2a4681cf89cbc5b31422
Introduce osmo_gettid() API
This API wraps conventional gettid() linux-specific API, which even in
Linux itself is sometimes not properly supported/announced.
This API also allows future porting to other platforms if needed, and so
far falls back to getpid() if no gettid(9 can be found.
Code ported from osmo-trx.git, see commit 7a07de1efd4eb7cc11c33d3ad25cb2df70aa1ef1.
Related: OS#5027
Change-Id: Id7534beeb22fcd50813dab76dd68818e2ff87ec2
Replace my_gettid with libosmocore osmo_gettid API
The API was moved to libosmocore, let's use it instead of defining our
own here with all the complexity in build system involved.
Depends: libosmocore.git Change-Id Id7534beeb22fcd50813dab76dd68818e2ff87ec2
Related: OS#5027
Change-Id: I19e32fbc47bd88a668e0c912e89b001b0f8831dd
logging: gsmtap: Fix fill PID field not stored in network byte order
Recent commit filling this field forgot to convert it to network byte
order.
Related: OS#5027
Fixes: bb149ecda21a4f9b102245ffc6a2870592d32c0b
Change-Id: I50857f35cb28138fa6f28100afeaa00f492f303a
logging: gsmtap: Store TID instead of PID in pkt hdr
This allows differentiating threads withing an application, while still
keeping same numbering for single-threaded application (since first
thread ID is always the same as the process group ID).
Related: OS#5027
Change-Id: I33da02524fc064e133b2b762af7060139c4cfd81
History
#3 Updated by pespin 18 days ago
osmo_gettid() added to libosmocore:
https://gerrit.osmocom.org/c/libosmocore/+/22949 Introduce osmo_gettid() API
osmo-trx using the new libosmocore API:
https://gerrit.osmocom.org/c/osmo-trx/+/22950 Replace my_gettid with libosmocore osmo_gettid API
- use new osmo_gettid() API in logging_gsmtap.c
#4 Updated by pespin 18 days ago
- Status changed from Stalled to Feedback
- % Done changed from 80 to 90
I fixed PID not being filled as big endian:
https://gerrit.osmocom.org/c/libosmocore/+/22951 logging: gsmtap: Fix fill PID field not stored in network byte order
Then, patch submitted to use TID instead of PID (did a few tests with osmo-trx and it looks good):
https://gerrit.osmocom.org/c/libosmocore/+/22952 logging: gsmtap: Store TID instead of PID in pkt hdr
Once those are merged, we can close the ticket.
#6 Updated by pespin 17 days ago
- Related to Feature #5032: Add VTY option to write TID in log line prefix added
logging: gsmtap: Fill PID field for each message
It was recently discovered that PID field in gsmtap log messages was
always set to 0. Before this patch, the field was never being set.
The approach of this patch is to record the PID of process one, in
order to avoid calling getpid() syscall on each
log line to be sent. The counterpart of this optimization is that
eventual fork() calls would still keep the old incorrect value, but I
think nobody can safely assume that fork() is possible once all this
kind of infrastructure has already been configured (fork() should only
be done really at the start of the program before any osmocom foo is
initialized, or to immediatelly call exec()).
Related: OS#5027
Change-Id: I7db00d1810f0860166bffa0bda8566caa82e06a9