-
Notifications
You must be signed in to change notification settings - Fork 565
vmm: add PayloadConfig::validate to improve error reporting + doc update #7166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
c11d5bc to
0838d78
Compare
I think this list will get more confusing with #7117 where when you have firmware you can also specify kernel, command line and initrd. |
I made the list a little more compact; this allows to extend the list in the future for |
0838d78 to
421791e
Compare
rbradford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me - this codifies what we have. But I just wanted to highlight that #7117 might change this.
Would it be a good idea to edit the original issue with an additional checkbox (section: "steps to undraft this" or similar) to look for this specific aspect? Assuming the other PR will be rebased onto this |
03269f4 to
29c7af4
Compare
vmm/src/vm_config.rs
Outdated
| match (&self.firmware, &self.kernel, &self.initramfs, &self.cmdline) { | ||
| (Some(_firmware), None, None, None) => Ok(()), | ||
| (None, Some(_kernel), _, _) => Ok(()), | ||
| (Some(_firmware), Some(_kernel), _, _) => { | ||
| Err(PayloadConfigError::FirmwarePlusOtherPayloads) | ||
| } | ||
| (Some(_firmware), _, Some(_initrd), _) => { | ||
| Err(PayloadConfigError::FirmwarePlusOtherPayloads) | ||
| } | ||
| (_, None, _, Some(_cmdline)) => Err(PayloadConfigError::CmdlineWithoutKernel), | ||
| (None, None, _, _) => Err(PayloadConfigError::MissingBootitem), | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right and aligns with our current behavior on x86_64. But I think we should take the chance to simply it.
As I read the code, the payload requirement on aarch64 (also risc-v) is cleaner though it is a bit more relax. Pretty much something like:
match (&self.firmware, &self.kernel) {
(Some(_firmware), None) => Ok(()),
(None, Some(_kernel)) => Ok(()),
(Some(_firmware), Some(_kernel)) => {
Err(PayloadConfigError::FirmwarePlusOtherPayloads)
}
(None, None) => Err(PayloadConfigError::MissingBootitem),
}
I'd change the payload requirement on x86_64 and making it consistent on all platforms. If we worry about silently ignoring kernel comdline and initrd with firmware boot, adding a warning message would be good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the relaxed approach. However, I discovered a problem with warn messages. After using this patch:
Show Code
diff --git a/vmm/src/vm.rs b/vmm/src/vm.rs
index e2e046ad7..c9cb0a376 100644
--- a/vmm/src/vm.rs
+++ b/vmm/src/vm.rs
@@ -1235,7 +1235,7 @@ impl Vm {
&payload.initramfs,
&payload.cmdline,
) {
- (Some(firmware), None, None, None) => {
+ (Some(firmware), None, _, _) => {
let firmware = File::open(firmware).map_err(Error::FirmwareFile)?;
Self::load_kernel(firmware, None, memory_manager)
}
diff --git a/vmm/src/vm_config.rs b/vmm/src/vm_config.rs
index 4619fcc8d..14570b26e 100644
--- a/vmm/src/vm_config.rs
+++ b/vmm/src/vm_config.rs
@@ -694,9 +694,6 @@ pub enum PayloadConfigError {
/// When a firmware is specified, providing kernel, initrd, or cmdline is not supported.
#[error("When a firmware is specified, providing kernel, initrd, or cmdline is not supported")]
FirmwarePlusOtherPayloads,
- /// Provided cmdline without kernel.
- #[error("Provided cmdline without kernel")]
- CmdlineWithoutKernel,
/// No bootitem provided: neither firmware nor kernel.
#[error("No bootitem provided: neither firmware nor kernel")]
MissingBootitem,
@@ -726,19 +723,29 @@ pub struct PayloadConfig {
impl PayloadConfig {
/// Validates the payload config and succeeds if Cloud Hypervisor will be
/// able to boot the configuration.
+ ///
+ /// Some odd yet non-critical configurations will result in a warning
+ /// message rather than an error.
pub fn validate(&self) -> Result<(), PayloadConfigError> {
- match (&self.firmware, &self.kernel, &self.initramfs, &self.cmdline) {
- (Some(_firmware), None, None, None) => Ok(()),
- (None, Some(_kernel), _, _) => Ok(()),
- (Some(_firmware), Some(_kernel), _, _) => {
- Err(PayloadConfigError::FirmwarePlusOtherPayloads)
- }
- (Some(_firmware), _, Some(_initrd), _) => {
+ // Hard validation
+ let res = match (&self.firmware, &self.kernel) {
+ (Some(_firmware), None) => Ok(()),
+ (None, Some(_kernel)) => Ok(()),
+ (Some(_firmware), Some(_kernel)) => {
Err(PayloadConfigError::FirmwarePlusOtherPayloads)
}
- (_, None, _, Some(_cmdline)) => Err(PayloadConfigError::CmdlineWithoutKernel),
- (None, None, _, _) => Err(PayloadConfigError::MissingBootitem),
+ (None, None) => Err(PayloadConfigError::MissingBootitem),
+ };
+
+ // After filtering out illegal configurations, no warn about odd
+ // configurations.
+ match (&self.firmware, &self.kernel, &self.initramfs, &self.cmdline) {
+ (_, None, _, Some(_cmdline)) => log::warn!("provided cmdline without kernel"),
+ (_, None, Some(_initramfs), _) => log::warn!("provided initramfs without kernel"),
+ _ => {},
}
+
+ res
}
}
I noticed that each warning is printed twice - at least. Multiple code paths actually perform some validations multiple times (such as: change config (add device) and re-validate). Therefore, I'm in favor of being more restrictive here - for all platforms: no warning messages but hard errors.
Otherwise people will also get spammed with warn messages every time a device is hot-plugged which causes a re-validation of the config. And moving the check out of PayloadConfig::validate() also doesn't feel right.
I'm looking forward to your thoughts on that! Perhaps, you have ideas how to tackle this even differently!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing the findings - I can certainly see the problem of spamming warning messages.
I think we can solve this problem by:
- Make
pub fn validate(&mut self)forPayload; - Warn it the first time, and overwrite the unused field to
None(as that's the actual config we are using with the VM anway);
Also, I'd use nested match statement, which reads much clearer to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm we’re aligned: you want to allow users to specify --cmdline and --initrd together with --firmware. This has no real effect, and we prefer printing a warning rather than rejecting the invocation of cloud Hypervisor. Correct? The latest state of the PR should reflect that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. That's the behavior we have for aarch64 (and riscv) before this change. Making x86_64 to follow the same behavior is a good change to me.
I find large match patterns with more than three components can be difficult to read and error-prone. How about:
match (&self.firmware, &self.kernel) {
(Some(_firmware), None) => {
if self.cmdline.is_some() || self.initramfs.is_some() {
log::warn!(
"Ignoring cmdline and initramfs as firmware is provided as the payload"
);
self.cmdline = None;
self.initramfs = None;
}
Ok(())
}
(None, Some(_kernel)) => Ok(()),
(Some(_firmware), Some(_kernel)) => Err(PayloadConfigError::FirmwarePlusOtherPayloads),
(None, None) => Err(PayloadConfigError::MissingBootitem),
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks better indeed, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Can you adopt the changes above so that we can land this one? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think/hope that we are finally ready to go :)
|
FYI: Technically speaking, I am back from vacation. Unfortunately, I injured myself while exercising and can hardly type. I hope to resume work in mid-August. |
@phip1611 Sorry to hear about that. Getting better soon. Take care. |
|
Convert this PR to a draft. Feel free to bring it back when it is ready. Thank you. |
29c7af4 to
422ff1b
Compare
|
I've noticed there is room for improvement for harmonizing load_payload(), load_kernel(), and load_firmware() between the three architectures. Then the boot path for all three architectures will be more streamlined. I however would prefer this in a dedicated PR. That being said, is the current PR ready to go? |
4257630 to
466fe53
Compare
Sounds good.
Yes - I looked through this and was happy. I was waiting for @likebreath to re-review. |
Overall looks good. One comment added above. |
466fe53 to
39bd8af
Compare
bc16e6f to
cdee05a
Compare
Currently, the following scenarios are supported by Cloud Hypervisor to bootstrap a VM: 1. provide firmware 2. provide kernel 3. provide kernel + cmdline 4. provide kernel + initrd 5. provide kernel + cmdline + initrd As the difference between `--firmware` and `--kernel` is not very clear currently, especially as both use/support a Xen PVH entry, adding this helps to identify the cause of misconfiguration. Signed-off-by: Philipp Schuster <[email protected]> On-behalf-of: SAP [email protected]
cdee05a to
a9818be
Compare
Currently (if I'm not missing something!), the following scenarios are supported by Cloud Hypervisor to bootstrap a VM (across all archs):
As the difference between
--firmwareand--kernelis not very clear currently, especially as both use/support a Xen PVH entry, adding this helps to identify the cause of misconfiguration. Further, I've updated the documentation.