Implement image save and export container #14121

ptrivedi · 2026-01-28T05:04:56Z

No description provided.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

benhillis · 2026-01-28T17:03:10Z

src/windows/wslaservice/exe/WSLASession.cpp

+    UNREFERENCED_PARAMETER(ProgressCallback);
+    RETURN_HR_IF_NULL(E_POINTER, ContainerID);
+    std::lock_guard lock{m_lock};
+    THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !m_dockerClient.has_value());


nit: we tend to like to have blank lines after throw or return statements to make things more readable.

Looks like before and after

benhillis · 2026-01-28T17:03:41Z

src/windows/wslaservice/exe/WSLASession.cpp

+    auto onCompleted = [&]() {
+        io.Cancel();
+        WSL_LOG("OnCompletedCalledForExport", TraceLoggingValue("OnCompletedCalledForExport", "Content"));
+    };


newline after closing braces.

should add this to the clang formatter...

too many newlines! but done

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 15 comments.

Copilot · 2026-01-28T17:10:57Z

src/windows/wslaservice/exe/WSLASession.cpp

+    if (RequestCodePair.first != 200)
+    {
+        io.AddHandle(std::make_unique<relay::ReadHandle>(
+            common::relay::HandleWrapper{RequestCodePair.second->stream.native_handle()}, accumulateError));


In ExportContainerImpl, accumulateError is passed by reference (line 550), while in SaveImageImpl it's moved with std::move (line 600). This inconsistency is unnecessary - both should use the same approach. Since the lambda is used only once, using std::move is more efficient, so ExportContainerImpl should also use std::move for consistency.

Suggested change

common::relay::HandleWrapper{RequestCodePair.second->stream.native_handle()}, accumulateError));

common::relay::HandleWrapper{RequestCodePair.second->stream.native_handle()}, std::move(accumulateError)));

Copilot · 2026-01-28T17:10:58Z

test/windows/WSLATests.cpp

+            LPSTR containerId = nullptr;
+            VERIFY_SUCCEEDED(container.Get().GetID(&containerId));
+            WSLA_ERROR_INFO errorInfo{};
+            VERIFY_SUCCEEDED(m_defaultSession->ExportContainer(HandleToULong(containerTarFileHandle.get()), containerId, nullptr, &errorInfo));


Memory leak: The containerId string allocated by GetID using CoTaskMemAlloc (via wil::unique_cotaskmem_ansistring) is never freed. After line 554, add CoTaskMemFree(containerId); to release the memory, or use a smart pointer like wil::unique_cotaskmem_string to manage the lifetime automatically.

Suggested change

VERIFY_SUCCEEDED(m_defaultSession->ExportContainer(HandleToULong(containerTarFileHandle.get()), containerId, nullptr, &errorInfo));

VERIFY_SUCCEEDED(m_defaultSession->ExportContainer(HandleToULong(containerTarFileHandle.get()), containerId, nullptr, &errorInfo));

CoTaskMemFree(containerId);

Copilot · 2026-01-28T17:10:58Z

test/windows/WSLATests.cpp

+            VERIFY_FAILED(m_defaultSession->ExportContainer(HandleToULong(contTarFileHandle.get()), "dummy", nullptr, &errorInfo));
+            std::string errMsg = errorInfo.UserErrorMessage;
+            LogInfo("Error message: %hs", errMsg.c_str());
+            VERIFY_IS_TRUE(errMsg.find("No such container") != std::string::npos);


Memory leak: The errorInfo.UserErrorMessage string allocated using CoTaskMemAlloc (via wil::unique_cotaskmem_ansistring) is never freed. After line 571, add CoTaskMemFree(errorInfo.UserErrorMessage); to release the memory.

Suggested change

VERIFY_IS_TRUE(errMsg.find("No such container") != std::string::npos);

VERIFY_IS_TRUE(errMsg.find("No such container") != std::string::npos);

CoTaskMemFree(errorInfo.UserErrorMessage);

Copilot · 2026-01-28T17:10:59Z

src/windows/wslaservice/exe/WSLASession.cpp

+    wil::unique_handle imageFileHandle{wsl::windows::common::wslutil::DuplicateHandleFromCallingProcess(ULongToHandle(OutputHandle))};
+    THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !m_dockerClient.has_value());
+    relay::MultiHandleWait io;
+    std::optional<boost::beast::http::status> importResult;


The variable importResult is declared but never used. It should be removed as it appears to be copy-pasted from another function (possibly ImportImageImpl).

Suggested change

std::optional<boost::beast::http::status> importResult;

Copilot · 2026-01-28T17:10:59Z

src/windows/wslaservice/exe/WSLASession.cpp

+    wil::unique_handle imageFileHandle{wsl::windows::common::wslutil::DuplicateHandleFromCallingProcess(ULongToHandle(OutputHandle))};
+    THROW_HR_IF(HRESULT_FROM_WIN32(ERROR_INVALID_STATE), !m_dockerClient.has_value());
+    relay::MultiHandleWait io;
+    std::optional<boost::beast::http::status> importResult;


The variable importResult is declared but never used. It should be removed as it appears to be copy-pasted from another function (possibly ImportImageImpl).

Suggested change

std::optional<boost::beast::http::status> importResult;

Copilot · 2026-01-28T17:11:01Z

test/windows/WSLATests.cpp

+            VERIFY_ARE_EQUAL(fileSize.QuadPart > 0, false);
+            WSLA_ERROR_INFO errorInfo{};
+            VERIFY_SUCCEEDED(m_defaultSession->SaveImage(HandleToULong(imageTarFileHandle.get()), "hello-world:latest", nullptr, &errorInfo));
+            VERIFY_ARE_EQUAL(0 == errorInfo.UserErrorMessage, true);


For consistency with the rest of the codebase, use nullptr for pointer comparisons instead of 0. This should be VERIFY_ARE_EQUAL(errorInfo.UserErrorMessage, nullptr); or VERIFY_IS_NULL(errorInfo.UserErrorMessage); if such a macro exists.

Suggested change

VERIFY_ARE_EQUAL(0 == errorInfo.UserErrorMessage, true);

VERIFY_ARE_EQUAL(errorInfo.UserErrorMessage, nullptr);

Copilot · 2026-01-28T17:11:02Z

src/windows/wslaservice/inc/wslaservice.idl

+    HRESULT SaveImage([in] ULONG OutputHandle, [in] LPCSTR ImageNameOrID, [ in, unique ] IProgressCallback * ProgressCallback, [in, out, unique] WSLA_ERROR_INFO * ErrorInfo);
    HRESULT ListImages([out, size_is(, *Count)] struct WSLA_IMAGE_INFORMATION** Images, [out] ULONG* Count);
    HRESULT DeleteImage([in] const struct WSLA_DELETE_IMAGE_OPTIONS* Options, [out, size_is(, *Count)] struct WSLA_DELETED_IMAGE_INFORMATION** DeletedImages, [out] ULONG* Count, [in, out, unique] WSLA_ERROR_INFO* ErrorInfo);

    // Container management.
    HRESULT CreateContainer([in] const struct WSLA_CONTAINER_OPTIONS* Options, [out] IWSLAContainer** Container, [in, out, unique] WSLA_ERROR_INFO* ErrorInfo);
    HRESULT OpenContainer([in] LPCSTR Id, [out] IWSLAContainer** Container);
    HRESULT ListContainers([out, size_is(, *Count)] struct WSLA_CONTAINER** Images, [out] ULONG* Count);
+    HRESULT ExportContainer([in] ULONG OutputHandle, [in] LPCSTR ContainerID, [ in, unique ] IProgressCallback * ProgressCallback, [in,out,unique] WSLA_ERROR_INFO * ErrorInfo);


The parameter annotations have inconsistent spacing. Line 348 uses [ in, unique ] with spaces inside brackets, while line 356 uses [in,out,unique] without spaces. This should be standardized to match the convention used elsewhere in the file (line 345 uses [in, unique] and [in, out, unique]).

Copilot · 2026-01-28T17:11:02Z

test/windows/WSLATests.cpp

+        {
+            std::filesystem::path imageTar = std::filesystem::path{g_testDataPath} / L"HelloWorldExported.tar";
+            wil::unique_handle imageTarFileHandle{CreateFileW(
+                imageTar.c_str(), GENERIC_WRITE | GENERIC_READ, FILE_SHARE_READ, nullptr, TRUNCATE_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr)};


TRUNCATE_EXISTING requires the file to already exist, otherwise CreateFileW will fail with ERROR_FILE_NOT_FOUND. Since this is a new export operation, the file likely doesn't exist yet. Use CREATE_ALWAYS instead to create the file if it doesn't exist or truncate it if it does. This same issue exists in both SaveImage test (line 495) and ExportContainer test (line 547).

Suggested change

imageTar.c_str(), GENERIC_WRITE | GENERIC_READ, FILE_SHARE_READ, nullptr, TRUNCATE_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr)};

