Attention is currently required from: pespin.

View Change

2 comments:

• File src/core/stats_tcp.c:

  • Patch Set #2, Line 25: #if !defined(EMBEDDED)

    You can probably remove this defined(EMBEDDED) block now, since it will be covered by the ifdef HAVE […]

    I see the point, but I'm a bit hesitant to drop the EMBEDDED guard purely
    because of HAVE_LINUX_TCP_H.

    With --enable-embedded this code path used to be disabled regardless of what
    headers are present. If we remove the EMBEDDED guard now, an embedded build
    could end up compiling it just because linux/tcp.h happens to be available in
    the toolchain sysroot, which would be a behaviour change compared to before.

    I'd prefer to keep the embedded decision explicit (either keep the EMBEDDED
    guard or force HAVE_LINUX_TCP_H / the corresponding feature off when
    --enable-embedded is used in configure.ac), and only use HAVE_LINUX_TCP_H to
    guard the Linux-specific implementation details. This way embedded builds stay
    consistent, and CI should remain happy.

  • Patch Set #2, Line 52: static struct osmo_tcp_stats_config s_tcp_stats_config = {

    This should also be inside the HAVE_LIJNUX_TCP_H block below.

    I don't think this can be moved entirely under HAVE_LINUX_TCP_H.

    The data structure is accessed unconditionally from other code paths:
    - stats.c calls osmo_stats_tcp_set_interval(osmo_tcp_stats_config->interval)
    - stats_vty.c reads/writes osmo_tcp_stats_config->{interval,batch_size} and
      prints them in the VTY config output

    If the definition/allocation is moved under HAVE_LINUX_TCP_H, those callers
    will end up dereferencing a NULL pointer at runtime, leading to a segfault.

To view, visit change 41922. To unsubscribe, or for help writing mail filters, visit settings.