Skip to content

(master) xnest: replace global arrays by devPrivate's - #3719

Open
metux wants to merge 1 commit into
masterfrom
pr/master-xnest-replace-global-arrays-by-devprivate-s-_2026-09-22_19-10-30
Open

metux wants to merge 1 commit into
masterfrom
pr/master-xnest-replace-global-arrays-by-devprivate-s-_2026-09-22_19-10-30

Conversation

@metux

@metux metux commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

instead of having several global arrays for storing additional
per-screen private data, use devPrivate infrastructure.

that way also decoupling that data from sequential screen numbers.

Signed-off-by: Enrico Weigelt, metux IT consult info@metux.net

@metux metux self-assigned this Sep 22, 2026
@metux
metux requested a review from a team September 22, 2026 17:12
metux added a commit that referenced this pull request Sep 22, 2026
instead of having several global arrays for storing additional
per-screen private data, use devPrivate infrastructure.

that way also decoupling that data from sequential screen numbers.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
PR: #3719
@metux metux added the bot-review-changes-requested Automated bot review requested changes (blocking finding) label Sep 22, 2026
metux added a commit that referenced this pull request Sep 23, 2026
instead of having several global arrays for storing additional
per-screen private data, use devPrivate infrastructure.

that way also decoupling that data from sequential screen numbers.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
PR: #3719
@metux
metux force-pushed the pr/master-xnest-replace-global-arrays-by-devprivate-s-_2026-09-22_19-10-30 branch 4 times, most recently from 33f2685 to fb77312 Compare September 23, 2026 14:27
@metux

metux commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Automated review — generated by Starfleet ship Enterprise (model: heavy-model) on behalf of @metux. Not a human review.

Verdict: pass — the blocking bug is fixed; CI fully green including the Xnest runtime smoke test.

What changed since the previous review

  • xnestScreenPrivateKeyRec is now registered (dixRegisterPrivateKey(&…, PRIVATE_SCREEN, 0),
    hw/xnest/Screen.c, in xnestOpenScreen).
  • The priv is allocated before any use (calloc + dixSetPrivate at the top of xnestOpenScreen) and
    freed in xnestCloseScreen.
  • xnestScreen() and xnestSaveScreen() now NULL-guard the priv.
  • Dead xnestSetScreenPrivate() removed; shared layout moved to xnest-screen_priv.h with a
    static inline getter; new eventmask.c/xnest-eventmask.h wired into meson.build.

Verification

  • CI: 20 pass / 0 fail, including xserver-build-ubuntu (builds -Dxnest=true -Dwerror=true
    and runs the X-NAMESPACE/XNS smoke test that actually starts Xnest, build-xserver.yml:150-185)
    — so startup-time registration/alloc is exercised at runtime, not just compiled.
  • Pattern matches the canonical PRIVATE_SCREEN idiom (register size 0 + manual dixSetPrivate,
    as in dbe/randr/picture).
  • No _X_EXPORT/struct-layout change → no driver-ABI impact (abi-changes-check passes).

Advisory (non-blocking)

  1. eventmask.c (new): xnestUpdateEventMask() guards if (walkScreen) but dereferences
    screenPriv->defaultWindow without checking screenPriv — inconsistent with the guarded
    xnestScreen()/xnestSaveScreen(). In the normal lifecycle the priv is always set, but a
    teardown/regen window would NULL-deref. Suggest if (walkScreen && screenPriv).
  2. Header hygiene: xnest-screen_priv.h uses DevPrivateKeyRec, dixLookupPrivate() and
    ->devPrivates but only includes screenint.h; a standalone include fails (verified). It
    compiles only because each consumer .c happens to include privates.h/scrnintstr.h first.
    Add #include "privates.h" (+ scrnintstr.h) to make the header self-contained.
  3. Teardown: xnestCloseScreen() frees the priv but doesn't clear it (dixSetPrivate(..., NULL)) —
    any late callback after close would use a dangling pointer. Minor for the current single-shot
    lifecycle.

Rules

  • Backport: N/A — refactor, targets master.
  • Driver-ABI: no impact.

@metux metux added bot-review-passed Automated bot review found no blocking issues and removed bot-review-changes-requested Automated bot review requested changes (blocking finding) labels Sep 23, 2026
instead of having several global arrays for storing additional
per-screen private data, use devPrivate infrastructure.

that way also decoupling that data from sequential screen numbers.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
metux force-pushed the pr/master-xnest-replace-global-arrays-by-devprivate-s-_2026-09-22_19-10-30 branch from fb77312 to 2aeb07d Compare September 23, 2026 14:57
@metux

metux commented Sep 23, 2026

Copy link
Copy Markdown
Contributor Author

🤖 Automated review — generated by Starfleet ship Enterprise (model: heavy-model) on behalf of @metux. Not a human review.

Verdict: pass (review update) — all prior advisories addressed, CI re-running but code-correct.

Update since previous review

  • eventmask NULL-guard fixed: xnestUpdateEventMask() now dereferences screenPriv only
    if (screenPriv) (was: if (walkScreen)).
  • Header is now self-contained: xnest-screen_priv.h includes include/privates.h +
    include/scrnintstr.h, so DevPrivateKeyRec, dixLookupPrivate(), ->devPrivates resolve
    regardless of include order (verified).
  • Bonus cleanup: the duplicated per-screen eventmask loops in Keyboard.c/Pointer.c now
    call the shared xnestUpdateEventMask() helper (eventmask.c).
  • Teardown (free() in xnestCloseScreen) is a non-issue — the ScreenRec is torn down right
    after close, so no stale-pointer window.

Still-valid review points (from previous round)

  • Key registered (dixRegisterPrivateKey, PRIVATE_SCREEN, 0) + priv calloc'd/dixSetPrivate'd
    at the top of xnestOpenScreen(), before any consumer runs; freed in xnestCloseScreen().
  • xnestScreen() and xnestSaveScreen() NULL-guard the priv.
  • Remaining unguarded derefs (Color/Cursor/GCOps/Pixmap, xnestWindowParent macro) are only
    reachable on live, fully-open xnest screens where the priv is guaranteed to be set — consistent
    with the documented lifecycle.
  • Canonical PRIVATE_SCREEN idiom; no _X_EXPORT/struct-layout change → no driver-ABI impact.

CI

Re-running on this revision (previous revision: 20 pass / 0 fail incl. the ubuntu lane that builds
-Dxnest=true -Dwerror=true and starts Xnest in the X-NAMESPACE smoke test).

  • Backport: N/A — refactor, targets master.
  • Driver-ABI: no impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-review-passed Automated bot review found no blocking issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant