Skip to content

treewide: streamline #[source] and Error (follow-up (1/2)) #7085

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 8 commits into from
May 22, 2025
Merged

treewide: streamline #[source] and Error (follow-up (1/2)) #7085

rbradford merged 8 commits into from
May 22, 2025

Conversation

@phip1611
Copy link
Member

@phip1611 phip1611 commented May 21, 2025 •
edited
Loading

While working on #7066, I've noticed that I've missed more Error types in the code that would benefit from streamlining them. So, this is a follow-up of #7069 basically doing the same.

I've used some tooling to analyze the code base, but it seems that I've finally caught all missing pieces!

I'd like to add that I think this is well invested time, as

  • future refactoring of error types can be done much more streamlined (no more manual Display and Error implementations, but thiserror for everything)
  • error chains (cause chains) can be nicely printed
  • whatever crate out there something nice with the std::error::Error trait, Cloud Hypervisor will be prepared

I was able to locate all missing pieces using this Python script (happily generated by ChatGPT):

import os
import re

# Matches a tuple enum variant with one inner type ending in 'Error'
variant_pattern = re.compile(r'^\s*(\w+)\s*\(\s*([A-Za-z0-9_:]+Error)\s*\)\s*,?\s*$')

for root, _, files in os.walk("."):
    for f in files:
        if not f.endswith(".rs"):
            continue
        path = os.path.join(root, f)
        with open(path, "r", encoding="utf-8") as file:
            lines = file.readlines()

        for i, line in enumerate(lines):
            match = variant_pattern.match(line)
            if match:
                # Look back for #[source]
                has_source = False
                for j in range(i - 1, -1, -1):
                    prev = lines[j].strip()
                    if not prev or prev.startswith("//"):
                        continue
                    if "#[source]" in prev:
                        has_source = True
                    break

                if not has_source:
                    print(f"{path}:{i+1}: {line.strip()}")

phip1611 added 2 commits May 21, 2025 13:03
@phip1611
misc: block: streamline #[source] and Error impl
9a638ee 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: arch: streamline #[source] and Error impl
c9f837b 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611 phip1611 requested a review from a team as a code owner May 21, 2025 11:41
rbradford
rbradford approved these changes May 21, 2025
View reviewed changes
Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

Thanks - this has been on the TODO list for a long time!

@rbradford rbradford enabled auto-merge May 21, 2025 11:44
@phip1611
misc: devices: streamline #[source] and Error impl
ecee7d7 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
auto-merge was automatically disabled May 21, 2025 11:45

Head branch was pushed to by a user without write access

@phip1611 phip1611 force-pushed the more-error-follow-up branch from f477d0f to e60d5a9 Compare May 21, 2025 11:45
This was referenced May 21, 2025
Closed
Merged
@rbradford
Copy link
Member

rbradford commented May 21, 2025

@phip1611 I think this the failure on the CI test is genuine:

Error running ch-remote command: "target/x86_64-unknown-linux-gnu/release/ch-remote" "--api-socket=/tmp/ch3qKose/cloud-hypervisor.sock" "pause"
1804
stderr: Error running command: http client error: HTTP output is missing protocol statement

phip1611 added 5 commits May 22, 2025 10:01
@phip1611
misc: option_parser: streamline #[source] and Error impl
2fe237e 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: test_infra: streamline #[source] and Error impl
5095525 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: rate_limiter: streamline #[source] and Error impl
766854e 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: performance-metrics: streamline #[source] and Error impl
75de02d 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611
misc: net_util: streamline #[source] and Error impl
4b4531c 
This streamlines the Error implementation in the Cloud Hypervisor code
base to match the remaining parts so that everything follows the agreed
conventions. These are leftovers missed in the previous commits.

Signed-off-by: Philipp Schuster <[email protected]>
On-behalf-of: SAP [email protected]
@phip1611 phip1611 force-pushed the more-error-follow-up branch from e60d5a9 to 4b4531c Compare May 22, 2025 08:05
@phip1611 phip1611 changed the title treewide: streamline #[source] and Error (follow-up) treewide: streamline #[source] and Error (follow-up (1/2)) May 22, 2025
@phip1611
Copy link
Member Author

phip1611 commented May 22, 2025 •
edited
Loading

After three hours of debugging, I traced the problem down to the following commits:

  • misc: ch-remote: streamline #[source] and Error impl
  • misc: vmm/api: streamline #[source] and Error impl

I was not able to find anything useful; I could reduce the feedback cycle of running the integration test locally to around six minutes, which is still not very pleasant. I've ran the test locally using this command: bash ./scripts/dev_cli.sh tests --integration -- --test-filter test_api_http_pause_resume -- --show-output

TL;DR: I moved the problematic part to a new PR (#7089). I'm seeking help to debug this, I don't know how to continue. Guidance or collaboration is highly appreciated!

@phip1611 phip1611 mentioned this pull request May 22, 2025
Merged
2 tasks
@rbradford rbradford added this pull request to the merge queue May 22, 2025
Merged via the queue into cloud-hypervisor:main with commit ea6d5a0 May 22, 2025
40 of 41 checks passed
@phip1611 phip1611 deleted the more-error-follow-up branch June 16, 2025 13:24
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@phip1611 @rbradford