From 478b83e3dff7055d7a976dc5aaa8cc4eec0158d9 Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Thu, 7 May 2026 19:22:30 -0700 Subject: [PATCH] CLI: Initialize cidfile option (#40455) * Initialize cidfile option Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- localization/strings/en-US/Resources.resw | 6 ++- .../wslc/arguments/ArgumentDefinitions.h | 2 +- .../wslc/commands/ContainerCreateCommand.cpp | 2 +- .../wslc/commands/ContainerRunCommand.cpp | 2 +- src/windows/wslc/services/ContainerModel.cpp | 46 +++++++++++++++++++ src/windows/wslc/services/ContainerModel.h | 18 ++++++++ .../wslc/services/ContainerService.cpp | 14 +++++- src/windows/wslc/tasks/ContainerTasks.cpp | 5 ++ test/windows/wslc/CommandLineTestCases.h | 2 + .../wslc/e2e/WSLCE2EContainerCreateTests.cpp | 31 +++++++++++++ .../wslc/e2e/WSLCE2EContainerRunTests.cpp | 34 ++++++++++++++ 11 files changed, 156 insertions(+), 6 deletions(-) diff --git a/localization/strings/en-US/Resources.resw b/localization/strings/en-US/Resources.resw index e6d9c115e..275b9fea5 100644 --- a/localization/strings/en-US/Resources.resw +++ b/localization/strings/en-US/Resources.resw @@ -2678,7 +2678,7 @@ On first run, creates the file with all settings commented out at their defaults Working directory inside the container - Write the container ID to the provided path. + Write the container ID to the provided path IP address of the DNS nameserver in resolv.conf @@ -2758,6 +2758,10 @@ On first run, creates the file with all settings commented out at their defaults Invalid {} argument value: '{}'. Expected a memory size (e.g. 256M, 1G) {FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated{Locked="256M"}{Locked="1G"} + + CID file '{}' already exists + {FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated + Image '{}' not found, pulling {FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated diff --git a/src/windows/wslc/arguments/ArgumentDefinitions.h b/src/windows/wslc/arguments/ArgumentDefinitions.h index dd18acd1f..ec3e3ad10 100644 --- a/src/windows/wslc/arguments/ArgumentDefinitions.h +++ b/src/windows/wslc/arguments/ArgumentDefinitions.h @@ -38,7 +38,7 @@ _(Attach, "attach", L"a", Kind::Flag, L _(BuildArg, "build-arg", NO_ALIAS, Kind::Value, Localization::WSLCCLI_BuildArgDescription()) \ _(BuildPull, "pull", NO_ALIAS, Kind::Flag, Localization::WSLCCLI_BuildPullArgDescription()) \ _(BuildTarget, "target", NO_ALIAS, Kind::Value, Localization::WSLCCLI_BuildTargetArgDescription()) \ -/*_(CIDFile, "cidfile", NO_ALIAS, Kind::Value, Localization::WSLCCLI_CIDFileArgDescription())*/ \ +_(CIDFile, "cidfile", NO_ALIAS, Kind::Value, Localization::WSLCCLI_CIDFileArgDescription()) \ _(Command, "command", NO_ALIAS, Kind::Positional, Localization::WSLCCLI_CommandArgDescription()) \ _(ContainerId, "container-id", NO_ALIAS, Kind::Positional, Localization::WSLCCLI_ContainerIdArgDescription()) \ _(Force, "force", L"f", Kind::Flag, Localization::WSLCCLI_ForceArgDescription()) \ diff --git a/src/windows/wslc/commands/ContainerCreateCommand.cpp b/src/windows/wslc/commands/ContainerCreateCommand.cpp index 8746bf7c3..676f8ac69 100644 --- a/src/windows/wslc/commands/ContainerCreateCommand.cpp +++ b/src/windows/wslc/commands/ContainerCreateCommand.cpp @@ -31,7 +31,7 @@ std::vector ContainerCreateCommand::GetArguments() const Argument::Create(ArgType::ImageId, true), Argument::Create(ArgType::Command), Argument::Create(ArgType::ForwardArgs), - // Argument::Create(ArgType::CIDFile), + Argument::Create(ArgType::CIDFile), Argument::Create(ArgType::DNS, false, NO_LIMIT), // Argument::Create(ArgType::DNSDomain), Argument::Create(ArgType::DNSOption, false, NO_LIMIT), diff --git a/src/windows/wslc/commands/ContainerRunCommand.cpp b/src/windows/wslc/commands/ContainerRunCommand.cpp index f8c2c7d8e..49a54d249 100644 --- a/src/windows/wslc/commands/ContainerRunCommand.cpp +++ b/src/windows/wslc/commands/ContainerRunCommand.cpp @@ -31,7 +31,7 @@ std::vector ContainerRunCommand::GetArguments() const Argument::Create(ArgType::ImageId, true), Argument::Create(ArgType::Command), Argument::Create(ArgType::ForwardArgs), - // Argument::Create(ArgType::CIDFile), + Argument::Create(ArgType::CIDFile), Argument::Create(ArgType::Detach), Argument::Create(ArgType::DNS, false, NO_LIMIT), // Argument::Create(ArgType::DNSDomain), diff --git a/src/windows/wslc/services/ContainerModel.cpp b/src/windows/wslc/services/ContainerModel.cpp index 3cd2f67de..baaaa562f 100644 --- a/src/windows/wslc/services/ContainerModel.cpp +++ b/src/windows/wslc/services/ContainerModel.cpp @@ -332,4 +332,50 @@ TmpfsMount TmpfsMount::Parse(const std::string& value) result.m_options = value.substr(colonPos + 1); return result; } + +CidFile::CidFile(const std::optional& path) +{ + if (!path.has_value()) + { + return; + } + + m_path = *path; + auto [file, openError] = wil::try_create_new_file(std::filesystem::path(*m_path).c_str(), GENERIC_WRITE); + if (!file.is_valid()) + { + if (openError == ERROR_FILE_EXISTS || openError == ERROR_ALREADY_EXISTS) + { + THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(openError), Localization::WSLCCLI_CIDFileAlreadyExistsError(*m_path)); + } + + const auto errorMessage = wsl::windows::common::wslutil::GetSystemErrorString(HRESULT_FROM_WIN32(openError)); + THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(openError), Localization::MessageWslcFailedToOpenFile(*m_path, errorMessage)); + } + + m_file = std::move(file); +} + +CidFile::~CidFile() +{ + if (m_committed || !m_path.has_value()) + { + return; + } + + m_file.reset(); + std::error_code ec; + std::filesystem::remove(std::filesystem::path(*m_path), ec); +} + +void CidFile::Commit(const std::string& containerId) +{ + if (m_file) + { + DWORD bytesWritten{}; + THROW_IF_WIN32_BOOL_FALSE(::WriteFile(m_file.get(), containerId.data(), static_cast(containerId.size()), &bytesWritten, nullptr)); + } + + m_committed = true; +} } // namespace wsl::windows::wslc::models diff --git a/src/windows/wslc/services/ContainerModel.h b/src/windows/wslc/services/ContainerModel.h index 039ffd2ce..88dc88a43 100644 --- a/src/windows/wslc/services/ContainerModel.h +++ b/src/windows/wslc/services/ContainerModel.h @@ -52,6 +52,7 @@ struct ContainerOptions std::vector DnsOptions; std::vector Tmpfs; std::vector> Labels; + std::optional CidFile{}; }; struct CreateContainerResult @@ -290,4 +291,21 @@ private: std::string m_containerPath; std::string m_options; }; + +class CidFile +{ +public: + explicit CidFile(const std::optional& path); + ~CidFile(); + + NON_COPYABLE(CidFile); + NON_MOVABLE(CidFile); + + void Commit(const std::string& containerId); + +private: + std::optional m_path{}; + wil::unique_hfile m_file; + bool m_committed = false; +}; } // namespace wsl::windows::wslc::models diff --git a/src/windows/wslc/services/ContainerService.cpp b/src/windows/wslc/services/ContainerService.cpp index 58d73304d..221074c5f 100644 --- a/src/windows/wslc/services/ContainerService.cpp +++ b/src/windows/wslc/services/ContainerService.cpp @@ -20,6 +20,7 @@ Abstract: #include #include #include +#include #include #include @@ -341,10 +342,18 @@ std::wstring ContainerService::FormatPorts(WSLCContainerState state, const std:: int ContainerService::Run(Session& session, const std::string& image, ContainerOptions runOptions) { + // Reserve the CID file (fails if it already exists) before creating the container so a + // container isn't created when the caller-requested path can't be written. The file is + // removed automatically if we don't reach Commit() below. + CidFile cidFile(runOptions.CidFile); + // Create the container auto runningContainer = CreateInternal(session, image, runOptions); auto& container = runningContainer.Get(); + WSLCContainerId containerId{}; + THROW_IF_FAILED(container.GetId(containerId)); + // Start the created container WSLCContainerStartFlags startFlags{}; WI_SetFlagIf(startFlags, WSLCContainerStartFlagsAttach, !runOptions.Detach); @@ -352,6 +361,7 @@ int ContainerService::Run(Session& session, const std::string& image, ContainerO // Disable auto-delete only after successful start runningContainer.SetDeleteOnClose(false); + cidFile.Commit(containerId); // Handle attach if requested if (WI_IsFlagSet(startFlags, WSLCContainerStartFlagsAttach)) @@ -360,19 +370,19 @@ int ContainerService::Run(Session& session, const std::string& image, ContainerO return consoleService.AttachToCurrentConsole(runningContainer.GetInitProcess()); } - WSLCContainerId containerId{}; - THROW_IF_FAILED(container.GetId(containerId)); PrintMessage(L"%hs", stdout, containerId); return 0; } CreateContainerResult ContainerService::Create(Session& session, const std::string& image, ContainerOptions runOptions) { + CidFile cidFile(runOptions.CidFile); auto runningContainer = CreateInternal(session, image, runOptions); runningContainer.SetDeleteOnClose(false); auto& container = runningContainer.Get(); WSLCContainerId id{}; THROW_IF_FAILED(container.GetId(id)); + cidFile.Commit(id); return {.Id = id}; } diff --git a/src/windows/wslc/tasks/ContainerTasks.cpp b/src/windows/wslc/tasks/ContainerTasks.cpp index b3917016e..6ba5069e2 100644 --- a/src/windows/wslc/tasks/ContainerTasks.cpp +++ b/src/windows/wslc/tasks/ContainerTasks.cpp @@ -226,6 +226,11 @@ void SetContainerOptionsFromArgs(CLIExecutionContext& context) { ContainerOptions options; + if (context.Args.Contains(ArgType::CIDFile)) + { + options.CidFile = context.Args.Get(); + } + if (context.Args.Contains(ArgType::Name)) { options.Name = WideToMultiByte(context.Args.Get()); diff --git a/test/windows/wslc/CommandLineTestCases.h b/test/windows/wslc/CommandLineTestCases.h index e7bfa4e4a..cb0c40851 100644 --- a/test/windows/wslc/CommandLineTestCases.h +++ b/test/windows/wslc/CommandLineTestCases.h @@ -71,6 +71,7 @@ COMMAND_LINE_TEST_CASE( true) COMMAND_LINE_TEST_CASE(L"container run ubuntu bash -c 'echo Hello World'", L"run", true) COMMAND_LINE_TEST_CASE(L"container run ubuntu", L"run", true) +COMMAND_LINE_TEST_CASE(L"container run --cidfile C:\\temp\\cidfile ubuntu", L"run", true) COMMAND_LINE_TEST_CASE(L"container run -it --name foo ubuntu", L"run", true) COMMAND_LINE_TEST_CASE(L"container run --rm -it --name foo ubuntu", L"run", true) COMMAND_LINE_TEST_CASE(L"stop", L"stop", true) @@ -84,6 +85,7 @@ COMMAND_LINE_TEST_CASE(L"container start --attach cont", L"start", true) COMMAND_LINE_TEST_CASE(L"container start -a cont", L"start", true) COMMAND_LINE_TEST_CASE(L"create ubuntu:latest", L"create", true) COMMAND_LINE_TEST_CASE(L"container create --name foo ubuntu", L"create", true) +COMMAND_LINE_TEST_CASE(L"container create --cidfile C:\\temp\\cidfile --name foo ubuntu", L"create", true) COMMAND_LINE_TEST_CASE(L"create --workdir /app ubuntu", L"create", true) COMMAND_LINE_TEST_CASE(L"create -w /app ubuntu", L"create", true) COMMAND_LINE_TEST_CASE(L"container create --workdir /app ubuntu sh", L"create", true) diff --git a/test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp b/test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp index f7d9f5ce9..90f65edcd 100644 --- a/test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp +++ b/test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp @@ -106,6 +106,36 @@ class WSLCE2EContainerCreateTests VerifyContainerIsListed(containerId, L"created"); } + WSLC_TEST_METHOD(WSLCE2E_Container_Create_CIDFile_Valid) + { + // Prepare a CID file path that does not exist + const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename(); + VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); + auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); }); + + auto result = RunWslc(std::format( + L"container create --cidfile \"{}\" --name {} {}", EscapePath(cidFilePath.wstring()), WslcContainerName, DebianImage.NameAndTag())); + result.Verify({.Stderr = L"", .ExitCode = 0}); + + const auto containerId = result.GetStdoutOneLine(); + VERIFY_IS_TRUE(std::filesystem::exists(cidFilePath)); + VERIFY_ARE_EQUAL(containerId, ReadFileContent(cidFilePath.wstring())); + } + + WSLC_TEST_METHOD(WSLCE2E_Container_Create_CIDFile_AlreadyExists) + { + const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename(); + auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); }); + + auto result = RunWslc(std::format( + L"container create --cidfile \"{}\" --name {} {}", EscapePath(cidFilePath.wstring()), WslcContainerName, DebianImage.NameAndTag())); + result.Verify( + {.Stderr = std::format(L"CID file '{}' already exists\r\nError code: ERROR_FILE_EXISTS\r\n", EscapePath(cidFilePath.wstring())), + .ExitCode = 1}); + + VerifyContainerIsNotListed(WslcContainerName); + } + WSLC_TEST_METHOD(WSLCE2E_Container_Create_DuplicateContainerName) { VerifyContainerIsNotListed(WslcContainerName); @@ -797,6 +827,7 @@ private: { std::wstringstream options; options << L"The following options are available:\r\n" // + << L" --cidfile Write the container ID to the provided path\r\n" << L" --dns IP address of the DNS nameserver in resolv.conf\r\n" << L" --dns-option Set DNS options\r\n" << L" --dns-search Set DNS search domains\r\n" diff --git a/test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp b/test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp index d7a1155ad..83dd35dfd 100644 --- a/test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp +++ b/test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp @@ -88,6 +88,39 @@ class WSLCE2EContainerRunTests VerifyContainerIsListed(WslcContainerName, L"exited"); } + WSLC_TEST_METHOD(WSLCE2E_Container_Run_CIDFile_Valid) + { + // Prepare a CID file path that does not exist + const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename(); + VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); + auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); }); + + auto result = RunWslc(std::format( + L"container run -d --cidfile \"{}\" --name {} {} sleep infinity", + EscapePath(cidFilePath.wstring()), + WslcContainerName, + DebianImage.NameAndTag())); + result.Verify({.Stderr = L"", .ExitCode = 0}); + + const auto containerId = result.GetStdoutOneLine(); + VERIFY_IS_TRUE(std::filesystem::exists(cidFilePath)); + VERIFY_ARE_EQUAL(containerId, ReadFileContent(cidFilePath.wstring())); + } + + WSLC_TEST_METHOD(WSLCE2E_Container_Run_CIDFile_AlreadyExists) + { + const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename(); + auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); }); + + auto result = RunWslc(std::format( + L"container run --cidfile \"{}\" --name {} {}", EscapePath(cidFilePath.wstring()), WslcContainerName, DebianImage.NameAndTag())); + result.Verify( + {.Stderr = std::format(L"CID file '{}' already exists\r\nError code: ERROR_FILE_EXISTS\r\n", EscapePath(cidFilePath.wstring())), + .ExitCode = 1}); + + VerifyContainerIsNotListed(WslcContainerName); + } + WSLC_TEST_METHOD(WSLCE2E_Container_Run_Entrypoint) { auto result = RunWslc(std::format(L"container run --rm --entrypoint /bin/whoami {}", DebianImage.NameAndTag())); @@ -786,6 +819,7 @@ private: { std::wstringstream options; options << L"The following options are available:\r\n" + << L" --cidfile Write the container ID to the provided path\r\n" << L" -d,--detach Run container in detached mode\r\n" << L" --dns IP address of the DNS nameserver in resolv.conf\r\n" << L" --dns-option Set DNS options\r\n"