Remove gr_ prefix from internal headers and symbols - #586
Conversation
The gr_ prefix on symbols is meant to denote public API. The daemon private headers in main/ are not part of the public API so the prefix is misleading. Rename them to plain names and switch all includes accordingly. Also rename gr_queue.h to sys_queue.h since queue.h would be too generic. Signed-off-by: Robin Jarry <rjarry@redhat.com>
The metrics types and functions (gr_metrics_writer, gr_metrics_collector, gr_metrics_register, gr_metric_emit, etc.) are internal to the daemon, not part of the public API. Remove the gr_ prefix from all of them. Signed-off-by: Robin Jarry <rjarry@redhat.com>
The event subscription and notification functions (gr_event_subscribe, gr_event_push, gr_event_serialize, etc.) and their associated types are internal to the daemon. Remove the gr_ prefix from all of them. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Rename gr_module to module, and gr_register_module() to module_register(). These are internal to the daemon and do not belong in the public API namespace. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Rename gr_api_handler() to api_handler() and gr_api_handler_func to api_handler_func. The handler registration mechanism is internal to the daemon and not exposed in the public API. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Rename GR_LOG_TYPE() to LOG_TYPE(), gr_log_type to log_type, and gr_log_types to log_types. These are daemon-internal logging helpers and not part of the public API. Signed-off-by: Robin Jarry <rjarry@redhat.com>
Rename gr_id_pool and all its functions (gr_id_pool_create, gr_id_pool_destroy, gr_id_pool_get, gr_id_pool_put, etc.) to drop the gr_ prefix. The ID pool is a daemon-internal utility, not part of the public API. Signed-off-by: Robin Jarry <rjarry@redhat.com>
📝 WalkthroughWalkthroughThis pull request systematically refactors the codebase to decouple internal APIs from the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/ip/datapath/icmp_input.c (1)
25-25:⚠️ Potential issue | 🔴 CriticalCritical: Buffer overflow vulnerability (pre-existing).
The array
icmp_cb[UINT8_MAX]has size 255, with valid indices 0-254. However,icmp_typeis auint8_tthat can be 255, causing out-of-bounds access at lines 57, 58, 77, and 80. A malformed ICMP packet with type 255 would trigger a buffer overflow.🛡️ Proposed fix
-static control_queue_cb_t icmp_cb[UINT8_MAX]; +static control_queue_cb_t icmp_cb[UINT8_MAX + 1];Note: This is a pre-existing issue, not introduced by this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/ip/datapath/icmp_input.c` at line 25, The icmp_cb array is sized as UINT8_MAX (255) but is indexed by an uint8_t icmp_type which can be 255, causing out-of-bounds access; fix by either allocating icmp_cb with space for all 256 values (use UINT8_MAX + 1 / 256 entries) or add an explicit bounds check before any access using icmp_type (e.g., in the places that reference icmp_cb with icmp_type) to reject/handle icmp_type == 255 safely; update the declaration of icmp_cb and/or the indexing sites (references to icmp_cb and the icmp_type usages) accordingly to prevent overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@modules/ip/datapath/icmp_input.c`:
- Line 25: The icmp_cb array is sized as UINT8_MAX (255) but is indexed by an
uint8_t icmp_type which can be 255, causing out-of-bounds access; fix by either
allocating icmp_cb with space for all 256 values (use UINT8_MAX + 1 / 256
entries) or add an explicit bounds check before any access using icmp_type
(e.g., in the places that reference icmp_cb with icmp_type) to reject/handle
icmp_type == 255 safely; update the declaration of icmp_cb and/or the indexing
sites (references to icmp_cb and the icmp_type usages) accordingly to prevent
overflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9179bd97-cb7f-433d-a6e9-c892497e8fb6
📒 Files selected for processing (101)
api/printf_test.capi/string_test.cdevtools/ai-instructions.mdmain/_cmocka.hmain/api.cmain/config.hmain/control_queue.cmain/dpdk.cmain/event.cmain/event.hmain/id_pool.hmain/id_pool_test.cmain/log.cmain/log.hmain/main.cmain/metrics.cmain/metrics.hmain/module.cmain/module.hmain/signals.cmain/sort.cmain/sort.hmain/sys_queue.hmain/unix.cmain/vec.hmain/vec_test.cmodules/dhcp/control/client.cmodules/dhcp/control/options.cmodules/dhcp/control/packet.cmodules/infra/api/affinity.cmodules/infra/api/iface.cmodules/infra/api/nexthop.cmodules/infra/api/stats.cmodules/infra/api/trace.cmodules/infra/control/bond.cmodules/infra/control/bond.hmodules/infra/control/ctlplane.cmodules/infra/control/graph.cmodules/infra/control/graph.hmodules/infra/control/group_nexthop.cmodules/infra/control/iface.cmodules/infra/control/iface.hmodules/infra/control/l3_nexthop.cmodules/infra/control/lacp.cmodules/infra/control/loopback.cmodules/infra/control/mempool.cmodules/infra/control/netlink.cmodules/infra/control/nexthop.cmodules/infra/control/nexthop.hmodules/infra/control/port.cmodules/infra/control/port_test.cmodules/infra/control/vlan.cmodules/infra/control/vrf.cmodules/infra/control/worker.cmodules/infra/control/worker.hmodules/infra/control/worker_test.cmodules/infra/datapath/control_input.cmodules/infra/datapath/control_output.cmodules/infra/datapath/drop.cmodules/infra/datapath/eth_input.cmodules/infra/datapath/iface_input.cmodules/infra/datapath/iface_output.cmodules/infra/datapath/loop_input.cmodules/infra/datapath/main_loop.cmodules/infra/datapath/port_rx.cmodules/infra/datapath/port_tx.cmodules/infra/datapath/snap_input.cmodules/infra/datapath/trace.cmodules/ip/control/address.cmodules/ip/control/icmp.cmodules/ip/control/nexthop.cmodules/ip/control/route.cmodules/ip/datapath/icmp_input.cmodules/ip/datapath/ip_input.cmodules/ip/datapath/ip_local.cmodules/ip/datapath/ip_output.cmodules/ip6/control/address.cmodules/ip6/control/icmp6.cmodules/ip6/control/nexthop.cmodules/ip6/control/route.cmodules/ip6/control/router_advert.cmodules/ip6/datapath/icmp6_input.cmodules/ip6/datapath/ip6_error.cmodules/ip6/datapath/ip6_input.cmodules/ip6/datapath/ip6_local.cmodules/ip6/datapath/ip6_output.cmodules/ip6/datapath/ndp_na_input.cmodules/ipip/control.cmodules/l2/control/bridge.cmodules/l2/control/fdb.cmodules/l2/control/flood.cmodules/l2/control/vxlan.cmodules/l4/l4_input_local.cmodules/policy/api/dnat44.cmodules/policy/api/snat44.cmodules/policy/control/conntrack.cmodules/policy/control/nat.hmodules/policy/control/snat44_dynamic.cmodules/policy/control/snat44_static.cmodules/srv6/control/route.cmodules/srv6/datapath/srv6_local.c
Summary
This pull request removes the
gr_prefix from internal headers and symbols throughout the codebase, transitioning from external-style includes (<gr_*.h>) to local project includes ("*.h") and renaming public API functions and types accordingly.Scope of Changes
Header Includes:
<gr_config.h>,<gr_event.h>,<gr_log.h>,<gr_metrics.h>,<gr_queue.h>,<gr_vec.h>,<gr_sort.h>,<gr_id_pool.h>,<gr_cmocka.h>) with local project headers ("config.h","event.h","log.h","metrics.h","sys_queue.h","vec.h","sort.h","id_pool.h","_cmocka.h")API Renames:
Module registration:
gr_register_module(struct gr_module *)→module_register(struct module *)struct gr_module→struct moduleLogging:
GR_LOG_TYPE(name)→LOG_TYPE(name)struct gr_log_type→struct log_typegr_log_types→log_typesEvents:
gr_event_subscribe()→event_subscribe()gr_event_push()→event_push()gr_event_serializer()→event_serializer()gr_event_serialize()→event_serialize()gr_event_sub_cb_t→event_sub_cb_tgr_event_serializer_cb_t→event_serializer_cb_tMetrics:
gr_metrics_register()→metrics_register()gr_metrics_ctx_init()→metrics_ctx_init()gr_metrics_labels_add()→metrics_labels_add()gr_metric_emit()→metric_emit()gr_metric_emit_histogram()→metric_emit_histogram()gr_metrics_set_affinity()→metrics_set_affinity()gr_metrics_start()/gr_metrics_stop()→metrics_start()/metrics_stop()struct gr_metrics_ctx→struct metrics_ctxstruct gr_metrics_writer→struct metrics_writerstruct gr_metrics_collector→struct metrics_collectorenum gr_metric_type→enum metric_type(valuesGR_METRIC_*→METRIC_*)ID Pool:
gr_id_pool_*functions →id_pool_*functionsstruct gr_id_pool→struct id_poolAPI Handlers:
gr_api_handler()macro →api_handler()macro__gr_api_handler()→__api_handler()gr_api_handler_func→api_handler_funcFiles Modified
Core infrastructure:
main/api.c,main/config.h,main/event.c,main/event.h,main/log.c,main/log.h,main/metrics.c,main/metrics.h,main/module.c,main/module.h,main/id_pool.h,main/id_pool_test.cData structures:
main/sort.c,main/sort.h,main/control_queue.c,main/dpdk.c,main/main.c,main/signals.c,main/unix.cTest files:
api/printf_test.c,api/string_test.c,main/vec_test.c,modules/infra/control/port_test.c,modules/infra/control/worker_test.c,modules/ip/datapath/ip_input.c,modules/ip6/datapath/ip6_input.cModule control and datapath: All
modules/subdirectories includingdhcp,infra,ip,ip6,ipip,l2,l4,policy, andsrv6Documentation:
devtools/ai-instructions.mdAlterations to Exported Entities
Changes are systematic across modules but notably affect: