-
Notifications
You must be signed in to change notification settings - Fork 1.5k
WSLA: Add initial wsladiag tool with basic --list command and placeholder ListSessions #13725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WSLA: Add initial wsladiag tool with basic --list command and placeholder ListSessions #13725
Conversation
…lder ListSessions implementation
src/windows/wsladiag/main.cpp
Outdated
| #include <iostream> | ||
| #include <windows.h> | ||
|
|
||
| int wmain(int argc, wchar_t** argv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a helper class to make command line parsing easier, I would recommend using that instead (here's an example usage: https://github.com/microsoft/WSL/blob/8eee7439b2f1a7bbb0a6d5da04c1bce7b7ec8a0c/src/windows/common/WslClient.cpp#L1550C5-L1558C20)
src/windows/wsladiag/CMakeLists.txt
Outdated
| @@ -0,0 +1,7 @@ | |||
| set(SOURCES main.cpp) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI We'll probably want to add this new executable to the MSI package.
To that, we'll need a add dependency in: https://github.com/microsoft/WSL/blob/feature/wsl-for-apps/msipackage/CMakeLists.txt (so that the MSI package depends on this new wsladiag target
And then add an entry in: package.wix.in (most likely something similar to this)
src/windows/wsladiag/main.cpp
Outdated
|
|
||
| int wmain(int argc, wchar_t** argv) | ||
| { | ||
| bool list = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a new main() function, we need to do all the initialization like we do for processes. For now we probably to add:
wsl::windows::common::wslutil::ConfigureCrt();
wsl::windows::common::wslutil::InitializeWil();
WslTraceLoggingInitialize(LxssTelemetryProvider, !wsl::shared::OfficialBuild);
auto cleanupTelemetry = wil::scope_exit_log(WI_DIAGNOSTICS_INFO, []() { WslTraceLoggingUninitialize(); });
wsl::windows::common::wslutil::SetCrtEncoding(_O_U8TEXT);
auto coInit = wil::CoInitializeEx(COINIT_MULTITHREADED);
wsl::windows::common::wslutil::CoInitializeSecurity();
WSADATA data;
THROW_IF_WIN32_ERROR(WSAStartup(MAKEWORD(2, 2), &data));
A few of those will be required before we can actually call into the COM API
|
@microsoft-github-policy-service agree company="Microsoft" |
|
Latest update: Note: msipackage does not build locally due to missing clang/MSBuild tooling, |
src/windows/wsladiag/main.cpp
Outdated
| L"Usage:\n" | ||
| L" wsladiag --list List WSLA sessions\n" | ||
| L" wsladiag --help Show this help", | ||
| stdout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Other WSL processes currently write help messages to stdout, but since this is a new executable I think we should do the right thing and write message like this on stderr instead
|
FYI, to format the code to our clang-format rules, you can run You can also enable clang-format in VS for that to be done automatically. |
The msipackage should build locally, so I'd be curious to see which error you're hitting, since there isn't a tooling requirement to build it (we use WIX, which is installed via nuget when you build) The wiring does look correct though |
|
Update Pushed the initial implementation of the --list command in wsladiag. This version: Creates and initializes IWSLAUserSession Calls ListSessions and prints session ID, creator PID, and display name Handles UTF-8 → UTF-16 conversion for DisplayName Cleans up all CoTaskMem-allocated fields and the session array Uses the existing WSLA COM impersonation helpers Adds basic usage/help output for the new flag I'm still fixing an unrelated local environment issue where CMake can’t find the compiler, but the feature code itself is complete and ready for review. |
src/windows/wsladiag/main.cpp
Outdated
|
|
||
| THROW_IF_FAILED(userSession->ListSessions(&sessions, &sessionCount)); | ||
|
|
||
| auto cleanupSessions = wil::scope_exit([&]() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing manually, I'd recommend using: wil::unique_cotaskmem_array_ptr
Here's an example usage: https://github.com/microsoft/WSL/blob/7226b05100b20955692ec83914644c07d035047f/src/windows/common/svccomm.cpp#L592C2-L593C133
src/windows/wsladiag/main.cpp
Outdated
| std::wstring displayName = L"<unnamed>"; | ||
| if (session.DisplayName != nullptr) | ||
| { | ||
| const int length = MultiByteToWideChar(CP_UTF8, 0, session.DisplayName, -1, nullptr, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have helpers to allow std::format to automatically do this between std::string & std::wstring so we don't need to call MultiByteToWideChar() manually here
src/windows/wsladiag/main.cpp
Outdated
| catch (...) | ||
| { | ||
| const auto hr = wil::ResultFromCaughtException(); | ||
| wslutil::PrintMessage(std::format(L"Error listing WSLA sessions: 0x{:08x}\n", static_cast<unsigned int>(hr)), stderr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make debugging a bit easier, I'd also recommend printing the error code as a string. This method can be used to do that: https://github.com/microsoft/WSL/blob/7226b05100b20955692ec83914644c07d035047f/src/windows/common/wslutil.cpp#L619C1-L620C1 (note that it might return empty if we don't have a matching string for that error code)
src/windows/wsladiag/main.cpp
Outdated
| wslutil::ConfigureCrt(); | ||
| wslutil::InitializeWil(); | ||
|
|
||
| WslTraceLoggingInitialize(LxssTelemetryProvider, !wsl::shared::OfficialBuild); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OneBlue this seems like a strange provider to use here. Is there a more appropriate one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call.
The closest match we have is WslaServiceTelemetryProvider, (although the same isn't a great match). We could either rename that provider to WslaTelemetryProvider and use it here as well as in the service, or create another one specifically for wsladiag.exe.
I'd be OK with both, but I have a preference for the first option, since one telemetry provider just for wsldiag feels overkill
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I think we can just use a single provider for all of WSLA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification! I agree using a single provider makes more sense. I went with the first option and renamed the existing WslaServiceTelemetryProvider to WslaTelemetryProvider, and updated both the service and wsladiag.exe to use it. Let me know if you'd like this split out differently.
|
Update: |
src/windows/wsladiag/main.cpp
Outdated
| { | ||
| const auto& session = sessions[i]; | ||
|
|
||
| const std::wstring displayName = Utf8ToDisplayName(session.DisplayName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to skip the explicit conversion here (std::format should handle that automatically for us since we have this helper defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I missed that we already have std::formatter<char*, wchar_t> wired up.
I’ll remove the Utf8ToDisplayName helper and let std::format handle the UTF-8 conversion directly, only keeping a small fallback for the case.
|
I removed the explicit UTF-8 wide string conversion and now rely on the repo’s std::formatter<char*, wchar_t> helper. DisplayName is passed directly to std::format, with a small fallback for the null case (""). This keeps the code cleaner and matches the formatting logic already defined in the shared formatter. |
|
Update Implemented WSLA session creation and enumeration (CreateSession and ListSessions) in the service. Fixed session tracking by storing ComPtr instead of raw pointers to ensure correct lifetime management. Added basic session information population (SessionId + DisplayName). Rebuilt the service, deployed the updated MSI, and verified the full flow: |
src/windows/wsladiag/CMakeLists.txt
Outdated
| @@ -1,3 +1,5 @@ | |||
| cmake_minimum_required(VERSION 3.20) # add this at the very top | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed (it's already in the root CMakeLists.txt)
src/windows/wsladiag/CMakeLists.txt
Outdated
|
|
||
| target_precompile_headers(wsladiag REUSE_FROM common) | ||
| # Only try to reuse precompiled headers if 'common' exists | ||
| if (TARGET common) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
common always exist, so we can remove the if
| private: | ||
| wil::unique_tokeninfo_ptr<TOKEN_USER> m_tokenInfo; | ||
|
|
||
| ULONG m_nextSessionId = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use an std::atomic for this so we don't need to acquire the session lock to increment it
| CATCH_RETURN(); | ||
|
|
||
| HRESULT wsl::windows::service::wsla::WSLAUserSession::ListSessions(WSLA_SESSION_INFORMATION** Sessions, ULONG* SessionsCount) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a try / CATCH_LOG() at the method level so an exception thrown here can't crash the service
| for (size_t i = 0; i < m_wslaSessions.size(); ++i) | ||
| { | ||
| output[i].SessionId = m_wslaSessions[i]->GetId(); | ||
| m_wslaSessions[i]->GetDisplayName(&output[i].DisplayName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unlikely, but if an exception was thrown here, we could leak the strings allocated by GetDisplayName.
One easy way to work around this would be to change WSLA_SESSION_INFORMATION::DisplayName to a fixed size string, something like:
wchar_t DisplayName[256];
That way we don't need to dynamically allocate a string to hold it, and we don't need to manually delete it either
| wslutil::PrintMessage(std::format(L"Found {} WSLA session{}:\n", sessions.size(), sessions.size() > 1 ? L"s" : L""), stdout); | ||
|
|
||
| wslutil::PrintMessage(L"ID\tCreator PID\tDisplay Name\n", stdout); | ||
| wslutil::PrintMessage(L"--\t-----------\t------------\n", stdout); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably OK as the first implementation, but long term (maybe in a followup PR), we'll want to compute what's the longest string that we'll display, so we can have the right number of columns displayed here so everything is alligned regardless of the DisplayName's sizes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's how do it for wsl --list for reference: https://github.com/microsoft/WSL/blob/ba90ee11fa83a1cb7d908d64cb50197afa6a8aeb/src/windows/common/WslClient.cpp#L730C8-L738C12
(We could probably do a more modern version of that)
src/windows/wsladiag/main.cpp
Outdated
| wslutil::PrintMessage(L"ID\tCreator PID\tDisplay Name\n", stdout); | ||
| wslutil::PrintMessage(L"--\t-----------\t------------\n", stdout); | ||
|
|
||
| for (ULONG i = 0; i < sessions.size(); ++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We can iterate directly over sessions like this:
for (const auto & session: sessions)
|
Update: |
| using wsl::windows::service::wsla::WSLASession; | ||
|
|
||
| WSLASession::WSLASession(const WSLA_SESSION_SETTINGS& Settings, WSLAUserSessionImpl& userSessionImpl, const VIRTUAL_MACHINE_SETTINGS& VmSettings) : | ||
| WSLASession::WSLASession(ULONG id, const WSLA_SESSION_SETTINGS& Settings, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: move Settings arg to new line for consistency
| { | ||
| std::lock_guard lock(m_wslaSessionsLock); | ||
| auto it = m_sessions.emplace(session.Get()); | ||
| m_wslaSessions.emplace_back(session); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't. m_wslaSessions will actually making the sessions impossible to "delete" since the service would hold a COM reference on those forever.
I'd recommend removing m_wslaSessions and using m_sessions here
| HRESULT CreateSession(const WSLA_SESSION_SETTINGS* Settings, const VIRTUAL_MACHINE_SETTINGS* VmSettings, IWSLASession** WslaSession); | ||
|
|
||
| HRESULT ListSessions(_Out_ WSLA_SESSION_INFORMATION** Sessions, _Out_ ULONG* SessionsCount); | ||
| void OnVmTerminated(WSLAVirtualMachine* machine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is OnVmTerminated added back here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's probably a merge issue. Let's remove this.
|
|
||
| HRESULT wsl::windows::service::wsla::WSLAUserSessionImpl::ListSessions(_Out_ WSLA_SESSION_INFORMATION** Sessions, _Out_ ULONG* SessionsCount) | ||
| { | ||
| const auto count = m_wslaSessions.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this variable isn't needed. We can directly use size() when we assign to SessionsCount
|
|
||
| RETURN_IF_FAILED(m_wslaSessions[i]->GetDisplayName(&tempName)); | ||
|
|
||
| if (tempName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tempName should never be null, so we can remove this if. We can add a WI_ASSERT() to be safe if we want to though
| output[i].CreatorPid = 0; // placeholder until we populate this later | ||
| PWSTR tempName = nullptr; | ||
|
|
||
| RETURN_IF_FAILED(m_wslaSessions[i]->GetDisplayName(&tempName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify things a bit, we could have GetDisplayName write to a pointer passed in. That would remove the extra memory allocation.
This change adds the first version of the
wsladiagdiagnostic tool and hooks it up to the WSLA host service.wsladiagexecutable undersrc/windows/wsladiagand integrates it into the CMake build.main.cppthat parses--listand--helparguments and prints a placeholder message for--list.WSLAUserSession::ListSessionsto return a safe empty result instead ofE_NOTIMPL, so it is ready to be called fromwsladiagin a follow-up change.wsladiag.exeruns and accepts--list/--help.Detailed Description of the Pull Request / Additional comments
This PR is the first step toward a WSLA-specific diagnostic tool (
wsladiag) that can be used by developers to inspect and debug WSLA sessions.Tool side (
wsladiag):wsladiagexecutable undersrc/windows/wsladiag.main.cppthat:--listand--helpcommand-line options.--list, currently prints a placeholder message. A follow-up change will connect this to the WSLA host serviceListSessionsCOM API.Host service side (
WSLAUserSession::ListSessions):E_NOTIMPLstub implementation with a safe placeholder that:WSLA_SESSION_INFORMATIONlist and aSessionsCountof0.wsladiag) to start wiring up the COM call without hittingE_NOTIMPL. A later change will populate the result fromWSLAUserSessionImpl::m_sessionsand return real session information.This PR is intended as a starting point / draft to confirm the overall direction (shape of the tool, build integration, and host API surface) before adding the actual session enumeration and debug-shell functionality.
Validation Steps Performed
cmake --build . --config Debugwsladiag.exeis produced underbin\x64\Debug.wsladiag.exe --help→ shows usage text.wsladiag.exe --list→ prints the placeholder message without errors.WSLAUserSession::ListSessionsfromE_NOTIMPLto an empty result does not break the service build.