Code quality improvements, CERALIVE fork migration, and documentation restructure#5
Merged
andrescera merged 19 commits intomainfrom Jan 9, 2026
Merged
Conversation
The previous implementation called read_bitrate_file() directly from signal context, which invoked non-async-signal-safe functions (fopen, getline, malloc, free). This could cause deadlocks or corruption. Changes: - Add volatile sig_atomic_t flag for signal communication - Add minimal sighup_handler() that only sets the flag - Check flag in stall_check() to reload bitrate file safely - Fix file handle leak in read_bitrate_file() (missing fclose) - Remove unsafe cast to __sighandler_t Fixes Critical Issue #1 and Low Issue #13 from code-quality-and-risks.md
The previous implementation called stop() directly from signal context, which invoked g_main_loop_quit() - not async-signal-safe. While GLib tries to handle this, it's technically undefined behavior. Changes: - Add stop_from_signal() GLib callback wrapper - Use g_unix_signal_add() instead of raw signal() - Signals are now handled safely from the GLib main loop Fixes High Issue #5 from code-quality-and-risks.md
srt_startup() was called but srt_cleanup() was never called. This leaks SRT library resources. While not critical for a single-run process, this could cause issues if the code is embedded or run multiple times. Fixes High Issue #2 from code-quality-and-risks.md
The pipeline file descriptor was never closed and the mmap region was never munmapped. This caused minor resource leaks. Changes: - Close pipeline_fd immediately after mmap (mmap keeps its own reference) - Also close fd on empty file error path - Rename 'len' to 'launch_string_len' for clarity and proper type (size_t) - Add munmap() call at program exit Fixes High Issue #3 from code-quality-and-risks.md
Using assert() for runtime errors is problematic: in release builds (NDEBUG), checks are removed entirely and failures silently proceed with incorrect socket configuration. Changes: - Replace all assert(ret == 0) after srt_setsockflag with proper checks - Print descriptive error messages using srt_getlasterror_str() - Return -4 error code for socket option failures - Add -4 case to error reason switch in main() - For SRTO_PEERLATENCY getter, just warn instead of failing (non-critical) Fixes High Issue #4 from code-quality-and-risks.md
CLOCK_MONOTONIC_RAW is Linux-specific. Use POSIX-compliant CLOCK_MONOTONIC for better portability (macOS, BSD). Also: - Replace assert() with proper error handling - Fix potential integer overflow in nanosecond division - Rename variable to 'ts' for clarity Fixes Medium Issue #11 from code-quality-and-risks.md
Add explicit includes for functions used: - <arpa/inet.h> for inet_addr() - <errno.h> for errno (needed for strtol error handling) - <time.h> for nanosleep() (upcoming fix) Previously worked via transitive includes, but this is fragile. Fixes Low Issue #12 from code-quality-and-risks.md
belacoder uses SRTO_RETRANSMITALGO which requires SRT 1.4.0 or later. Add a compile-time check to provide a clear error message instead of confusing compile or link errors with older SRT versions. Fixes Low Issue #15 from code-quality-and-risks.md
usleep() was deprecated in POSIX.1-2008. Use nanosleep() instead for better portability and standards compliance. Fixes Low Issue #16 from code-quality-and-risks.md
Custom min/max macros evaluated arguments multiple times, which could cause bugs if arguments have side effects (e.g., min(++x, y)). GLib's MIN/MAX macros are properly implemented to avoid this issue. Keep lowercase aliases for compatibility with existing code. Fixes Medium Issue #7 from code-quality-and-risks.md
Replace hardcoded numeric values with named constants to improve code readability and make tuning easier. New constant groups: - EMA_* for exponential moving average smoothing factors - RTT_* for RTT tracking parameters - BS_TH* for buffer size threshold multipliers This makes the bitrate control algorithm easier to understand and allows tuning parameters to be adjusted in one place. Fixes Medium Issue #8 from code-quality-and-risks.md
strtol returns 0 on failure which could be confused with valid input. The endptr was ignored, so trailing garbage like "500000abc" was silently accepted. Changes: - Add parse_long() helper with full error checking - Check errno for overflow/underflow - Validate endptr to reject trailing garbage - Allow trailing whitespace/newline (for file parsing) - Range checking built into the helper - Update parse_bitrate(), parse_ip_port(), and CLI parsing Fixes Medium Issue #9 from code-quality-and-risks.md
At maximum bitrate (30 Mbps), calculations are safe with int, but if ABS_MAX_BITRATE is increased, overflow could occur. Use int64_t for intermediate calculations. Also: - Track previous rounded bitrate to avoid redundant encoder updates - Cleaner separation between calculation and update logic Fixes Medium Issue #10 from code-quality-and-risks.md
Replace g_print() and printf() with fprintf(stderr, ...) for consistent logging. Only version output (-v) remains on stdout as that's the expected behavior for version queries. Fixes Low Issue #17 from code-quality-and-risks.md
- Update code-quality-and-risks.md to mark all fixed issues - Update bitrate-control.md to reference new named constants - Add new documentation files: - architecture.md - System overview - dependencies.md - Build and runtime requirements - balancing-algorithms.md - Alternative algorithm designs - rust-go-migration-feasibility.md - Language migration analysis All 14 code issues (1 critical, 4 high, 5 medium, 4 low) have been fixed. 2 issues remain deferred for future structural refactoring.
- architecture.md: Add sections on signal handling and resource management - README.md: Note that SRT 1.4.0+ is enforced at compile time - dependencies.md: Document compile-time SRT version check
- Add forward declaration for read_bitrate_file() to fix implicit declaration warning when called from stall_check() - Add gpointer parameter to connection_housekeeping() to match GSourceFunc signature expected by g_timeout_add()
- Switch from IRLServer/srt to CERALIVE/srt (the recommended fork) - Fix cmake build commands (use out-of-source build) - Remove obsolete branch checkout (master is correct)
- Update all repository references to CERALIVE (srt, srtla, belacoder) - Add network bonding section to README and architecture docs - Move proposal docs to GitHub issues: - code-quality-and-risks.md → Issue #2 - balancing-algorithms.md → Issue #3 - rust-go-migration-feasibility.md → Issue #4 - Clean up stale cross-references in remaining docs - Keep only actual documentation (architecture, bitrate-control, dependencies)
andrescera
added a commit
that referenced
this pull request
Jan 9, 2026
The previous implementation called stop() directly from signal context, which invoked g_main_loop_quit() - not async-signal-safe. While GLib tries to handle this, it's technically undefined behavior. Changes: - Add stop_from_signal() GLib callback wrapper - Use g_unix_signal_add() instead of raw signal() - Signals are now handled safely from the GLib main loop Fixes High Issue #5 from code-quality-and-risks.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR brings belacoder up to quality standards with comprehensive fixes to signal handling, error handling, resource management, and documentation. It also migrates all references to use CERALIVE forks (srt, srtla, belacoder).
Code Quality Improvements (
belacoder.c)🔒 Signal Handling Safety
volatile sig_atomic_t reload_bitrate_flaginstead of calling unsafe functions from signal handlerg_unix_signal_add()for SIGTERM/SIGINT to properly integrate with GLib main loopassert()with properifchecks andfprintf(stderr, ...)error messagesparse_long()helper with full errno, endptr, and range validation#errorif SRT < 1.4.0 (required forSRTO_RETRANSMITALGO)🧹 Resource Management
srt_cleanup()at end of mainmunmap()for pipeline file andclose()for file descriptor📐 Code Quality
#define(EMA_SLOW, RTT_MIN_DRIFT, BS_TH3_MULT, etc.)MIN()/MAX()instead of custom macros (prevents double evaluation)CLOCK_MONOTONICinstead of Linux-specificCLOCK_MONOTONIC_RAWusleep()withnanosleep()int64_tfor bitrate calculationsfprintf(stderr, ...)<signal.h>,<time.h>, etc.)Build System
Dockerfile
Documentation
Repository References Updated
BELABOX/srt→CERALIVE/srtBELABOX/srtla→CERALIVE/srtlaBELABOX/belacoder→CERALIVE/belacoderNetwork Bonding
Moved Proposals to GitHub Issues
docs/code-quality-and-risks.mddocs/balancing-algorithms.mddocs/rust-go-migration-feasibility.mdFinal Documentation Structure
Commits (19 total)
Related Issues