imageTar.c_str(), GENERIC_WRITE | GENERIC_READ, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)};

Copilot · 2026-01-28T17:11:03Z

src/windows/wslaservice/exe/DockerHTTPClient.cpp

+std::pair<uint32_t, std::unique_ptr<DockerHTTPClient::HTTPRequestContext>> DockerHTTPClient::ExportContainer(const std::string& ContainerNameOrID)
+{
+    return SendRequestWithContext(verb::get, std::format("http://localhost/containers/{}/export", ContainerNameOrID), {}, nullptr, {});


Inconsistent parameter naming: This method uses ContainerNameOrID (with "ID" in uppercase), while the similar SaveImage method uses NameOrId (with "Id" in mixed case). For consistency across the codebase, consider using the same casing convention. Based on other methods in this class (like AttachContainer, InspectContainer) which use Id, the parameter should be named ContainerNameOrId or simply Id to match the existing pattern.

Suggested change

std::pair<uint32_t, std::unique_ptr<DockerHTTPClient::HTTPRequestContext>> DockerHTTPClient::ExportContainer(const std::string& ContainerNameOrID)

{

return SendRequestWithContext(verb::get, std::format("http://localhost/containers/{}/export", ContainerNameOrID), {}, nullptr, {});

std::pair<uint32_t, std::unique_ptr<DockerHTTPClient::HTTPRequestContext>> DockerHTTPClient::ExportContainer(const std::string& ContainerNameOrId)

{

return SendRequestWithContext(verb::get, std::format("http://localhost/containers/{}/export", ContainerNameOrId), {}, nullptr, {});

Copilot · 2026-01-28T17:11:03Z

src/windows/wslaservice/exe/WSLASession.h

        _Inout_opt_ WSLA_ERROR_INFO* ErrorInfo) override;
    IFACEMETHOD(LoadImage)(_In_ ULONG ImageHandle, _In_ IProgressCallback* ProgressCallback, _In_ ULONGLONG ContentLength) override;
    IFACEMETHOD(ImportImage)(_In_ ULONG ImageHandle, _In_ LPCSTR ImageName, _In_ IProgressCallback* ProgressCallback, _In_ ULONGLONG ContentLength) override;
+    IFACEMETHOD(SaveImage)(_In_ ULONG OutputHandle, _In_ LPCSTR ImageNameOrID, _In_ IProgressCallback* ProgressCallback, WSLA_ERROR_INFO* Error) override;


Missing SAL annotation: The Error parameter should be annotated with _Inout_opt_ to be consistent with other similar methods in this class (PullImage line 64, DeleteImage line 73, CreateContainer line 76, ExportContainer line 79).

Suggested change

IFACEMETHOD(SaveImage)(_In_ ULONG OutputHandle, _In_ LPCSTR ImageNameOrID, _In_ IProgressCallback* ProgressCallback, WSLA_ERROR_INFO* Error) override;

IFACEMETHOD(SaveImage)(_In_ ULONG OutputHandle, _In_ LPCSTR ImageNameOrID, _In_ IProgressCallback* ProgressCallback, _Inout_opt_ WSLA_ERROR_INFO* Error) override;

src/windows/wslaservice/exe/DockerHTTPClient.cpp

OneBlue · 2026-01-28T20:25:27Z

src/windows/wslaservice/exe/WSLAContainer.cpp

    }
 }

+void WSLAContainerImpl::GetID(LPSTR* Output)


We can remove those since they're already implemented in https://github.com/microsoft/WSL/blob/535759e0d596c8a5f1ab8336c581da4f60cd0905/src/windows/wslaservice/exe/WSLAContainer.cpp#L873C1-L874C1

OneBlue · 2026-01-28T20:28:01Z

test/windows/WSLATests.cpp

+            VERIFY_SUCCEEDED(m_defaultSession->SaveImage(HandleToULong(imageTarFileHandle.get()), "hello-world:latest", nullptr, &errorInfo));
+            VERIFY_ARE_EQUAL(0 == errorInfo.UserErrorMessage, true);
+            VERIFY_IS_TRUE(GetFileSizeEx(imageTarFileHandle.get(), &fileSize));
+            VERIFY_ARE_EQUAL(fileSize.QuadPart > 0, true);


