Project

General

Profile

Bug #5027

logging_gsmtap.c code not filling PID field in pkt header

Added by pespin 13 days ago. Updated 11 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
02/16/2021
Due date:
% Done:

100%

Spec Reference:

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

Related to libosmocore - Feature #5032: Add VTY option to write TID in log line prefixFeedback02/17/2021

Associated revisions

Revision bb149ecd (diff)
Added by pespin 13 days ago

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

Revision cec9eda2 (diff)
Added by pespin 12 days ago

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

Revision 88e4058f (diff)
Added by pespin 12 days ago

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

Revision 57389405 (diff)
Added by pespin 12 days ago

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

Revision 2f765f02 (diff)
Added by pespin 12 days ago

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

Revision 74dddefc (diff)
Added by pespin 12 days ago

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

#1 Updated by laforge 13 days ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100

patch merged

#2 Updated by laforge 13 days ago

  • Status changed from Resolved to Stalled
  • % Done changed from 100 to 80

#3 Updated by pespin 12 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

TODO:
  • use new osmo_gettid() API in logging_gsmtap.c

#4 Updated by pespin 12 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.

#5 Updated by pespin 11 days ago

  • Status changed from Feedback to Resolved
  • % Done changed from 90 to 100

Merged, closing.

#6 Updated by pespin 11 days ago

  • Related to Feature #5032: Add VTY option to write TID in log line prefix added

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)