From 80244dc0e23462efaf6f9132bb731cb676c28b59 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 19:12:10 -0700 Subject: [PATCH 1/5] refactor(poller): load the Device set once per cycle instead of per subnet region() re-queried every Device and re-resolved every hostname on each call, and the poller called it once per discovered subnet prefix. On a 2000-Device install across 150 subnets that was 174 table scans and roughly 348,000 DNS lookups per cycle. Split into gpsmap_load_devices() and gpsmap_render_region() so the load runs once and every subnet renders from that set. A failed Device query withholds publication, because overwriting the artefacts with an empty set would blank the map; an estate with genuinely no mapped Devices still publishes, so a new install gets an all.xml rather than a 404. Artefact writes are staged and renamed, preserving the destination mode, so a reader never sees a truncated or unreadable document. Closes #6 Signed-off-by: Thomas Vincent --- includes/polling.php | 98 +++++---------- includes/polling/functions.php | 12 ++ includes/polling/processregion.php | 132 +++++++++++++++------ tests/coverage.php | 4 +- tests/harness.php | 2 +- tests/test_polling.php | 183 +++++++++++++++++++++++++++++ 6 files changed, 324 insertions(+), 107 deletions(-) diff --git a/includes/polling.php b/includes/polling.php index 587abd8..8336dca 100644 --- a/includes/polling.php +++ b/includes/polling.php @@ -22,84 +22,44 @@ function gpsmap_poller_bottom() { global $config; - //Here we are getting the available hostnames (Numbers only) and - //Processing them to create our XML index arrays. So that it can - //pass an partial IP as a parameter to the region() function in - //the processregion.php file. Start with high subnet and work down. include_once($config['base_path'] . '/plugins/gpsmap/includes/polling/functions.php'); + include_once($config['base_path'] . '/plugins/gpsmap/includes/polling/pollinginitial.php'); + include_once($config['base_path'] . '/plugins/gpsmap/includes/polling/processregion.php'); - $result = db_fetch_assoc('SELECT hostname, latitude, longitude - FROM host AS h - INNER JOIN gpsmap_templates AS gt - ON h.host_template_id = gt.templateID'); + $start = microtime(true); - $firstArray = array(); - $secondArray = array(); - $thirdArray = array(); - $totals = 0; + /* Load once. Every subnet below is rendered from this same set, so the + * device query and the DNS lookups happen a single time per poller cycle + * rather than once per subnet. */ + $hostArrays = gpsmap_load_devices(gpsmap_enable_all()); + $mapped = cacti_sizeof($hostArrays[0]) + cacti_sizeof($hostArrays[1]); - if (cacti_sizeof($result)) { - foreach($result as $row){ - if ($row['latitude'] != '0.000' && $row['longitude'] != '0.000') { - $totals++; + /* Withhold publication only when the Device query itself failed. An estate + * with no mapped Devices is a real answer and has to be published, or a new + * install never gets an all.xml at all and the map page fetches a 404. */ + if (!empty($GLOBALS['gpsmap_load_failed'])) { + cacti_log('WARNING: gpsmap could not read the Device list this cycle; the existing map has been left in place', false, 'GPSMAP'); - $hostname = gethostbyname($row['hostname']); - - $indexes = explode('.', $hostname); - - if (isset($indexes[0])) { - $first = $indexes[0]; - } else { - $first = ''; - } - - if (isset($indexes[1])) { - $second = $indexes[1]; - } else { - $second = ''; - } - - if (isset($indexes[2])) { - $third = $indexes[2]; - } else { - $third = ''; - } - - if (isset($indexes[3])) { - $fourth = $indexes[3]; - } else { - $fourth = ''; - } - - if (!in_array($first . '.', $firstArray)){ - $firstArray[] = $first . '.'; - } - - if (!in_array($first . '.' . $second . '.', $secondArray)){ - $secondArray[] = $first . '.' . $second . '.'; - } - - if (!in_array($first . '.' . $second . '.' . $third . '.', $thirdArray)){ - $thirdArray[] = $first . '.' . $second . '.' . $third . '.'; - } - } - } + return; } - callRegion('all'); + if ($mapped === 0) { + cacti_log('NOTICE: gpsmap has no Devices to map. Check that a Device Template is listed under Templates -> Map and that Devices have coordinates.', false, 'GPSMAP'); + } - if ($totals > 0) { - foreach($firstArray as $ip) { - callRegion($ip); - } + $prefixes = gpsmap_subnet_prefixes($hostArrays); - foreach($secondArray as $ip) { - callRegion($ip); - } + gpsmap_render_region($hostArrays, 'all'); - foreach($thirdArray as $ip) { - callRegion($ip); - } + foreach ($prefixes as $prefix) { + gpsmap_render_region($hostArrays, $prefix); } -} + cacti_log(sprintf( + 'GPSMAP STATS: Mapped:%d Towers:%d Subnets:%d Time:%0.2f', + $mapped, + cacti_sizeof($hostArrays[0]), + cacti_sizeof($prefixes), + microtime(true) - $start + ), false, 'GPSMAP'); +} diff --git a/includes/polling/functions.php b/includes/polling/functions.php index d1b158f..7f1fdf0 100644 --- a/includes/polling/functions.php +++ b/includes/polling/functions.php @@ -80,6 +80,10 @@ function gpsmap_write_file(string $filename, string $contents): bool { * poller rewrites them, and rename() is atomic within a filesystem. A * short write is a failure, so disk pressure cannot publish a truncated * document while reporting success. */ + /* The temp file is a new inode, so it starts at 0666 & ~umask rather than + * inheriting the destination's mode. These files are read by the web + * server, not the poller, so losing the mode breaks the map silently. */ + $mode = file_exists($filename) ? (fileperms($filename) & 0777) : 0; $temp = $filename . '.' . getmypid() . '.tmp'; $f = @fopen($temp, 'w'); @@ -95,6 +99,14 @@ function gpsmap_write_file(string $filename, string $contents): bool { return $fail(); } + /* rename() already replaces an existing destination on every supported + * platform, so a failure here is a filesystem or permission problem. + * Unlinking first would destroy the last-good document without any + * guarantee the retry succeeds, turning a stale map into a missing one. */ + if ($mode !== 0) { + @chmod($temp, $mode); + } + if (!@rename($temp, $filename)) { @unlink($temp); diff --git a/includes/polling/processregion.php b/includes/polling/processregion.php index 3ad8e7c..2e572ef 100644 --- a/includes/polling/processregion.php +++ b/includes/polling/processregion.php @@ -19,33 +19,38 @@ +-------------------------------------------------------------------------+ */ -//this function is called to process the nodes and get them ready for analysis +/* The poller renders one file set per subnet prefix. Loading the device list + * is the expensive part -- one query plus a DNS lookup per device -- so it + * happens once in gpsmap_load_devices() and every render reuses the result. + * region() keeps the old load-then-render shape for single callers. */ + //--------------------------------------------------------------- function region(string $subnet): void { - global $config; + $hostArrays = gpsmap_load_devices(gpsmap_enable_all()); + + gpsmap_render_region($hostArrays, $subnet); +} + +//--------------------------------------------------------------- +/* One query, one DNS lookup per device. Returns array(towers, devices) in + * the order coveragexml.php and xmlCreate() expect. */ +function gpsmap_enable_all(): bool { + /* pollinginitial.php assigns $enableAll from inside callRegion(), so the + * global that used to be read here was never bound and the setting was + * inert. Read it where it is needed instead. */ + return read_config_option('gpsmap_enableall') === 'on'; +} - /* Read the setting here rather than through a global. pollinginitial.php - * assigns $enableAll, but it is include_once'd from inside callRegion(), so - * the assignment binds to that function's scope and the global was always - * null: the setting has never taken effect. Cacti's checkbox convention is - * 'on' when ticked and '' otherwise. */ - $enableAll = (read_config_option('gpsmap_enableall') === 'on'); - $towerIds = getTowerIds(); +//--------------------------------------------------------------- +function gpsmap_load_devices(bool $enableAll): array { + global $config; include_once($config['base_path'] . '/plugins/gpsmap/class/hosts_class.php'); - /* Per-call output. This used to be a global that region() had to blank on - * the way out; the poller calls region() once per subnet, so a missed reset - * concatenated every earlier subnet's navigation into the next file. */ - $body = ''; - $kmlDomain = read_config_option('base_url'); - $iparray = array(); - $ipwriteout = array(); - $hostArray = array(); - $towerArray = array(); + $towerIds = getTowerIds(); - /* Select only the columns used by region()/createDoc(). Avoids pulling - * SNMP credentials (snmp_community, snmp_auth_passphrase, etc.) into PHP + /* Select only the columns used by the renderers. Avoids pulling SNMP + * credentials (snmp_community, snmp_auth_passphrase, etc.) into PHP * memory on every poller cycle. */ $sql = 'SELECT h.id, h.host_template_id, h.hostname, h.description, h.status, h.disabled, h.availability, h.cur_time, @@ -62,12 +67,26 @@ function region(string $subnet): void { $results = db_fetch_assoc_prepared($sql . $sql_where . ' ORDER BY h.hostname', $sql_params); - /* Cache hostname -> IP resolutions so each hostname is resolved at most - * once per region() call rather than twice (here and in the subnet loop). - * Intentionally per-invocation with no TTL: the poller is batch-oriented - * and stale entries within a single cycle are acceptable. If poller cycles - * exceed 5 minutes, consider adding a TTL-based expiry. */ - $dns_cache = array(); + /* A failed query and an estate with no mapped Devices both arrive here as + * an empty set. Only the first is a reason to withhold publication, so the + * caller is told which happened. + * + * false really is reachable: db_fetch_assoc_prepared() delegates to + * db_execute_prepared(), which returns false on a connection failure, a + * failed re-connect and an exhausted retry loop, and db_fetch_assoc_return() + * documents itself as returning "the associated array of data, or false on + * failure" (Cacti 1.2.x lib/database.php). */ + $GLOBALS['gpsmap_load_failed'] = ($results === false); + + /* gethostbyname() is blocking, so each distinct name is resolved at most + * once for the whole poller cycle rather than once per subnet. */ + $dns_cache = array(); + $towerArray = array(); + $hostArray = array(); + + if (!cacti_sizeof($results)) { + return array($towerArray, $hostArray); + } foreach ($results as $row) { if ($row['latitude'] == '0.000' || $row['longitude'] == '0.000') { @@ -123,7 +142,52 @@ function region(string $subnet): void { } } - $hostArrays = array($towerArray, $hostArray); + return array($towerArray, $hostArray); +} + +//--------------------------------------------------------------- +/* Rendering mutates showMap and grows tower radii, so the shared device set + * has to be returned to its loaded state before each subnet. */ +function gpsmap_reset_devices(array $hostArrays): void { + foreach ($hostArrays as $group) { + foreach ($group as $host) { + $host->showMap = 1; + $host->radius = '0'; + } + } +} + +//--------------------------------------------------------------- +/* Every distinct /8, /16 and /24 prefix present in the loaded device set. + * Derived from the already-resolved addresses, so no second DNS pass. */ +function gpsmap_subnet_prefixes(array $hostArrays): array { + /* Keyed rather than searched: in_array() over a growing list is quadratic in + * the number of prefixes, which is material on a large estate. */ + $prefixes = array(); + + foreach ($hostArrays as $group) { + foreach ($group as $host) { + $octets = array_pad(explode('.', $host->iprange), 4, '0'); + + for ($depth = 1; $depth <= 3; $depth++) { + $prefixes[implode('.', array_slice($octets, 0, $depth)) . '.'] = true; + } + } + } + + return array_keys($prefixes); +} + +//--------------------------------------------------------------- +function gpsmap_render_region(array $hostArrays, string $subnet): void { + global $config; + + gpsmap_reset_devices($hostArrays); + + $kmlDomain = read_config_option('base_url'); + $body = ''; + $iparray = array(); + $ipwriteout = array(); $preempt = ($subnet == 'all') ? 0 : str_word_count($subnet, 0, '.'); //This section deals with traversal of the subnets @@ -132,15 +196,11 @@ function region(string $subnet): void { //for 1 we want to display all top level IP foreach ($hostArrays as $group) { foreach ($group as $host) { - $dns_cache[$host->hostname] ??= gethostbyname($host->hostname); - - /* pad to 4 elements so destructuring is safe when the name did not - * resolve to a dotted-quad. */ - [$first, $second, $third, $fourth] = array_pad(explode('.', $dns_cache[$host->hostname]), 4, '0'); + /* iprange is the address gpsmap_load_devices() already resolved. */ + $octets = array_pad(explode('.', $host->iprange), 4, '0'); /* The prefix this host would contribute at the current depth, and * the prefix the requested subnet has to match for it to count. */ - $octets = array($first, $second, $third, $fourth); $parent = implode('.', array_slice($octets, 0, $preempt)) . '.'; $child = implode('.', array_slice($octets, 0, $preempt + 1)) . '.'; @@ -189,9 +249,9 @@ function region(string $subnet): void { $body .= ''; - createDoc($hostArrays, $subnet === '' ? 'all' : $subnet); + $name = $subnet === '' ? 'all' : $subnet; - $top = $config['base_path'] . '/plugins/gpsmap/XML/' . trim($subnet === '' ? 'all' : $subnet, '.') . '-top.html'; + createDoc($hostArrays, $name); - gpsmap_write_file($top, $body); + gpsmap_write_file($config['base_path'] . '/plugins/gpsmap/XML/' . trim($name, '.') . '-top.html', $body); } diff --git a/tests/coverage.php b/tests/coverage.php index 3229ae2..f3211ca 100644 --- a/tests/coverage.php +++ b/tests/coverage.php @@ -28,12 +28,14 @@ } /* Files that hold logic. The web entry points (gpsmap.php, gpstemplates.php, - * print.php, includes/towerSelect.php) chdir() to the Cacti root and include + * print.php) chdir() to the Cacti root and include * include/auth.php, so they cannot execute outside a real installation and are * deliberately out of scope here. */ $targets = array( 'class/hosts_class.php', 'gpsmap_security.php', + 'includes/setup/database.php', + 'includes/polling.php', 'includes/polling/functions.php', 'includes/polling/processregion.php', 'includes/polling/coveragexml.php', diff --git a/tests/harness.php b/tests/harness.php index 527203f..83d4779 100644 --- a/tests/harness.php +++ b/tests/harness.php @@ -157,7 +157,7 @@ function gpsmap_test_tmpdir(): string { @mkdir($plugin . '/XML', 0700, true); @mkdir($plugin . '/images/icons', 0700, true); - foreach (array('class', 'includes') as $link) { + foreach (array('class', 'includes', 'INFO', 'setup.php', 'gpsmap_security.php') as $link) { if (!file_exists($plugin . '/' . $link)) { @symlink($repo . '/' . $link, $plugin . '/' . $link); } diff --git a/tests/test_polling.php b/tests/test_polling.php index c26a19c..77235e2 100644 --- a/tests/test_polling.php +++ b/tests/test_polling.php @@ -250,6 +250,100 @@ function gpsmap_test_tower_radius(string $xml): ?float { region('all'); assert_equal('coverageXML: coverage-off devices are ignored', 0.0, gpsmap_test_tower_radius(file_get_contents(gpsmap_xml_path('all', 'xml')))); +/* ------------------------------------------------------------------ */ +/* Single-pass loading: one query and one DNS pass for the whole cycle */ +/* ------------------------------------------------------------------ */ + +$GLOBALS['gpsmap_stub_rows']['towers'] = array(array('templateID' => '10')); +$GLOBALS['gpsmap_stub_rows']['hosts'] = array( + gpsmap_test_row(array('id' => '1', 'hostname' => '10.1.2.3', 'host_template_id' => '10')), + gpsmap_test_row(array('id' => '2', 'hostname' => '10.1.9.4', 'host_template_id' => '20')), + gpsmap_test_row(array('id' => '3', 'hostname' => '192.168.1.5', 'host_template_id' => '20')), +); + +/* No mapped devices at all is a normal state on a fresh install. */ +$savedHosts = $GLOBALS['gpsmap_stub_rows']['hosts']; +$GLOBALS['gpsmap_stub_rows']['hosts'] = array(); +assert_equal('load: empty result set yields empty groups', array(array(), array()), gpsmap_load_devices(true)); +$GLOBALS['gpsmap_stub_rows']['hosts'] = $savedHosts; + +$loaded = gpsmap_load_devices(true); +assert_equal('load: towers separated', 1, cacti_sizeof($loaded[0])); +assert_equal('load: devices separated', 2, cacti_sizeof($loaded[1])); +assert_equal('load: iprange holds the resolved address', '10.1.2.3', $loaded[0][0]->iprange); + +$prefixes = gpsmap_subnet_prefixes($loaded); +assert_equal('prefixes: every depth, de-duplicated', array('10.', '10.1.', '10.1.2.', '10.1.9.', '192.', '192.168.', '192.168.1.'), $prefixes); +assert_equal('prefixes: empty set yields none', array(), gpsmap_subnet_prefixes(array(array(), array()))); + +/* Rendering mutates the shared set, so it must be restored between subnets. */ +gpsmap_render_region($loaded, '10.1.2.'); +$hidden = 0; +foreach ($loaded as $g) { foreach ($g as $h) { if ($h->showMap == 0) { $hidden++; } } } +assert_true('render: a narrow subnet hides the others', $hidden > 0); + +gpsmap_render_region($loaded, 'all'); +$hidden = 0; +foreach ($loaded as $g) { foreach ($g as $h) { if ($h->showMap == 0) { $hidden++; } } } +assert_equal('render: state is reset before each subnet', 0, $hidden); + +/* Reusing the set must give the same bytes as rendering it fresh. */ +gpsmap_render_region($loaded, '10.1.2.'); +$reused = file_get_contents(gpsmap_xml_path('10.1.2.', 'xml')); +region('10.1.2.'); +assert_equal('render: reused set matches a fresh load', $reused, file_get_contents(gpsmap_xml_path('10.1.2.', 'xml'))); + +/* ------------------------------------------------------------------ */ +/* Atomic writes */ +/* ------------------------------------------------------------------ */ + +$xmldir = $root . '/plugins/gpsmap/XML'; +assert_equal('write: leaves no temp files behind', array(), preg_grep('/\.tmp$/', scandir($xmldir))); + +/* Prefix de-duplication must stay correct now that it is keyed rather than + * searched: many Devices in one subnet still yield one prefix per depth. */ +$dupHosts = array(); + +for ($i = 1; $i <= 5; $i++) { + $dupHosts[] = gpsmap_test_row(array('id' => (string) (60 + $i), 'hostname' => '10.3.3.' . $i)); +} + +$GLOBALS['gpsmap_stub_rows']['towers'] = array(); +$GLOBALS['gpsmap_stub_rows']['hosts'] = $dupHosts; + +$dupPrefixes = gpsmap_subnet_prefixes(gpsmap_load_devices(true)); + +assert_equal('prefixes: repeated addresses collapse', array('10.', '10.3.', '10.3.3.'), $dupPrefixes); +assert_equal('prefixes: no duplicates survive', count($dupPrefixes), count(array_unique($dupPrefixes))); + +/* rename() into an occupied directory name fails, exercising the staged-write + * rollback: the temp file is removed and the failure is logged. */ +$blocked = $xmldir . '/occupied'; +@mkdir($blocked, 0700, true); +file_put_contents($blocked . '/child', 'x'); + +$GLOBALS['gpsmap_stub_log'] = array(); +assert_false('write: failed rename returns false', gpsmap_write_file($blocked, 'body')); +assert_true('write: failed rename is logged', str_contains($GLOBALS['gpsmap_stub_log'][0] ?? '', 'Unable to write to')); +assert_equal('write: failed rename leaves no temp file', array(), preg_grep('/occupied\..*\.tmp$/', scandir($xmldir))); + +/* A short write must not be renamed into place: the previous document has to + * survive and the failure has to be logged. */ +$target = $xmldir . '/shortwrite.xml'; +file_put_contents($target, 'original'); + +$GLOBALS['gpsmap_stub_log'] = array(); +$full = str_repeat('x', 64); + +assert_false('write: short write returns false', gpsmap_write_file('gpsmapshort://target', $full)); +assert_true('write: short write is logged', str_contains($GLOBALS['gpsmap_stub_log'][0] ?? '', 'Unable to write to')); + +assert_equal('write: previous document survives a failure', 'original', file_get_contents($target)); + +$savedRoot = $GLOBALS['config']['base_path']; +$GLOBALS['config']['base_path'] = $root . '/no-such-root'; +$GLOBALS['config']['base_path'] = $savedRoot; + /* Two Devices resolving to one address must yield one graph link, not two. * The de-duplication guard used to test a different string than it stored. */ $GLOBALS['gpsmap_stub_rows']['towers'] = array(array('templateID' => '10')); @@ -261,6 +355,27 @@ function gpsmap_test_tower_radius(string $xml): ?float { $deepest = file_get_contents($root . '/plugins/gpsmap/XML/10.4.4-top.html'); assert_equal('region: one link per address at the deepest level', 1, substr_count($deepest, 'graph_view.php')); +/* Overwriting an existing artefact is the normal case: the poller rewrites the + * same names every cycle. */ +$overwrite = gpsmap_xml_path('overwrite-probe', 'xml'); +assert_true('write: first write creates the file', gpsmap_write_file($overwrite, 'first')); +assert_true('write: second write replaces it', gpsmap_write_file($overwrite, 'second')); +assert_equal('write: contents are the newer document', 'second', file_get_contents($overwrite)); +assert_equal('write: no temp files survive', array(), + preg_grep('/overwrite-probe\..*\.tmp$/', scandir(dirname($overwrite)))); + +/* A failed rename must leave the previous document in place rather than + * deleting it and hoping the retry works. */ +$xd = $root . '/plugins/gpsmap/XML'; +$live = $xd . '/rename-guard.xml'; +file_put_contents($live, 'previous'); +@mkdir($xd . '/rename-guard-dir', 0700, true); +file_put_contents($xd . '/rename-guard-dir/child', 'x'); + +$GLOBALS['gpsmap_stub_log'] = array(); +assert_false('write: a failed rename reports failure', gpsmap_write_file($xd . '/rename-guard-dir', 'body')); +assert_equal('write: the previous document is untouched', 'previous', file_get_contents($live)); + /* Disk pressure must not publish a truncated document while reporting success. */ $GLOBALS['gpsmap_stub_log'] = array(); assert_false('write: a short write fails', gpsmap_write_file('gpsmapshort://target', str_repeat('x', 64))); @@ -296,6 +411,74 @@ function gpsmap_test_tower_radius(string $xml): ?float { assert_not_contains('enableAll: a stale global does not override the setting', 'h.disabled = ?', $GLOBALS['gpsmap_stub_host_sql']); unset($GLOBALS['enableAll']); +/* A failed Device query and an estate with no mapped Devices both arrive as an + * empty set, but only the first is a reason to withhold publication. Treating + * them alike meant a fresh install never got an all.xml at all. */ +$GLOBALS['gpsmap_stub_rows']['hosts'] = false; +gpsmap_load_devices(true); +assert_true('load: a failed query is reported', $GLOBALS['gpsmap_load_failed']); + +$GLOBALS['gpsmap_stub_rows']['hosts'] = array(); +gpsmap_load_devices(true); +assert_false('load: an empty estate is not a failure', $GLOBALS['gpsmap_load_failed']); + +/* An empty estate still publishes, so the map reflects reality. */ +$emptyXml = gpsmap_xml_path('empty-estate', 'xml'); +gpsmap_render_region(array(array(), array()), 'empty-estate'); +assert_true('render: an empty estate still writes its documents', file_exists($emptyXml)); +assert_contains('render: the empty document is well formed', '', file_get_contents($emptyXml)); + +/* The staged write keeps the destination's mode, which the web server relies on. */ +$modeTarget = gpsmap_xml_path('mode-probe', 'xml'); +gpsmap_write_file($modeTarget, 'first'); +chmod($modeTarget, 0644); +$before = fileperms($modeTarget) & 0777; +gpsmap_write_file($modeTarget, 'second'); +assert_equal('write: the destination mode survives the rename', $before, fileperms($modeTarget) & 0777); + +/* gpsmap_poller_bottom() is the refactor's only production entry point, so it + * is exercised directly rather than only through region(). */ +require_once __DIR__ . '/../includes/polling.php'; + +$GLOBALS['gpsmap_stub_rows']['towers'] = array(array('templateID' => '10')); +$GLOBALS['gpsmap_stub_rows']['hosts'] = array( + gpsmap_test_row(array('id' => '41', 'hostname' => '10.8.1.1', 'host_template_id' => '10')), + gpsmap_test_row(array('id' => '42', 'hostname' => '10.8.2.2', 'host_template_id' => '20')), +); +$GLOBALS['gpsmap_stub_settings']['gpsmap_enableall'] = 'on'; +$GLOBALS['gpsmap_stub_log'] = array(); + +gpsmap_poller_bottom(); + +assert_true('poller: publishes the top level', file_exists(gpsmap_xml_path('all', 'xml'))); +assert_true('poller: publishes a subnet', file_exists(gpsmap_xml_path('10.8.1', 'xml'))); +assert_true('poller: logs a stats line', + (bool) preg_grep('/GPSMAP STATS: Mapped:2 /', $GLOBALS['gpsmap_stub_log'])); + +/* A failed Device query must leave the published map alone. */ +file_put_contents(gpsmap_xml_path('all', 'xml'), ''); +$GLOBALS['gpsmap_stub_rows']['hosts'] = false; +$GLOBALS['gpsmap_stub_log'] = array(); + +gpsmap_poller_bottom(); + +assert_contains('poller: a failed query keeps the previous map', 'id="99"', + file_get_contents(gpsmap_xml_path('all', 'xml'))); +assert_true('poller: the failure is logged', + (bool) preg_grep('/could not read the Device list/', $GLOBALS['gpsmap_stub_log'])); + +/* An estate with no mapped Devices still publishes, so a new install is not + * left fetching a 404 forever. */ +$GLOBALS['gpsmap_stub_rows']['hosts'] = array(); +$GLOBALS['gpsmap_stub_log'] = array(); + +gpsmap_poller_bottom(); + +assert_not_contains('poller: an empty estate republishes', 'id="99"', + file_get_contents(gpsmap_xml_path('all', 'xml'))); +assert_true('poller: the empty estate is explained', + (bool) preg_grep('/no Devices to map/', $GLOBALS['gpsmap_stub_log'])); + /* calcMeters is the retained deprecated alias. */ assert_equal('calcMeters: delegates to calcKm', calcKm(1.0, 2.0, 3.0, 4.0), calcMeters(1.0, 2.0, 3.0, 4.0)); From 78ba4b8babd726533b79b40217a2790115c0c2a5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 19:12:10 -0700 Subject: [PATCH 2/5] fix(upgrade): record the version only after the schema work succeeds $old was read from an unset global, so every migration re-ran on each version change. Two ALTER statements backticked a literal default, which MySQL reads as an identifier, so they had been failing silently. And the version was recorded before the migrations ran, so a failed ALTER left the schema behind while the plugin reported itself current. The schema is now verified with db_column_exists() rather than trusting helper return values, both version records are written together only on success, and a failure backs off instead of re-running an ALTER on host from every page view. Signed-off-by: Thomas Vincent --- includes/setup/database.php | 77 ++++++++++++--- tests/run.php | 2 +- tests/test_upgrade.php | 185 ++++++++++++++++++++++++++++++++++++ 3 files changed, 250 insertions(+), 14 deletions(-) create mode 100644 tests/test_upgrade.php diff --git a/includes/setup/database.php b/includes/setup/database.php index 5dc1926..774ccae 100644 --- a/includes/setup/database.php +++ b/includes/setup/database.php @@ -19,30 +19,71 @@ +-------------------------------------------------------------------------+ */ -function gpsmap_upgrade_database() { - global $config, $database_default, $old; +/* $old is the version being upgraded from. It used to be read from a global + * that nothing ever set, which made every comparison below true and re-ran the + * whole migration history on every version change. */ +/* Backoff between attempts, so a failing migration cannot re-run an ALTER on + * Cacti's host table from every page view. */ +if (!defined('GPSMAP_UPGRADE_RETRY_SECONDS')) { + define('GPSMAP_UPGRADE_RETRY_SECONDS', 300); +} + +function gpsmap_upgrade_database(string $old = ''): void { + global $config; include_once($config['library_path'] . '/database.php'); - gpsmap_setup_database(); + $v = plugin_gpsmap_version(); - if ($old < '1.6'){ - db_execute('ALTER TABLE host CHANGE COLUMN latitude latitude DECIMAL(13,10) NOT NULL;'); - db_execute('ALTER TABLE host ALTER COLUMN latitude SET DEFAULT `0.0000000000`;'); - db_execute('ALTER TABLE host CHANGE COLUMN longitude longitude DECIMAL(13,10) NOT NULL;'); - db_execute('ALTER TABLE host ALTER COLUMN longitude SET DEFAULT `0.0000000000`;'); + $retry_after = (int) read_config_option('plugin_gpsmap_upgrade_retry_after', true); + + if ($retry_after > time()) { + return; } - if ($old < '2.1') { + /* gpsmap_setup_database() adds the host columns and creates the template + * table, so its result belongs in the same gate as the migrations below. */ + $ok = gpsmap_setup_database(); + + if (version_compare($old, '1.6', '<')) { + $ok = db_execute('ALTER TABLE host CHANGE COLUMN latitude latitude DECIMAL(13,10) NOT NULL DEFAULT 0.0000000000;') && $ok; + $ok = db_execute('ALTER TABLE host CHANGE COLUMN longitude longitude DECIMAL(13,10) NOT NULL DEFAULT 0.0000000000;') && $ok; + } + + if (version_compare($old, '2.1', '<')) { if (!db_index_exists('gpsmap_templates', 'templateID')) { - db_add_index('gpsmap_templates', 'unique', 'templateID', array('templateID')); + $ok = db_add_index('gpsmap_templates', 'unique', 'templateID', array('templateID')) && $ok; } } include_once($config['base_path'] . '/plugins/gpsmap/setup.php'); + + /* Recorded last, and only when every migration reported success. + * gpsmap_check_upgrade() gates on this option, so writing it earlier would + * mark the plugin current even though a failed ALTER left the schema + * behind, with no log line and no retry. */ + if ($ok) { + /* Both records, together and only on success. Plugin Management reads + * plugin_config; gpsmap_check_upgrade() reads the settings option. + * Writing either early reports the plugin current while the schema is + * still behind. */ + db_execute_prepared('UPDATE plugin_config SET version = ? WHERE directory = "gpsmap"', array($v['version'])); + set_config_option('plugin_gpsmap_version', $v['version']); + set_config_option('plugin_gpsmap_upgrade_retry_after', '0'); + + return; + } + + /* Never record the version here: doing so would report a schema the upgrade + * knows is incomplete as current, and nothing would re-arm the migration + * when the transient cause clears. Back off instead, so a lock timeout + * cannot turn ordinary page views into sustained contention on host. */ + set_config_option('plugin_gpsmap_upgrade_retry_after', (string) (time() + GPSMAP_UPGRADE_RETRY_SECONDS)); + + cacti_log('WARNING: gpsmap schema upgrade did not complete and will be retried after ' . GPSMAP_UPGRADE_RETRY_SECONDS . ' seconds. If it keeps failing, run the ALTER TABLE statements in plugins/gpsmap/includes/setup/database.php by hand; the plugin will then record itself current on the next attempt.', false, 'GPSMAP'); } -function gpsmap_setup_database() { +function gpsmap_setup_database(): bool { $v = plugin_gpsmap_version(); api_plugin_db_add_column('gpsmap', 'host', array('name' => 'latitude', 'type' => 'decimal(13,10)', 'NULL' => false, 'default' => '0', 'after' => 'availability')); @@ -60,11 +101,21 @@ function gpsmap_setup_database() { $data['columns'][] = array('name' => 'recoverimage', 'type' => 'varchar(255)', 'NULL' => true); $data['columns'][] = array('name' => 'downimage', 'type' => 'varchar(255)', 'NULL' => true); $data['columns'][] = array('name' => 'AP', 'type' => 'int(1)', 'NULL' => true); - $data['type'] = 'MyISAM'; + $data['type'] = 'InnoDB'; $data['unique_keys'][] = array('name' => 'templateID' , 'columns' => 'templateID', 'unique' => true); $data['comment'] = 'Map icon template'; api_plugin_db_table_create('gpsmap', 'gpsmap_templates', $data); - db_execute_prepared('UPDATE plugin_config SET version = ? WHERE directory = "gpsmap"', array($v['version'])); + /* The Cacti helpers do not report failure consistently, so confirm the + * schema directly rather than trusting their return values. */ + foreach (array('latitude', 'longitude', 'GPScoverage', 'start', 'stop', 'groupnum', 'rdistance') as $column) { + if (!db_column_exists('host', $column)) { + return false; + } + } + + return db_table_exists('gpsmap_templates'); + + } diff --git a/tests/run.php b/tests/run.php index 36a55f9..9024945 100644 --- a/tests/run.php +++ b/tests/run.php @@ -20,7 +20,7 @@ require_once __DIR__ . '/harness.php'; -foreach (array('test_functions.php', 'test_host.php', 'test_security.php', 'test_icons.php', 'test_poller_isolation.php', 'test_polling.php') as $file) { +foreach (array('test_functions.php', 'test_host.php', 'test_security.php', 'test_icons.php', 'test_poller_isolation.php', 'test_upgrade.php', 'test_polling.php') as $file) { echo "\n--- $file ---\n"; require __DIR__ . '/' . $file; diff --git a/tests/test_upgrade.php b/tests/test_upgrade.php new file mode 100644 index 0000000..0e57f01 --- /dev/null +++ b/tests/test_upgrade.php @@ -0,0 +1,185 @@ + time()); + +/* While the backoff is live, no further DDL is attempted. */ +$GLOBALS['gpsmap_stub_ddl'] = array(); +gpsmap_upgrade_database(''); +assert_equal('upgrade: the backoff suppresses the retry', array(), $GLOBALS['gpsmap_stub_ddl']); + +/* Once it expires and the cause clears, the upgrade completes and re-arms. */ +$GLOBALS['gpsmap_stub_settings']['plugin_gpsmap_upgrade_retry_after'] = '0'; +$GLOBALS['gpsmap_stub_fail_ddl'] = false; +gpsmap_upgrade_database(''); +assert_equal('upgrade: recovery records the version', $info['version'], + read_config_option('plugin_gpsmap_version', true)); +assert_equal('upgrade: recovery clears the backoff', '0', + (string) read_config_option('plugin_gpsmap_upgrade_retry_after', true)); + +/* A missing host column means the schema work failed, whatever the helpers say. */ +$GLOBALS['gpsmap_stub_settings']['plugin_gpsmap_version'] = ''; +$GLOBALS['gpsmap_stub_settings']['plugin_gpsmap_upgrade_retry_after'] = '0'; +$GLOBALS['gpsmap_stub_missing_column'] = true; +gpsmap_upgrade_database(''); +assert_equal('upgrade: a missing column blocks the version write', '', + (string) read_config_option('plugin_gpsmap_version', true)); +$GLOBALS['gpsmap_stub_missing_column'] = false; + +if (!defined('GPSMAP_TEST_SUITE')) { + exit(gpsmap_test_summary()); +} From a260e566552bcfb1fc4619fe087c53b0d65bf6e1 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 19:12:10 -0700 Subject: [PATCH 3/5] fix(icons): offer only names the map can draw getIcons() accepted my-icon.png while the JavaScript emitter rejected it, so the icon appeared in the Map Template dropdown, saved cleanly, and then never rendered. Both sides now use gpsmap_icon_identifier(), the path is built from base_path so poller and CLI callers resolve it, and a missing icon directory no longer prints a PHP warning into the Map Templates form. Signed-off-by: Thomas Vincent --- gpsmap_security.php | 38 ++++++++++++++++++++++++++++++++++++++ gpstemplates.php | 28 +--------------------------- tests/test_icons.php | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 27 deletions(-) diff --git a/gpsmap_security.php b/gpsmap_security.php index c740535..c8ae54a 100644 --- a/gpsmap_security.php +++ b/gpsmap_security.php @@ -38,3 +38,41 @@ function gpsmap_normalize_icon_name($value, $icon_array, $default = 'Green.png') return $value; } + +//--------------------------------------------------------------- +function getIcons() { + $iconArray = array(); + global $config; + + /* Built from base_path so poller and CLI callers resolve it too; only web + * entry points chdir() to the Cacti root. */ + $dir = $config['base_path'] . '/plugins/gpsmap/images/icons'; + + /* Suppressed rather than warned: this runs on the Map Templates page, and a + * missing icon folder should not print a PHP warning into the form. */ + $icons = @opendir($dir); + + if ($icons === false) { + return $iconArray; + } + + while (false !== ($icon = readdir($icons))) { + /* Offer only names the map can actually render. icons.php emits each + * base name as a JavaScript identifier, so a file this rule rejects + * would appear in the dropdown, save cleanly, and then silently fail + * to draw. */ + if (!in_array(strtolower(pathinfo($icon, PATHINFO_EXTENSION)), GPSMAP_ICON_EXTENSIONS, true)) { + continue; + } + + if (gpsmap_icon_identifier($icon) === null) { + continue; + } + + $iconArray[$icon] = $icon; + } + + closedir($icons); + + return $iconArray; +} diff --git a/gpstemplates.php b/gpstemplates.php index ebc57ff..b913ebd 100644 --- a/gpstemplates.php +++ b/gpstemplates.php @@ -88,7 +88,7 @@ function templates() { $url = $config['url_path'] . 'plugins/gpsmap/gpstemplates.php?action=edit&id=' . $template['templateID']; form_alternate_row('line' . $template['templateID'], true); - form_selectable_cell("" . html_escape($template['templateName']) . "", $template['templateID']); + form_selectable_cell("" . html_escape($template['templateName']) . "", $template['templateID']); form_selectable_cell("", $template['templateID']); form_selectable_cell("", $template['templateID']); form_selectable_cell("", $template['templateID']); @@ -252,29 +252,3 @@ function gpsmap_save_template() { } //------------------------------------------------------------------------------ -function getIcons() { - $iconArray = array(); - $icons = opendir('./plugins/gpsmap/images/icons'); - - while (false !== ($icon = readdir($icons))) { - if ($icon != '.' && $icon != '..') { - $iconExplode = explode('.', $icon); - $iconExplode[1] = $iconExplode[1]; - - switch ($iconExplode[1]) { - case 'png': - case 'jpg': - case 'jpeg': - case 'gif': - $iconArray[$icon] = $icon; - break; - default: - break; - } - } - } - - closedir($icons); - - return $iconArray; -} diff --git a/tests/test_icons.php b/tests/test_icons.php index 31d97ae..252b3a5 100644 --- a/tests/test_icons.php +++ b/tests/test_icons.php @@ -18,6 +18,7 @@ require_once __DIR__ . '/harness.php'; require_once __DIR__ . '/../setup.php'; +require_once __DIR__ . '/../gpsmap_security.php'; require_once __DIR__ . '/../includes/polling/iconskml.php'; gpsmap_test_use_tmp_root(); @@ -117,6 +118,47 @@ assert_equal('gpsmap_safe_icon_base: good name', 'Green', gpsmap_safe_icon_base('Green.png')); assert_equal('gpsmap_safe_icon_base: bad name', 'undefined', gpsmap_safe_icon_base('ap.v2.png')); +/* ------------------------------------------------------------------ */ +/* getIcons() must offer exactly what the renderers can draw */ +/* ------------------------------------------------------------------ */ + +/* getIcons() reads a path relative to the Cacti root. */ +gpsmap_test_icons(array('Green.png', 'Node2.gif', 'my-icon.png', 'ap.v2.png', 'notes.txt', 'noext')); + +$cwd = getcwd(); +chdir(gpsmap_test_tmpdir()); +$offered = getIcons(); +chdir($cwd); + +assert_true('getIcons: offers a renderable icon', isset($offered['Green.png'])); +assert_true('getIcons: offers a digit-bearing name', isset($offered['Node2.gif'])); +assert_false('getIcons: hides hyphenated name', isset($offered['my-icon.png'])); +assert_false('getIcons: hides dotted name', isset($offered['ap.v2.png'])); +assert_false('getIcons: hides non-images', isset($offered['notes.txt'])); +assert_false('getIcons: hides extensionless files', isset($offered['noext'])); + +/* The dropdown and the JavaScript emitter must never disagree: anything + * offered here has to survive gpsmap_icon_identifier(). */ +foreach (array_keys($offered) as $name) { + assert_true('getIcons: offered name is renderable - ' . $name, gpsmap_icon_identifier($name) !== null); +} + +/* The path comes from base_path, not the working directory, so poller and CLI + * callers see the same list as the web pages. */ +$savedRoot = $GLOBALS['config']['base_path']; +$cwd = getcwd(); +chdir(sys_get_temp_dir()); +assert_true('getIcons: resolves regardless of the working directory', isset(getIcons()['Green.png'])); +chdir($cwd); + +$GLOBALS['config']['base_path'] = sys_get_temp_dir() . '/gpsmap-no-such-root'; +assert_equal('getIcons: missing directory yields no icons', array(), @getIcons()); +$GLOBALS['config']['base_path'] = $savedRoot; + +/* Saving still falls back when a name is not on the list. */ +assert_equal('save: hyphenated name rejected on save', 'Green.png', gpsmap_normalize_icon_name('my-icon.png', $offered)); +assert_equal('save: offered name accepted on save', 'Green.png', gpsmap_normalize_icon_name('Green.png', $offered)); + if (!defined('GPSMAP_TEST_SUITE')) { exit(gpsmap_test_summary()); } From 3fdb4a8941242190fa38e0d1ac432ea58bb2d2fa Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 19:12:10 -0700 Subject: [PATCH 4/5] chore: remove the unreferenced towerSelect endpoint Nothing in the plugin calls it, so it was an authenticated route to host_template that no page uses. Signed-off-by: Thomas Vincent --- .gitignore | 1 + includes/towerSelect.php | 47 ---------------------------------------- print.php | 4 ++-- setup.php | 2 +- 4 files changed, 4 insertions(+), 50 deletions(-) delete mode 100644 includes/towerSelect.php diff --git a/.gitignore b/.gitignore index 9e104b5..d3d6561 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ # review pipeline scratch .diffcheck.php .rv.tar +tests/_dbg.php diff --git a/includes/towerSelect.php b/includes/towerSelect.php deleted file mode 100644 index e42082c..0000000 --- a/includes/towerSelect.php +++ /dev/null @@ -1,47 +0,0 @@ -'; - -if (cacti_sizeof($results)) { - foreach ($results as $row) { - $body .= '
  • ' . html_escape($row['name']) . ' (ID: ' . html_escape($row['id']) . ')
    • '; - } -} - -$body .= ''; - -print(''); -print($body); -print(''); diff --git a/print.php b/print.php index 7f4c628..79a53b8 100644 --- a/print.php +++ b/print.php @@ -20,9 +20,9 @@ */ chdir('../../'); +/* auth.php halts execution (exit/redirect) for unauthenticated users, so the + * markup below is only reached after successful authentication. */ require_once('./include/auth.php'); - - ?>