Add vhost-device-template crate

[vireshk]

This adds a basic template crate, which will be useful for new developers to understand how things work and layout all basic stuff they are expected to write to make it work.

The current implementation just parses all the requests from the virtqueue and prints the number of descriptor chains and size of each descriptor buffer in there.

This adds basic sanity tests as well for the same, which can be run to test the working of the crate.

[epilys]

LGTM, I left some suggestions but they are trivial

[vireshk]

I think I have taken care of most of the comments. Please review again :)

[epilys]

I suggest splitting the code in a lib.rs crate and a main.rs binary crate, with main.rs having only the main function. This way we can import the library crate in a tests folder.

https://doc.rust-lang.org/book/ch12-03-improving-error-handling-and-modularity.html#splitting-code-into-a-library-crate

[stefano-garzarella]

@vireshk nice, thanks for working on this!

What about also adding a paragraph in the main readme about the template?

[vireshk]

I suggest splitting the code in a lib.rs crate and a main.rs binary crate, with main.rs having only the main function. This way we can import the library crate in a tests folder.

https://doc.rust-lang.org/book/ch12-03-improving-error-handling-and-modularity.html#splitting-code-into-a-library-crate

This can be okay only if we do the same for all existing crates first. Not sure if we really want that.

Also I am not sure if it should be moved to lib.rs or something like i2c.rs which we already do.

[Ablu]

This can be okay only if we do the same for all existing crates first. Not sure if we really want that.

Also I am not sure if it should be moved to lib.rs or something like i2c.rs which we already do.

I assume that if we move anything to libs it should probably be moved to vm-virtio so that others can also benefit from it? 🤔 Or is there a major benefit from having tests under tests/ instead of under the source tree?

[epilys]

I suggest splitting the code in a lib.rs crate and a main.rs binary crate, with main.rs having only the main function. This way we can import the library crate in a tests folder.
doc.rust-lang.org/book/ch12-03-improving-error-handling-and-modularity.html#splitting-code-into-a-library-crate

This can be okay only if we do the same for all existing crates first. Not sure if we really want that.

Also I am not sure if it should be moved to lib.rs or something like i2c.rs which we already do.

Why not? By the way that's how I structured vhost-device-sound.

[vireshk]

Why not?

No reason. I am okay with lib.rs or something like i2c.rs, but I think that should be followed throughout. That's all.

By the way that's how I structured vhost-device-sound.

I haven't reviewed it yet :)

[vireshk]

I assume that if we move anything to libs it should probably be moved to vm-virtio so that others can also benefit from it?

The library crate does give an incorrect picture of the binary crate I feel. The APIs there aren't supposed to be exported but putting them in lib.rs might indicate that.

[Ablu]

Only nit-picking comments that probably applies to some of our existing daemons as well...

[Ablu]

I assume that if we move anything to libs it should probably be moved to vm-virtio so that others can also benefit from it?

The library crate does give an incorrect picture of the binary crate I feel. The APIs there aren't supposed to be exported but putting them in lib.rs might indicate that.

Sure. What I meant was that some abstractions may be helpful for non vhost-user virtio implementations. vsock already has some stuff in vm-virtio. So if we find stuff that is useful to share, moving that there maybe makes more sense. But I agree that lib.rs under vhost-device is a bit confusing. And test code probably won't be useful in vm-virtio.

[Ablu]

Why not? By the way that's how I structured vhost-device-sound.

Hm... But if we publish that to crates.io, that will allow people to use the bits in there, right? Do we want that? It might give the idea that we support using the lib directly.

[epilys]

It's a really common Rust pattern to allow for integration tests. The fact there is a lib.rs/main.rs split is not visible in crates.io. There's no different treatment on the registry (may change in the future, the plan is to distinguish binary crates. Right now all crates are shown as libraries) And we're not mentioning APIs in READMEs either.

See https://crates.io/crates/vhost-device-vsock:

Install

cargo add vhost-device-vsock

Run the following Cargo command in your project directory:
Or add the following line to your Cargo.toml:

vhost-device-vsock = "0.1.0"

yet, vhost-device-vsock has no lib.rs.

If it's too confusing for you, I have no strong feelings about this. It's just good practice to design your API cleanly and then consume it; even if you're the only user of it.

[stsquad]

This can be okay only if we do the same for all existing crates first. Not sure if we really want that.
Also I am not sure if it should be moved to lib.rs or something like i2c.rs which we already do.

I assume that if we move anything to libs it should probably be moved to vm-virtio so that others can also benefit from it? 🤔 Or is there a major benefit from having tests under tests/ instead of under the source tree?

That is the intention although it seems we are accumulating device logic quite quickly in this repo. That said it's not a major issue if there isn't the demand for the downstream VMM's who want to do in-VMM handling for VirtIO. virtio-sound is probably leading in this complexity considering the interaction between the VirtIO device and the various audio backends.

[vireshk]

What about also adding a paragraph in the main readme about the template?

Done.

[Ablu]

It's a really common Rust pattern to allow for integration tests. The fact there is a lib.rs/main.rs split is not visible in crates.io. There's no different treatment on the registry (may change in the future, the plan is to distinguish binary crates. Right now all crates are shown as libraries) And we're not mentioning APIs in READMEs either.

Yep. But you need export a lot more stuff as public like that. Since your main.rs is living in a separate crate, the libs needs to export all the functionality as pub.

If it's too confusing for you, I have no strong feelings about this. It's just good practice to design your API cleanly and then consume it; even if you're the only user of it.

Full ACK on that. I am not against making stuff reusable. I am just worried that we make vhost-device conflicting with the idea of vm-virtio :)

[vireshk]

If it's too confusing for you, I have no strong feelings about this. It's just good practice to design your API cleanly and then consume it; even if you're the only user of it.

Full ACK on that. I am not against making stuff reusable. I am just worried that we make vhost-device conflicting with the idea of vm-virtio :)

And we already kind of do that without a lib.rs, like gpio/i2c.rs, etc..

[vireshk]

Okay, we are ready for review again.. Updated few more bits.

[epilys]

It's a really common Rust pattern to allow for integration tests. The fact there is a lib.rs/main.rs split is not visible in crates.io. There's no different treatment on the registry (may change in the future, the plan is to distinguish binary crates. Right now all crates are shown as libraries) And we're not mentioning APIs in READMEs either.

Yep. But you need export a lot more stuff as public like that. Since your main.rs is living in a separate crate, the libs needs to export all the functionality as pub.

"the libs needs to export all the functionality as pub." And? We're not advertising the crate as a public stable API, visibility matters when you want to keep parts of your public API opaque. And as I said, there's no distinction as far as crates.io is concerned.

The point is: a lib.rs allows you to do integration tests. All the visibility/crates.io/vm-virtio are irrelevant.

Or is there a major benefit from having tests under tests/ instead of under the source tree?

Separation of concerns. Unit tests are testing a single thing at a time. Integration tests test for more complex interactions. Also, you don't have to recompile the crate everytime you change the tests.

These are official rust guidelines, it makes sense for rustvmm to be as Rust-like as possible.

[epilys]

One typo, otherwise LGTM

[stefano-garzarella]

I was going to click the merge button, but now I just have a copyright question.
Do we want the template to have one?
Or is it better to be generic and let the implementer have freedom to decide which one to use?

I think in the second case we should put a generic copyright and a TODO for who will use the template.

I'm in favor of both, I think we just need to be clear on whether this code has a copyright or not.

[vireshk]

I was going to click the merge button, but now I just have a copyright question. Do we want the template to have one? Or is it better to be generic and let the implementer have freedom to decide which one to use?

Copyright as in at the top of the source files ? I have already added them here.

[stefano-garzarella]

I was going to click the merge button, but now I just have a copyright question. Do we want the template to have one? Or is it better to be generic and let the implementer have freedom to decide which one to use?

Copyright as in at the top of the source files ? I have already added them here.

Yes, maybe I explained myself wrongly.

Exactly because we added it, this is going to force anybody using this template to use these copyrights that might be incompatible with others that they would like to use.

Do we want that? (I'm okay with that, I just want to understand whether or not one is allowed to change the copyright or this code is explicitly released with this copyright.)

[vireshk]

Exactly because we added it, this is going to force anybody using this template to use these copyrights that might be incompatible with others that they would like to use.

There is copyright information here: My name + Linaro and then there is SPDX License thing.. The License thing should be used by everyone, that's fine.

For the copyright part, I am not sure why it would be confusing to anyone to just update the same with their name / comapny or in some other way ?

Do we want that? (I'm okay with that, I just want to understand whether or not one is allowed to change the copyright or this code is explicitly released with this copyright.)

I think one should be allowed to add their own for their own crate.

[stefano-garzarella]

Exactly because we added it, this is going to force anybody using this template to use these copyrights that might be incompatible with others that they would like to use.

There is copyright information here: My name + Linaro and then there is SPDX License thing.. The License thing should be used by everyone, that's fine.

For the copyright part, I am not sure why it would be confusing to anyone to just update the same with their name / comapny or in some other way ?

Do we want that? (I'm okay with that, I just want to understand whether or not one is allowed to change the copyright or this code is explicitly released with this copyright.)

I think one should be allowed to add their own for their own crate.

Okay, but maybe the copyright they want to use is incompatible with these (I'm thinking of GPL for example) and so they can't use this template.

Is this okay with us? (I don't have a strong opinion, I just want to see if it's okay for you too)

[vireshk]

Okay, but maybe the copyright they want to use is incompatible with these (I'm thinking of GPL for example) and so they can't use this template.

Hmm.. We decided a while back on what licenses we should be using for crates in vhost-device and we settled with: "Apache-2.0 or BSD-3-Clause". I think we should promote the same going forward too ? I do see some GPL bits in SCMI, but then I am not sure what the right thing to do here is.

Is this okay with us? (I don't have a strong opinion, I just want to see if it's okay for you too)

I will still allow them to use the template and let them provide a reason why they don't want to go with "Apache-2.0 or BSD-3-Clause".

Will that work ?

[epilys]

Not a lawyer, but I'm sure they can use the BSD-3 parts of this template and relicense their work as gpl; BSD-3 allows this. Of course the BSD-3 parts of the template remain BSD-3.

[stefano-garzarella]

Okay, but maybe the copyright they want to use is incompatible with these (I'm thinking of GPL for example) and so they can't use this template.

Hmm.. We decided a while back on what licenses we should be using for crates in vhost-device and we settled with: "Apache-2.0 or BSD-3-Clause". I think we should promote the same going forward too ? I do see some GPL bits in SCMI, but then I am not sure what the right thing to do here is.

Yeah, perhaps we should clarify this (obviously not in this PR)

Is this okay with us? (I don't have a strong opinion, I just want to see if it's okay for you too)

I will still allow them to use the template and let them provide a reason why they don't want to go with "Apache-2.0 or BSD-3-Clause".

Will that work ?

You own the copyright, so I think you can do it, but only you, I think, unless we explicitly specify it by adding it in the copyright.

[stefano-garzarella]

Maybe we should use WTFPL (just kidding, just a game because it's Friday ;-P).
http://www.wtfpl.net/

[epilys]

Copyright re-assignment afaik is only necessary for re-licensing the code; since we use permissing licenses this would only allow people to use it for proprietary forks/releases. I don't know if I'm 100% correct but this isn't something we want, right? With Apache and BSD-3 everyone can use it in whatever FOSS way they want as long as they retain the copyright notices etc.

[stefano-garzarella]

Okay, so I think for now we merge as is, if there are problems in the future we'll see. But I hope not.

[mz-pdm]

I do see some GPL bits in SCMI, but then I am not sure what the right thing to do here is.

This is an example code modified from Linux kernel, i.e. documentation not related to the vhost-device code license.