libvirt: Only inject STORAGE_OPTS when bind-storage-ro is enabled #122
Conversation
Previously, STORAGE_OPTS environment configuration was unconditionally injected into VMs via both tmpfiles.d and a systemd service unit. This caused VMs created without `--bind-storage-ro` to reference a non-existent `/run/host-container-storage` path in their environment. Change things to only inject when opts.bind_storage_ro is set. Additionally, refactor to use a single smbios_creds vector instead of separate mount_unit_smbios_creds and storage_opts_creds vectors, making the code cleaner and easier to follow. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters <walters@verbum.org>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where STORAGE_OPTS were unconditionally injected into VMs, even when not required. The change correctly scopes the injection to only occur when the --bind-storage-ro flag is used. A comprehensive integration test has been added to verify this fix, ensuring that VMs created without the flag do not contain the unnecessary configuration. Additionally, the code has been refactored to use a single smbios_creds vector, which improves clarity and maintainability by consolidating SMBIOS credential handling. The changes are well-implemented and improve both the correctness and readability of the code. I have one suggestion to make the code even more idiomatic.
| for cred in smbios_creds { | ||
| qemu_args.push("-smbios".to_string()); | ||
| qemu_args.push(format!("type=11,value={}", mount_cred)); | ||
| qemu_args.push(format!("type=11,value={}", cred)); | ||
| } |
There was a problem hiding this comment.
For better performance and more idiomatic Rust, you can replace this loop with a combination of flat_map and extend. This avoids repeated push calls which might cause reallocations and is generally more expressive.
| for cred in smbios_creds { | |
| qemu_args.push("-smbios".to_string()); | |
| qemu_args.push(format!("type=11,value={}", mount_cred)); | |
| qemu_args.push(format!("type=11,value={}", cred)); | |
| } | |
| qemu_args.extend(smbios_creds.into_iter().flat_map(|cred| { | |
| [ | |
| "-smbios".to_string(), | |
| format!("type=11,value={}", cred), | |
| ] | |
| })); |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of unconditionally injecting STORAGE_OPTS by making the injection conditional on the --bind-storage-ro flag. The addition of the test_libvirt_run_no_storage_opts_without_bind_storage integration test is excellent, as it thoroughly verifies the fix by checking for the absence of the related configurations in the domain XML. The refactoring to use a single smbios_creds vector and consolidating the QEMU argument construction significantly improves code clarity and maintainability. I have one minor suggestion to further simplify the QEMU argument building logic.
| // Build QEMU args with all SMBIOS credentials | ||
| { | ||
| let encoded = data_encoding::BASE64.encode(tmpfiles_content.as_bytes()); | ||
| let smbios_cred = format!("io.systemd.credential.binary:tmpfiles.extra={encoded}"); | ||
| qemu_args.extend([ | ||
| "-smbios".to_string(), | ||
| format!("type=11,value={}", smbios_cred), | ||
| ]); | ||
| } | ||
|
|
||
| // Add SMBIOS credentials for mount units | ||
| for mount_cred in mount_unit_smbios_creds { | ||
| // Add all SMBIOS credentials (mount units, storage opts, etc.) | ||
| for cred in smbios_creds { | ||
| qemu_args.push("-smbios".to_string()); | ||
| qemu_args.push(format!("type=11,value={}", mount_cred)); | ||
| qemu_args.push(format!("type=11,value={}", cred)); | ||
| } |
There was a problem hiding this comment.
The logic for adding SMBIOS credentials to qemu_args is split into two parts (one for tmpfiles.extra and another for other credentials), which leads to some repetition. You could simplify this by combining all credentials into a single collection first and then iterating over it once to build the arguments. This would make the code more concise and easier to maintain.
For example:
// Collect all credentials into one vector
let mut all_creds = smbios_creds;
let encoded = data_encoding::BASE64.encode(tmpfiles_content.as_bytes());
all_creds.push(format!(
"io.systemd.credential.binary:tmpfiles.extra={}",
encoded
));
// Build QEMU args from the consolidated list
for cred in all_creds {
qemu_args.push("-smbios".to_string());
qemu_args.push(format!("type=11,value={}", cred));
}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where STORAGE_OPTS were unconditionally injected into VMs, and adds a new integration test to verify the behavior. The refactoring to consolidate SMBIOS credentials into a single vector is a great improvement for code clarity. I've added a couple of suggestions: one to further improve the credential handling logic for better maintainability, and another to make the new integration test more robust against potential name collisions.
| std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs() |
There was a problem hiding this comment.
Using the current time in seconds for unique names can lead to collisions if tests run in the same second. Using nanoseconds provides higher resolution and significantly reduces the chance of creating duplicate domain names, making the tests more robust.
| std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap() | |
| .as_secs() | |
| std::time::SystemTime::now() | |
| .duration_since(std::time::UNIX_EPOCH) | |
| .unwrap() | |
| .as_nanos() |
2 participants
Previously, STORAGE_OPTS environment configuration was unconditionally injected into VMs via both tmpfiles.d and a systemd service unit. This caused VMs created without
--bind-storage-roto reference a non-existent/run/host-container-storagepath in their environment.Change things to only inject when opts.bind_storage_ro is set.
Additionally, refactor to use a single smbios_creds vector instead of separate mount_unit_smbios_creds and storage_opts_creds vectors, making the code cleaner and easier to follow.
Assisted-by: Claude Code (Sonnet 4.5)