Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure errors types make use of displaydoc::Display andthiserror::Error #4544

Closed
JonathanWoollett-Light opened this issue Apr 8, 2024 · 2 comments · Fixed by #4585
Closed

Ensure errors types make use of displaydoc::Display andthiserror::Error #4544

JonathanWoollett-Light opened this issue Apr 8, 2024 · 2 comments · Fixed by #4585
Labels
Good first issue Indicates a good issue for first-time contributors

Comments

@JonathanWoollett-Light
Copy link
Contributor

JonathanWoollett-Light commented Apr 8, 2024 •
edited

Description

Some types used as error variants for Result::Err do not properly implement displaydoc::Display and thiserror::Error.

Some examples are:

Ideally all types which implement std::error::Error should implement std::fmt::Display using the std::fmt::Display implementation of types they contain (this is true for all error types both structs and enums). {0} prints the value with the std::fmt::Display implementation while {0:?} prints the value with the std::fmt::Debug implementation.

Exceptions are that:

  1. It may not be possible to print with a Display implementation due to the underlying type not implementing Error or Display e.g. if the underlying type is Vec<u8>.
  2. It may not be practical to print with the Display implementation due to the underlying type being from an external library. In this case it should still be changed, but this is unlikely to fit into the hackathon.

Possible instances can be seen

git grep "{0:\?}" src |wc -l 
src/firecracker/src/api_server_adapter.rs:28:    /// MicroVM stopped with an error: {0:?}
src/firecracker/src/main.rs:57:    /// Could not initialize metrics: {0:?}
src/firecracker/src/main.rs:585:    /// MicroVMStopped without an error: {0:?}
src/jailer/src/main.rs:47:    #[error("Failed to change owner for {0:?}: {1}")]
src/jailer/src/main.rs:51:    #[error("Failed to change permissions on {0:?}: {1}")]
src/jailer/src/main.rs:115:    #[error("Regex failed: {0:?}")]
src/rebase-snap/src/main.rs:21:    /// Invalid base file: {0:?}
src/rebase-snap/src/main.rs:23:    /// Invalid diff file: {0:?}
src/rebase-snap/src/main.rs:25:    /// Failed to seek data: {0:?}
src/rebase-snap/src/main.rs:27:    /// Failed to seek hole: {0:?}
src/rebase-snap/src/main.rs:29:    /// Failed to seek: {0:?}
src/rebase-snap/src/main.rs:31:    /// Failed to send the file: {0:?}
src/rebase-snap/src/main.rs:33:    /// Failed to get metadata: {0:?}
src/seccompiler/src/backend.rs:106:    /// {0:?}
src/snapshot-editor/src/edit_memory.rs:27:    /// Failed to send the file: {0:?}
src/vmm/src/arch/aarch64/vcpu.rs:30:    /// Failed FamStructWrapper operation: {0:?}
src/vmm/src/cpu_config/x86_64/cpuid/intel/normalize.rs:233:    /// Missing frequency: {0:?}.
src/vmm/src/cpu_config/x86_64/cpuid/intel/normalize.rs:235:    /// Missing space: {0:?}.
src/vmm/src/cpu_config/x86_64/cpuid/mod.rs:300:    /// Unsupported CPUID manufacturer id: \"{0:?}\" (only 'GenuineIntel' and 'AuthenticAMD' are supported).
src/vmm/src/devices/legacy/serial.rs:65:    /// Serial error: {0:?}
src/vmm/src/devices/virtio/virtio_block/io/mod.rs:33:    /// Unsupported engine type: {0:?}
src/vmm/src/lib.rs:281:    /// Failed to send event to vcpu thread: {0:?}
src/vmm/src/rpc_interface.rs:292:    /// Populating MMDS from file failed: {0:?}.
src/vmm/src/vmm_config/balloon.rs:27:    /// Error creating the balloon device: {0:?}
src/vmm/src/vmm_config/balloon.rs:29:    /// Error updating the balloon device configuration: {0:?}
src/vmm/src/vmm_config/drive.rs:24:    /// Unable to create the virtio block device: {0:?}
src/vmm/src/vmm_config/drive.rs:26:    /// Unable to create the vhost-user block device: {0:?}
src/vmm/src/vmm_config/vsock.rs:16:    /// Cannot create backend for vsock device: {0:?}
src/vmm/src/vmm_config/vsock.rs:18:    /// Cannot create vsock device: {0:?}
src/vmm/src/vstate/memory.rs:34:    /// Cannot access file: {0:?}
src/vmm/src/vstate/memory.rs:36:    /// Cannot create memory: {0:?}
src/vmm/src/vstate/memory.rs:38:    /// Cannot create memory region: {0:?}
src/vmm/src/vstate/memory.rs:40:    /// Cannot fetch system's page size: {0:?}
src/vmm/src/vstate/memory.rs:42:    /// Cannot dump memory: {0:?}
src/vmm/src/vstate/memory.rs:48:    /// Cannot create memfd: {0:?}
src/vmm/src/vstate/memory.rs:50:    /// Cannot resize memfd file: {0:?}

Acceptance Criteria

Merge a PR fixing instances of this.
@JonathanWoollett-Light JonathanWoollett-Light added the Good first issue Indicates a good issue for first-time contributors label Apr 8, 2024
@Ecazares15
Copy link
Contributor

Hello!

We are students from the University of Texas at Austin taking a virtualization course (cs360v) looking for opportunities to contribute to an open source project for class credit.

Could I be assigned to this?

@JonathanWoollett-Light
Copy link
Contributor Author

@Ecazares15 Hi, it would be great if you could contribute to this issue. Feel free to just post a PR next time you don't need to ask.

@Ecazares15 Ecazares15 mentioned this issue Apr 28, 2024
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Good first issue Indicates a good issue for first-time contributors
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Derived Error and Display Updates Ecazares15/firecracker
2 participants
@JonathanWoollett-Light @Ecazares15