Skip to content

MDEV-41080 startup code on Windows, remove checks for existing service - #5729

Open
vaintroub wants to merge 1 commit into
10.11from
10.11-MDEV-41080
Open

vaintroub wants to merge 1 commit into
10.11from
10.11-MDEV-41080

Conversation

@vaintroub

Copy link
Copy Markdown
Member

Summary

  • Remove the is_existing_service heuristic; always attempt StartServiceCtrlDispatcher and fall back to running as a normal process if that fails
  • Report unexpected errors from StartServiceCtrlDispatcher/RegisterServiceCtrlHandler to the Windows Event Log
  • Use the authoritative service name returned as the first argument to svc_main by the Service Control Manager, instead of guessing it from the command line

🤖 Generated with Claude Code

They were not necessary, just try to run as service, and fallback to
command line.

Add some diagnostics - unexpected errors from StartServiceCtrlDispatcher
and RegisterServiceCtrlHandler are now reported to Windows event log.

Also use authoritative service name, returned as first argument
in svc_main by service control manager.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The service dispatch table can prevent installed services from starting correctly.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
What changed in this PR

Updates Windows startup handling to use SCM dispatching, report errors, and rely on SCM-provided service names.

Changes:

  • Removes service-existence heuristics.
  • Adds Event Log reporting for startup errors.
  • Uses the service name supplied by SCM.
File Summary
sql/​winmain.cc Critical issue: use NULL rather than an empty service name in the dispatch table. The file header also needs updating to reflect the new startup behavior.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread sql/winmain.cc

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

No-argument service invocations still bypass the service dispatcher and fail to connect to the SCM.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

Comment thread sql/winmain.cc
/* Try to run as service, and fallback to mysqld_main(), if this fails */
svc_name= argv[argc - 1];
if (is_existing_service(svc_name) && !run_as_service(svc_name))
if (!run_as_service())

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pre-existing behavior, not something this commit changes (the argc == 1 early-return existed before this diff). It's also not reachable via any real service installed by this code: install_service() always appends the service name as a trailing argument to the generated ImagePath, so any service actually registered via --install/--install-manual has argc >= 2 when SCM starts it. A bare-exe ImagePath with no arguments would have to be hand-crafted outside this tooling.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants