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.

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

vhost_user: expand error messages for clarity #113

Merged
merged 3 commits into from
Aug 4, 2022

Conversation

stsquad
Copy link
Contributor

@stsquad stsquad commented Jul 13, 2022

Summary of the PR

The InvalidOperation error type covers a wide range of error cases
which can be inscrutable when passed to the user. To help with this
we:

  • add a textual reason field to the error message
  • create InactiveFeature for use of un-neogitated features
  • create InactiveOperation for use of un-negotiated protocol extensions

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • Any newly added unsafe code is properly documented.

@stsquad stsquad force-pushed the expand-vhost-user-errors branch 3 times, most recently from 9b985bd to aa3a436 Compare July 14, 2022 16:33
We can wrap up the feature checking into helpers to reduce the amount
of boilerplate code while enabling us to use a more idiomatic ?; exit
path on error.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
@slp
Copy link
Collaborator

slp commented Jul 22, 2022

LGTM, I think we can merge this one once the coverage number is fixed to the new value. Thanks @stsquad !

The InvalidOperation error type covers a wide range of error cases
which can be inscrutable when passed to the user. To help with this
we:

  - add a textual reason field to the error message
  - create InactiveFeature for use of un-neogitated features
  - create InactiveOperation for use of un-negotiated protocol extensions

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
The spec has been converted to .rst and now generated a nicer rendered
version whenever it is updated in QEMU's master branch. Lets use it
instead.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
@stsquad stsquad marked this pull request as ready for review July 22, 2022 10:24
@stsquad
Copy link
Contributor Author

stsquad commented Jul 22, 2022

I have a small patches waiting for vhost-user-master and vhost-device once the version for this crate is bumped .

@stsquad
Copy link
Contributor Author

stsquad commented Aug 3, 2022

Does this PR not get automatically merged once approved or does it need to be done manually?

@sboeuf
Copy link
Collaborator

sboeuf commented Aug 4, 2022

Does this PR not get automatically merged once approved or does it need to be done manually?

No it still has to be merged manually.

@sboeuf sboeuf merged commit ebaac3c into rust-vmm:main Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants