Savers: always run gl1_init(), never trust a stale g_started

port_new opened with "if (g_started) return 1". That was defensible while
gl1's state was whatever the previous saver left behind, but nativeInit now
calls gl1_lost() before every create, so gl1 is guaranteed DOWN on entry.
Reporting success there would hand the caller a saver with no shim under it.

So the gl1_lost() added alongside the leak fix did not just close the leak,
it turned that early return from redundant into wrong -- its safety rested
on a call-graph property that same change removed. All six ports now tear
down a stale run through port_free and always run gl1_init(). Each needs a
forward declaration of port_free, which is defined below port_new.

No behaviour change on any path reachable today: port_free always clears
g_started, so the guard never fires. The point is that a saver added later,
or an upstream cleanup that returns early, should fail in its own saver
rather than poison the next one -- the same reason port_free tears gl1 down
unconditionally.

Adapted from the ARMSX2 change (be674a64c5).
This commit is contained in:
jpolo1224
2026-08-23 02:44:04 -04:00
parent 24951bb863
commit 78650f32d8
6 changed files with 72 additions and 6 deletions
@@ -23,10 +23,21 @@ namespace { bool g_started = false; }
extern "C" {
/* Defined below. port_new tears a stale run down through it rather than trusting
* g_started, so the declaration has to come first. */
void flux_port_free();
/* preset is 1..6, matching the saver's own DEFAULTS1..DEFAULTS6. */
int flux_port_new(int preset)
{
if (g_started) return 1;
/* NOT "if (g_started) return 1": nativeInit calls gl1_lost() before every create, so
* gl1 is guaranteed DOWN on entry now. Reporting success here would hand the caller a
* saver with no shim under it. That guard was only ever safe because gl1 state
* survived between savers -- which is exactly the property gl1_lost() removed, so it
* went from redundant to wrong. Unreachable today (port_free always clears g_started),
* but the rule is that a stale run is torn down and gl1_init() always runs, rather
* than that every caller gets the ordering right. */
if (g_started) flux_port_free();
if (!gl1_init()) return 0;
if (preset < 1 || preset > 6) preset = 1;
@@ -24,10 +24,21 @@ namespace { bool g_started = false; }
extern "C" {
/* Defined below. port_new tears a stale run down through it rather than trusting
* g_started, so the declaration has to come first. */
void hyperspace_port_free();
int hyperspace_port_new(int preset)
{
(void) preset; /* No presets upstream; every knob was a registry value. */
if (g_started) return 1;
/* NOT "if (g_started) return 1": nativeInit calls gl1_lost() before every create, so
* gl1 is guaranteed DOWN on entry now. Reporting success here would hand the caller a
* saver with no shim under it. That guard was only ever safe because gl1 state
* survived between savers -- which is exactly the property gl1_lost() removed, so it
* went from redundant to wrong. Unreachable today (port_free always clears g_started),
* but the rule is that a stale run is torn down and gl1_init() always runs, rather
* than that every caller gets the ordering right. */
if (g_started) hyperspace_port_free();
if (!gl1_init()) return 0;
saver_hyperspace::setDefaults();
@@ -23,9 +23,20 @@ int g_preset = 1;
extern "C" {
/* Defined below. port_new tears a stale run down through it rather than trusting
* g_started, so the declaration has to come first. */
void lattice_port_free();
int lattice_port_new(int preset)
{
if (g_started) return 1;
/* NOT "if (g_started) return 1": nativeInit calls gl1_lost() before every create, so
* gl1 is guaranteed DOWN on entry now. Reporting success here would hand the caller a
* saver with no shim under it. That guard was only ever safe because gl1 state
* survived between savers -- which is exactly the property gl1_lost() removed, so it
* went from redundant to wrong. Unreachable today (port_free always clears g_started),
* but the rule is that a stale run is torn down and gl1_init() always runs, rather
* than that every caller gets the ordering right. */
if (g_started) lattice_port_free();
if (!gl1_init()) return 0;
g_preset = (preset >= 1 && preset <= 6) ? preset : 1;
@@ -16,9 +16,20 @@ namespace { bool g_started = false; }
extern "C" {
/* Defined below. port_new tears a stale run down through it rather than trusting
* g_started, so the declaration has to come first. */
void plasma_port_free();
int plasma_port_new(int preset)
{
if (g_started) return 1;
/* NOT "if (g_started) return 1": nativeInit calls gl1_lost() before every create, so
* gl1 is guaranteed DOWN on entry now. Reporting success here would hand the caller a
* saver with no shim under it. That guard was only ever safe because gl1 state
* survived between savers -- which is exactly the property gl1_lost() removed, so it
* went from redundant to wrong. Unreachable today (port_free always clears g_started),
* but the rule is that a stale run is torn down and gl1_init() always runs, rather
* than that every caller gets the ordering right. */
if (g_started) plasma_port_free();
if (!gl1_init()) return 0;
saver_plasma::setDefaults();
@@ -26,10 +26,21 @@ bool g_started = false;
extern "C" {
/* Defined below. port_new tears a stale run down through it rather than trusting
* g_started, so the declaration has to come first. */
void skyrocket_port_free();
int skyrocket_port_new(int preset)
{
(void) preset; /* No presets upstream; every knob was a registry value. */
if (g_started) return 1;
/* NOT "if (g_started) return 1": nativeInit calls gl1_lost() before every create, so
* gl1 is guaranteed DOWN on entry now. Reporting success here would hand the caller a
* saver with no shim under it. That guard was only ever safe because gl1 state
* survived between savers -- which is exactly the property gl1_lost() removed, so it
* went from redundant to wrong. Unreachable today (port_free always clears g_started),
* but the rule is that a stale run is torn down and gl1_init() always runs, rather
* than that every caller gets the ordering right. */
if (g_started) skyrocket_port_free();
if (!gl1_init()) return 0;
saver_skyrocket::setDefaults();
@@ -15,11 +15,22 @@ namespace { bool g_started = false; }
extern "C" {
/* Defined below. port_new tears a stale run down through it rather than trusting
* g_started, so the declaration has to come first. */
void solarwinds_port_free();
/* preset is 1..6 from the UI. Upstream's DEFAULTS1..DEFAULTS6 is a zero-based ENUM here, not
* the 1-based #defines Flux uses, so the UI value is shifted down. */
int solarwinds_port_new(int preset)
{
if (g_started) return 1;
/* NOT "if (g_started) return 1": nativeInit calls gl1_lost() before every create, so
* gl1 is guaranteed DOWN on entry now. Reporting success here would hand the caller a
* saver with no shim under it. That guard was only ever safe because gl1 state
* survived between savers -- which is exactly the property gl1_lost() removed, so it
* went from redundant to wrong. Unreachable today (port_free always clears g_started),
* but the rule is that a stale run is torn down and gl1_init() always runs, rather
* than that every caller gets the ordering right. */
if (g_started) solarwinds_port_free();
if (!gl1_init()) return 0;
if (preset < 1 || preset > 6) preset = 1;