Skip to content

Reduce duplicated code - #243

Merged
jserv merged 6 commits into
sysprog21:mainfrom
henrybear327:cleanup/centralize-helpers
Jul 23, 2026
Merged

Reduce duplicated code#243
jserv merged 6 commits into
sysprog21:mainfrom
henrybear327:cleanup/centralize-helpers

Conversation

@henrybear327

@henrybear327 henrybear327 commented Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Removes six instances of duplicated logic across the codebase,
folding each into a single shared definition. No behavior changes except where a
duplicate copy had a latent bug (see the FUSE write fix below).

Motivation

Several small utilities — a hex-nibble decoder, a counted path-component copy,
fd-flag manipulation, 2 MiB alignment, and a write_all loop — had been
reimplemented independently in two or more files. Each copy is a place a bug can
hide or the two can drift apart (one already had). Consolidating them gives every
call site one definition to read and maintain.

Changes

Each commit is a self-contained consolidation:

  • Fail materialized FUSE writes on zero progressfuse_write_all looped
    while (len > 0) and, on a write() returning 0, advanced by nothing and spun
    forever instead of erroring. Materializing a FUSE file for exec could hang the
    vCPU thread. Replaced with the shared write_all, which treats an unexpected
    zero-byte write as EIO.
  • Drop sidecar_write_all for the shared write_all — removed a second copy
    of utils.h's write_all with identical short-write / EINTR handling.
  • Use the ALIGN_2MIB macros in the Rosetta loaderalign2m_up /
    align2m_down duplicated ALIGN_2MIB_UP / ALIGN_2MIB_DOWN, and two more
    sites open-coded the & ~(BLOCK_2MIB - 1) mask. Now 2 MiB rounding has one
    definition.
  • Use the fd flag helpers instead of open-coding fcntl — the FIONBIO
    handler open-coded an F_GETFL / modify / F_SETFL sequence, and two
    pty-keepalive paths open-coded F_GETFD / F_SETFD | FD_CLOEXEC. Replaced
    with the existing fd_update_status_flag and fd_set_cloexec.
  • Add path_component_copy for counted path components — five component
    walks in path.c and sidecar.c open-coded the same NAME_MAX+1 buffer /
    ENAMETOOLONG check / memcpy + NUL-terminate block (with the bound check
    spelled two different ways). Folded into a path_component_copy helper next to
    path_next_component.
  • Share hex_nibble as the companion to bytes_to_hexgdbstub-rsp.c
    (hex_val) and sidecar.c (hex_nibble) each carried the same hex-digit
    decode ladder, the inverse of the existing bytes_to_hex encoder. Moved to
    utils.h so both directions of the codec live together.

Summary by cubic

Consolidates six duplicated helpers into shared utilities, reducing drift and simplifying maintenance. Also fixes a vCPU hang by failing FUSE materialization on zero‑progress writes; no other behavior changes.

  • Bug Fixes

    • Materialized FUSE writes now error on zero‑byte progress (EIO) instead of spinning forever.
  • Refactors

    • Use ALIGN_2MIB_UP/ALIGN_2MIB_DOWN in Rosetta; remove open‑coded masks.
    • Replace open‑coded fcntl sequences with fd_set_cloexec and fd_update_status_flag.
    • Add path_component_copy and use it for counted components in path.c and sidecar.c.
    • Move hex_nibble to utils.h; reuse in gdbstub-rsp and sidecar.
    • Remove sidecar_write_all and call shared write_all.

Written for commit 76d9378. Summary will update on new commits.

Review in cubic

fuse_write_all looped "while (len > 0)" and, on a write() that returned
0, advanced by nothing and spun forever instead of reporting an error.
Materializing a FUSE file for exec could therefore hang the vCPU thread.

Use write_all, which treats an unexpected zero-byte write as EIO. This
also removes a private copy of the same write loop.
sidecar_write_all was a second copy of utils.h's write_all with the same
short-write and EINTR handling. Call write_all instead.
align2m_up and align2m_down duplicated the ALIGN_2MIB_UP and
ALIGN_2MIB_DOWN macros in utils.h, and two more sites open-coded the
"& ~(BLOCK_2MIB - 1)" mask. Use the macros so 2 MiB rounding has one
definition.
The FIONBIO handler open-coded an F_GETFL / modify / F_SETFL sequence,
and two pty-keepalive paths open-coded F_GETFD / F_SETFD | FD_CLOEXEC.
utils.h already provides fd_update_status_flag and fd_set_cloexec for
exactly these; use them.
Five component walks in path.c and sidecar.c open-coded the same block:
declare a NAME_MAX+1 buffer, reject an overlong component with
ENAMETOOLONG, then memcpy the counted component and NUL-terminate. The
bound check was even spelled two ways ("len > NAME_MAX" and
"comp_len >= sizeof(buf)").

Fold that into a path_component_copy helper next to path_next_component,
which produces the counted, non-NUL-terminated components these sites
consume. Callers keep their own cleanup on the error path and gate it on
the helper's return.
gdbstub-rsp.c (hex_val) and sidecar.c (hex_nibble) each carried the same
hex-digit decode ladder, the inverse of the bytes_to_hex encoder already
in utils.h. Move it there as hex_nibble and drop both copies. gdb_hex_encode
already uses bytes_to_hex, so the two now share both directions of the codec.
@henrybear327
henrybear327 requested a review from jserv July 23, 2026 20:59
cubic-dev-ai[bot]

This comment was marked as resolved.

@jserv
jserv merged commit 94e3f2d into sysprog21:main Jul 23, 2026
10 checks passed
@henrybear327
henrybear327 deleted the cleanup/centralize-helpers branch July 24, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants