cleanup: refactor svccommio class #13843
Conversation
benhillis
requested review from
Brian-Perkins,
OneBlue,
craigloewen-msft and
justus-camp-microsoft
There was a problem hiding this comment.
Pull request overview
This PR refactors the SvcCommIo helper class to use RAII patterns, significantly simplifying the code and improving maintainability. The changes replace a monolithic implementation with well-structured RAII wrapper classes (ConsoleInput and ConsoleOutput) that automatically manage console state restoration.
Key changes:
- Introduced ConsoleInput and ConsoleOutput RAII wrapper classes with factory methods
- Eliminated manual cleanup code by leveraging RAII destructors
- Reduced code length and complexity through better separation of concerns
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/windows/common/svccommio.hpp | Adds ConsoleInput and ConsoleOutput RAII wrapper class declarations; updates SvcCommIo to use these wrappers instead of manual state tracking |
| src/windows/common/svccommio.cpp | Implements RAII wrappers with factory methods; refactors SvcCommIo constructor to use the new wrappers; removes manual cleanup code replaced by RAII destructors |
| { | ||
| _stdHandles.StdOut.Handle = LXSS_HANDLE_USE_CONSOLE; | ||
| _stdHandles.StdOut.HandleType = LxssHandleConsole; | ||
| return std::unique_ptr<ConsoleOutput>(new ConsoleOutput(std::move(ConsoleHandle), Mode)); |
There was a problem hiding this comment.
nit: I'd have a preference towards either having ConsoleOutput be default constructible / movable, or using an std::optional in SvcCommIo so we can avoid having to allocate a std::unique_ptr here
| auto RestoreOutput = wil::scope_exit([&] { | ||
| if (RestoreMode) | ||
| const auto Error = GetLastError(); | ||
| if (Error != ERROR_INVALID_PARAMETER && Error != ERROR_PIPE_NOT_CONNECTED) |
There was a problem hiding this comment.
Looks like currently throw WSL_E_CONSOLE if we hit this, what's the reason for the behavior change for that error code ?
This helper class has always been a bit of a mess, this change reimplements the class using RAII classes and reduces complexity and code length.