I would recommend something like calling file or tar tf to make sure that the tar is valid, or maybe try to import the image back

OneBlue · 2026-01-28T20:29:46Z

src/windows/wslaservice/inc/wslaservice.idl

    HRESULT CreateContainer([in] const struct WSLA_CONTAINER_OPTIONS* Options, [out] IWSLAContainer** Container, [in, out, unique] WSLA_ERROR_INFO* ErrorInfo);
    HRESULT OpenContainer([in] LPCSTR Id, [out] IWSLAContainer** Container);
    HRESULT ListContainers([out, size_is(, *Count)] struct WSLA_CONTAINER** Images, [out] ULONG* Count);
+    HRESULT ExportContainer([in] ULONG OutputHandle, [in] LPCSTR ContainerID, [ in, unique ] IProgressCallback * ProgressCallback, [in,out,unique] WSLA_ERROR_INFO * ErrorInfo);


I recommend implementing this method at the WSLAContainer level, that way we can lock it while the container export is in progress

OneBlue · 2026-01-28T20:30:30Z

test/windows/WSLATests.cpp

+            VERIFY_IS_TRUE(GetFileSizeEx(contTarFileHandle.get(), &fileSize));
+            VERIFY_ARE_EQUAL(fileSize.QuadPart > 0, false);
+            WSLA_ERROR_INFO errorInfo{};
+            VERIFY_FAILED(m_defaultSession->ExportContainer(HandleToULong(contTarFileHandle.get()), "dummy", nullptr, &errorInfo));


I recommend validating the error code here, so we know that we're correctly handling the error (although if we move this method to WSLAContainer that will solve this since Open() will fail on invalid name / id)

Implement image save and export container

c4a405c

ptrivedi requested a review from a team as a code owner January 28, 2026 05:04

ptrivedi requested review from OneBlue, benhillis, Copilot, craigloewen-msft and justus-camp-microsoft January 28, 2026 05:04

ptrivedi changed the base branch from master to feature/wsl-for-apps January 28, 2026 05:05

ptrivedi requested a review from a team as a code owner January 28, 2026 05:05

Copilot AI reviewed Jan 28, 2026

View reviewed changes

benhillis requested a review from Copilot January 28, 2026 17:01

Copilot started reviewing on behalf of benhillis January 28, 2026 17:02 View session

benhillis reviewed Jan 28, 2026

View reviewed changes

Copilot AI reviewed Jan 28, 2026

View reviewed changes

OneBlue requested changes Jan 28, 2026

View reviewed changes

	common::relay::HandleWrapper{RequestCodePair.second->stream.native_handle()}, accumulateError));
	common::relay::HandleWrapper{RequestCodePair.second->stream.native_handle()}, std::move(accumulateError)));

	VERIFY_SUCCEEDED(m_defaultSession->ExportContainer(HandleToULong(containerTarFileHandle.get()), containerId, nullptr, &errorInfo));
	VERIFY_SUCCEEDED(m_defaultSession->ExportContainer(HandleToULong(containerTarFileHandle.get()), containerId, nullptr, &errorInfo));
	CoTaskMemFree(containerId);

	VERIFY_IS_TRUE(errMsg.find("No such container") != std::string::npos);
	VERIFY_IS_TRUE(errMsg.find("No such container") != std::string::npos);
	CoTaskMemFree(errorInfo.UserErrorMessage);

	VERIFY_ARE_EQUAL(0 == errorInfo.UserErrorMessage, true);
	VERIFY_ARE_EQUAL(errorInfo.UserErrorMessage, nullptr);

	imageTar.c_str(), GENERIC_WRITE \| GENERIC_READ, FILE_SHARE_READ, nullptr, TRUNCATE_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr)};
	imageTar.c_str(), GENERIC_WRITE \| GENERIC_READ, FILE_SHARE_READ, nullptr, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, nullptr)};

	IFACEMETHOD(SaveImage)(_In_ ULONG OutputHandle, _In_ LPCSTR ImageNameOrID, _In_ IProgressCallback* ProgressCallback, WSLA_ERROR_INFO* Error) override;
	IFACEMETHOD(SaveImage)(_In_ ULONG OutputHandle, _In_ LPCSTR ImageNameOrID, _In_ IProgressCallback* ProgressCallback, _Inout_opt_ WSLA_ERROR_INFO* Error) override;

Implement image save and export container #14121

Are you sure you want to change the base?

Implement image save and export container #14121

Conversation

ptrivedi commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants