Skip to content

Conversation

alexandruag
Copy link
Collaborator

This PR simplifies the QueueState abstraction by removing the generic type parameter M, and also relaxes the trait bounds for AvailIter and Descriptor chain to not depend on GuestAddressSpace anymore, as the logic within is only interested in M dereferencing to something that implements GuestMemory (and not also in the part that it might be one of the associated type of a GuestAddressSpace implementation).

@alexandruag alexandruag force-pushed the bye_mem2 branch 2 times, most recently from 7536050 to 1038f7b Compare October 20, 2021 21:01
@alexandruag
Copy link
Collaborator Author

Hmm not sure why the coverage drops so much (why it drops at all actually). Will have a look into that.

fn check_status_desc<M: GuestMemory>(mem: &M, desc: Descriptor) -> Result<()> {
fn check_status_desc<M>(mem: &M, desc: Descriptor) -> Result<()>
where
M: GuestMemory + ?Sized,
Copy link
Member

Choose a reason for hiding this comment

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

Why special "?Sized" marker here?

@lauralt
Copy link
Contributor

lauralt commented Oct 21, 2021

Hmm not sure why the coverage drops so much (why it drops at all actually). Will have a look into that.

Comparing the kcov output between this branch and main, looks like kcov is counting this time all the lines that no longer have the M: GuestAddressSpace generic type. That explains why the coverage increased in this PR #76, although there were no tests added for the new functionality.

@jiangliu
Copy link
Member

Hmm not sure why the coverage drops so much (why it drops at all actually). Will have a look into that.

Comparing the kcov output between this branch and main, looks like kcov is counting this time all the lines that no longer have the M: GuestAddressSpace generic type. That explains why the coverage increased in this PR #76, although there were no tests added for the new functionality.

so let's update the coverage file:)

@lauralt
Copy link
Contributor

lauralt commented Oct 26, 2021

Can't the drop be covered by unit test(s)? 6.9% is pretty much.

@bonzini
Copy link
Member

bonzini commented Oct 26, 2021

Can't the drop be covered by unit test(s)?

There are no new lines of code, so it seems like an artifact of how kcov works.

@bonzini
Copy link
Member

bonzini commented Oct 26, 2021

@alexandruag this change also allows the removal of QueueStateGuard, like this. Would you like to pick it in your branch or should I send a separate pull request once you have committed this?

@lauralt
Copy link
Contributor

lauralt commented Oct 26, 2021

Can't the drop be covered by unit test(s)?

There are no new lines of code, so it seems like an artifact of how kcov works.

There were new lines of code in this PR that were not covered by unit tests. I wasn't suggesting to add tests in the current PR, but hopefully at some point before publishing the crate.

@bonzini
Copy link
Member

bonzini commented Oct 26, 2021

The pull request in #111 brings coverage up by 2.5% compared to this one. @alexandruag, if you rebase and fix the coverage I think this can be merged. Thanks!

@bonzini
Copy link
Member

bonzini commented Nov 4, 2021

I have included rebased patches in #111.

@jiangliu
Copy link
Member

jiangliu commented Nov 9, 2021

Any updates about this MR?

Copy link
Collaborator

@slp slp left a comment

Choose a reason for hiding this comment

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

I'll be fine merging this one once its rebased and coverage_config_x86_64.json is adjusted.

We are using a `: GuestAddressSpace` trait bound for `AvailIter` and
`DescriptorChain`, but the objects are actually only interested in
the fact that `M` can dereference to a `GuestMemory` implementation.

Signed-off-by: Alexandru Agache <aagch@amazon.com>
Signed-off-by: Alexandru Agache <aagch@amazon.com>
There were a couple of changes in
rust-vmm#76 that are not
yet tested. PR that covers that functionality is in review
here: rust-vmm#111.
Until that one is merged, the coverage needs to be decreased.

Signed-off-by: Laura Loghin <lauralg@amazon.com>
@lauralt
Copy link
Contributor

lauralt commented Nov 18, 2021

@slp @bonzini @jiangliu I rebased the PR.

Copy link
Collaborator

@slp slp left a comment

Choose a reason for hiding this comment

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

Thanks @alexandruag and @lauralt

@bonzini bonzini merged commit df6f87c into rust-vmm:main Nov 18, 2021
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.

5 participants