Skip to content

treewide: streamline #[source] and Error #7069

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
Merged
rbradford merged 16 commits into from
May 21, 2025
Merged

treewide: streamline #[source] and Error #7069

rbradford merged 16 commits into from
May 21, 2025

Conversation

@phip1611
Copy link
Member

@phip1611 phip1611 commented May 16, 2025 •
edited
Loading

Follow-up of #7061 and split-out from #7066.

This MR implements std::error::Error via the thiserror::Error derive macro for all error types where this wasn't done yet. Also #[source] is set properly everywhere. This will allow #7066 eventually.

This still follows the "current" style for errors that Display::fmt() includes any suberror like this ": {0}"

Steps to Undraft

@phip1611 phip1611 mentioned this pull request May 16, 2025
Merged
11 tasks
@phip1611 phip1611 marked this pull request as ready for review May 16, 2025 16:21
@phip1611 phip1611 requested a review from a team as a code owner May 16, 2025 16:21
@phip1611 phip1611 changed the title [WIP] treewide: streamline #[source] and Error treewide: streamline #[source] and Error May 16, 2025
@phip1611
Copy link
Member Author

phip1611 commented May 16, 2025 •
edited
Loading

I think my recent rebase messed up a few things and now the CI fails. Looking again into this soon, no later than Monday. Happy weekend!

@phip1611 phip1611 marked this pull request as draft May 16, 2025 16:42
@phip1611 phip1611 mentioned this pull request May 19, 2025
Merged
@phip1611 phip1611 marked this pull request as ready for review May 19, 2025 08:14
@phip1611 phip1611 force-pushed the more-error branch 6 times, most recently from a53ee9d to aa11770 Compare May 19, 2025 10:26
@phip1611
Copy link
Member Author

phip1611 commented May 19, 2025 •
edited
Loading

I think the failing ARM64 test is flaky and unrelated, but I'm not sure @rbradford

snue
snue reviewed May 19, 2025
View reviewed changes
vhost_user_net/src/lib.rs
Comment on lines 39 to +41
/// Failed to create kill eventfd.
CreateKillEventFd(io::Error),
#[error("Failed to create kill eventfd: {0}")]
CreateKillEventFd(#[source] io::Error),
Copy link
Contributor

@snue snue May 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everywhere the #[error] macro is added, we end up with duplicate documentation strings. I think this happened for other enums previously that already duplicate this. I could not spot any pattern for when there is additional documentation for the enum members. Does this make a difference for the generated documentation?

Copy link
Member Author

@phip1611 phip1611 May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's duplicated. Not a fan but currently, I don't see a long hanging fruit to solve this.

Using a smart combination of a few IDE shortcuts, the creation of this PR was thankfully quite easy and I'm quite confident the copy and paste error rate is close to zero.

Copy link
Contributor

@snue snue May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you are going for a tree-wide cleanup, this one-liner removes the clear duplicates on your PR branch:
(requires GNU sed for the -z multi line matching)

$ git ls-files | grep .rs | xargs -n 10 sed -z -i 's_\([[:space:]]*\)/// \(.*\)\.\n\1#\[error("\2_\1#\[error("\2_g'

Leads to:

$ git diff --stat
 arch/src/aarch64/fdt.rs              |  1 -
 arch/src/aarch64/mod.rs              |  4 ----
 arch/src/aarch64/uefi.rs             |  3 ---
 arch/src/riscv64/fdt.rs              |  1 -
 arch/src/riscv64/mod.rs              |  4 ----
 arch/src/x86_64/mod.rs               |  3 ---
 arch/src/x86_64/mptable.rs           | 10 ----------
 arch/src/x86_64/regs.rs              | 12 ------------
 arch/src/x86_64/smbios.rs            |  2 --
 block/src/async_io.rs                |  5 -----
 devices/src/interrupt_controller.rs  | 12 ------------
 pci/src/bus.rs                       |  7 -------
 pci/src/device.rs                    |  5 -----
 pci/src/msix.rs                      |  2 --
 vhost_user_block/src/lib.rs          |  2 --
 vhost_user_net/src/lib.rs            |  9 ---------
 virtio-devices/src/vsock/unix/mod.rs | 10 ----------
 vm-device/src/bus.rs                 |  3 ---
 vmm/src/device_manager.rs            | 47 -----------------------------------------------
 vmm/src/memory_manager.rs            | 16 ----------------
 20 files changed, 158 deletions(-)

Copy link
Contributor

@snue snue May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I checked the generated documentation and the #[error(...)] implementation isn't easily visible. So keeping the duplicate info for the doc where it exists may be the better option than just having the enum member name show up.

Copy link
Member Author

@phip1611 phip1611 May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be some derive macro magic streamlining the Display::fmt() impl and the doc comments, but let's stick to the duplication for now. I think having doc comments at all is much better.

Also as doc comment and the display repr as close by, it is easy to keep them in sync

phip1611 added 10 commits May 21, 2025 10:16
@phip1611
misc: option_parser: streamline #[source] and Error
8b3d32b 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: api_client: streamline #[source] and Error
ce30209 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: arch/aarch64: streamline #[source] and Error
41359e7 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: arch/riscv64: streamline #[source] and Error
f16f940 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: block: streamline #[source] and Error
60b95fc 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: devices: streamline #[source] and Error
92d8b7e 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: hypervisor: streamline #[source] and Error
4b108ea 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: net_util: streamline #[source] and Error
a447c5b 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: pci: streamline #[source] and Error
4f03d0e 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: vhost_user_block: streamline #[source] and Error
b309e86 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
phip1611 added 6 commits May 21, 2025 10:18
@phip1611
misc: vhost_user_net: streamline #[source] and Error
f3586c1 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: vsock: streamline #[source] and Error
f33ac1b 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: virtio-devices: streamline #[source] and Error
3aa810c 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: arch/x86_64: streamline #[source] and Error
20f1ad1 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: arch: streamline #[source] and Error
9ea89c3 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: vmm: streamline #[source] and Error
d62cec3 
This streamlines the code base to follow best practices for
error handling in Rust: Each error struct implements
std::error::Error (most due via thiserror::Error derive macro)
and sets its source accordingly.

This allows future work that nicely prints the error chains,
for example.

So far, the convention is that each error prints its
sub error as part of its Display::fmt() impl.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
Copy link
Member Author

phip1611 commented May 21, 2025 •
edited
Loading

I rebased this. Are we good to go, here? @rbradford I know the changes are a lot and hard to review!

@rbradford rbradford added this pull request to the merge queue May 21, 2025
Merged via the queue into cloud-hypervisor:main with commit fff62d9 May 21, 2025
41 checks passed
@phip1611 phip1611 deleted the more-error branch May 21, 2025 10:04
@phip1611 phip1611 mentioned this pull request May 21, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rbradford rbradford rbradford approved these changes

+1 more reviewer

@snue snue snue left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@phip1611 @rbradford @snue