MDEV-40023 Clean up dead code in check_expected_crash_and_restart#5229
MDEV-40023 Clean up dead code in check_expected_crash_and_restart#5229FarihaIS wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the restart command handling in the MariaDB test suite by removing the unused restart_bindir command and its associated environment variable handling. It also introduces strict error checking for unknown commands in the expect file. The review feedback highlights that the regular expression /^restart/ is too loose and will match any command starting with "restart" (e.g., "restarting"), suggesting it be tightened to /^restart\s*$/ to prevent silently ignoring invalid commands.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| my @rest_opt= split(' ', $1); | ||
| $mysqld->{'restart_opts'}= \@rest_opt; | ||
| } else { | ||
| } elsif ($last_line =~ /^restart/) { |
There was a problem hiding this comment.
The regular expression /^restart/ is too loose and will match any command starting with restart (such as restart_foo or restarting). This means invalid commands starting with restart will be silently treated as a plain restart instead of triggering the mtr_error as intended. To ensure only exact restart commands (with optional trailing whitespace) are matched, use /^restart\s*$/ instead.
} elsif ($last_line =~ /^restart\s*$/) {
There was a problem hiding this comment.
This is intentional as existing tests (e.g. maria_empty_logs.inc) write tagged restart commands like restart-maria_empty_logs.inc to the expect file as a plain restart, so using /^restart\s*$/ would reject those.
|
|
Remove the unused restart_bindir handler from mariadb-test-run.pl and start_mysqld.inc (no test ever sets $restart_bindir). Error on unrecognized expect file commands instead of silently ignoring them. All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.
2 participants
Description
check_expected_crash_and_restart()inmariadb-test-run.plcontains dead code forrestart_bindir(no test ever sets$restart_bindir) and silently ignores unknown commands in the expect file. This made it easy for bugs like MDEV-39153 to go undetected.Fix:
restart_bindirhandler frommariadb-test-run.plandstart_mysqld.incmtr_error()when an unrecognized command is found in the expect fileRelease Notes
N/A
How can this PR be tested?
Execute the full MTR test suite — no test uses
restart_bindir, so removing it should have no effect. Themtr_error()change will prevent any future invalid expect file commands from failing silently.Basing the PR against the correct MariaDB version
Copyright
All new code of the whole pull request, including one or several files that are either new files or modified ones, are contributed under the BSD-new license. I am contributing on behalf of my employer Amazon Web Services, Inc.