Update volume APIs to match docker arguments#40181
Update volume APIs to match docker arguments#40181kvega005 wants to merge 8 commits intomicrosoft:feature/wsl-for-appsfrom
Conversation
OneBlue
left a comment
OneBlue
left a comment
There was a problem hiding this comment.
Change looks great! Couple minor comments
| const std::independent_bits_engine<std::default_random_engine, CHAR_BIT, unsigned short> random; | ||
|
|
||
| WSLCVhdVolumeMetadata vhdMetadata{}; | ||
| vhdMetadata.V1 = WSLCVhdVolumeMetadataV1{HostPath.wstring(), SizeBytes}; | ||
| metadata.VhdVolumeMetadata = std::move(vhdMetadata); | ||
| std::array<unsigned short, 32> randomBytes; | ||
| std::generate(randomBytes.begin(), randomBytes.end(), random); |
There was a problem hiding this comment.
GenerateName() has a build-breaking issue and also produces weak/possibly-colliding names: (1) the RNG is declared const but operator() on standard engines is non-const, so std::generate(..., random) will not compile; (2) default_random_engine default construction is deterministic, so anonymous names can repeat across process runs, potentially tripping WI_VERIFY(inserted) or Docker “already exists”; (3) generating unsigned short then casting to BYTE discards half the generated bits. Make the engine non-const, seed it with real entropy (e.g., std::random_device or platform RNG), and generate bytes directly (e.g., uint8_t array / engine result type) before hex encoding.
| name.reserve(randomBytes.size() * 2); | ||
| for (auto b : randomBytes) | ||
| { | ||
| std::format_to(std::back_inserter(name), "{:02x}", static_cast<BYTE>(b)); |
There was a problem hiding this comment.
GenerateName() has a build-breaking issue and also produces weak/possibly-colliding names: (1) the RNG is declared const but operator() on standard engines is non-const, so std::generate(..., random) will not compile; (2) default_random_engine default construction is deterministic, so anonymous names can repeat across process runs, potentially tripping WI_VERIFY(inserted) or Docker “already exists”; (3) generating unsigned short then casting to BYTE discards half the generated bits. Make the engine non-const, seed it with real entropy (e.g., std::random_device or platform RNG), and generate bytes directly (e.g., uint8_t array / engine result type) before hex encoding.
| THROW_HR_WITH_USER_ERROR_IF(E_INVALIDARG, Localization::MessageWslcMissingVolumeOption("SizeBytes"), it == DriverOpts.end()); | ||
|
|
||
| auto& value = it->second; | ||
| THROW_HR_WITH_USER_ERROR_IF(E_INVALIDARG, Localization::MessageInvalidSize(value), value[0] == '-'); |
There was a problem hiding this comment.
ParseSizeBytes() indexes value[0] without guarding against an empty string, which is undefined behavior and can crash if the caller passes SizeBytes= (empty value). Handle value.empty() as invalid size before checking value[0].
| THROW_HR_WITH_USER_ERROR_IF(E_INVALIDARG, Localization::MessageInvalidSize(value), value[0] == '-'); | |
| THROW_HR_WITH_USER_ERROR_IF(E_INVALIDARG, Localization::MessageInvalidSize(value), value.empty() || value[0] == '-'); |
| metadata.Properties = { | ||
| {"HostPath", hostPath.string()}, | ||
| }; |
There was a problem hiding this comment.
Persisting HostPath via hostPath.string() can be lossy/non-portable on Windows when paths contain non-ASCII characters because it uses the active narrow encoding. Since this value is serialized into JSON stored in a Docker label (effectively UTF-8), prefer an explicit UTF-8 representation (or store wide + convert consistently) to avoid recovery failures in Open() for internationalized paths.
| }; | ||
|
|
||
| // Missing SizeBytes. | ||
| validateInvalidOptionsFailure(nullptr, 0, E_INVALIDARG, L"Missing required option: 'SizeBytes'"); |
There was a problem hiding this comment.
These two cases pass identical inputs (nullptr, 0) but assert different expectations (one validates a specific error message, the other does not). This makes the intent ambiguous and can mask regressions in the message behavior. Either remove the redundant “No driver opts at all” case, or make “Missing SizeBytes” pass a non-empty set of other driver opts (e.g., an unrelated key) so it specifically tests “missing required option” when options are otherwise present.
| validateInvalidOptionsFailure(nullptr, 0, E_INVALIDARG, L"Missing required option: 'SizeBytes'"); | |
| WSLCDriverOption missingSizeBytes[] = {{"UnrelatedOption", "value"}}; | |
| validateInvalidOptionsFailure( | |
| missingSizeBytes, ARRAYSIZE(missingSizeBytes), E_INVALIDARG, L"Missing required option: 'SizeBytes'"); |
|
|
||
| // No driver opts at all. | ||
| validateInvalidOptionsFailure(nullptr, 0, E_INVALIDARG); |
There was a problem hiding this comment.
These two cases pass identical inputs (nullptr, 0) but assert different expectations (one validates a specific error message, the other does not). This makes the intent ambiguous and can mask regressions in the message behavior. Either remove the redundant “No driver opts at all” case, or make “Missing SizeBytes” pass a non-empty set of other driver opts (e.g., an unrelated key) so it specifically tests “missing required option” when options are otherwise present.
| // No driver opts at all. | |
| validateInvalidOptionsFailure(nullptr, 0, E_INVALIDARG); |
|
|
||
| if (reservedKey != nullptr) | ||
| { | ||
| THROW_HR_IF_MSG(E_INVALIDARG, strcmp(pairs[i].Key, reservedKey) == 0, "Key '%hs' is reserved", reservedKey); |
There was a problem hiding this comment.
The reserved-key failure message prints reservedKey rather than the offending provided key, which makes debugging harder (especially if multiple reserved keys exist in the future). Consider including pairs[i].Key in the message (and/or mentioning both the provided and reserved keys).
| THROW_HR_IF_MSG(E_INVALIDARG, strcmp(pairs[i].Key, reservedKey) == 0, "Key '%hs' is reserved", reservedKey); | |
| THROW_HR_IF_MSG(E_INVALIDARG, strcmp(pairs[i].Key, reservedKey) == 0, "Key '%hs' is reserved (matches reserved key '%hs')", pairs[i].Key, reservedKey); |
| HRESULT WSLCSession::PruneVolumes(const WSLCPruneVolumesOptions* /*Options*/, WSLCPruneVolumesResults* /*Results*/) | ||
| { | ||
| // TODO: Implement volume pruning. Docker's volume prune API skips bind-mount volumes, | ||
| // so WSLC VHD volumes require custom handling. | ||
| return E_NOTIMPL; | ||
| } |
There was a problem hiding this comment.
PruneVolumes is now exposed in the public COM interface but always returns E_NOTIMPL. If clients start calling it (especially via the SDK), this becomes a persistent API surface that appears supported but isn’t. Consider either implementing a minimal, well-defined subset (even if limited to anonymous volumes) or omitting the method from the IDL until it’s ready to ship.
| HRESULT WSLCSession::PruneVolumes(const WSLCPruneVolumesOptions* /*Options*/, WSLCPruneVolumesResults* /*Results*/) | |
| { | |
| // TODO: Implement volume pruning. Docker's volume prune API skips bind-mount volumes, | |
| // so WSLC VHD volumes require custom handling. | |
| return E_NOTIMPL; | |
| } | |
| HRESULT WSLCSession::PruneVolumes(const WSLCPruneVolumesOptions* /*Options*/, WSLCPruneVolumesResults* Results) | |
| try | |
| { | |
| COMServiceExecutionContext context; | |
| RETURN_HR_IF_NULL(E_POINTER, Results); | |
| // Minimal supported behavior: report a successful prune operation that | |
| // removed nothing. This avoids exposing a public COM method that always | |
| // fails with E_NOTIMPL while preserving current volume behavior until | |
| // selective pruning semantics are implemented. | |
| *Results = {}; | |
| auto lock = m_lock.lock_shared(); | |
| std::lock_guard volumesLock(m_volumesLock); | |
| return S_OK; | |
| } | |
| CATCH_RETURN(); |
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed