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

Implementation of Builder pattern for Config #59

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

gabhijit
Copy link
Contributor

Implemented Builder for VMMConfig. (Note: This is an early
implementation, more like prototype. But almost all functionality is
present.)

Removed derive(Default) from MemoryConfig, KernelConfig and
VcpuConfig and added sensible defaults (memory = 256M, vcpu=1) (derive
default will trivially initialize this to 0 and hence we are required to
take a memory coniguration from CLI launcher).

Brought these changes to api (CLI) to demonstrate a use.

Removed required from 'memory' and 'vcpu' config.

Changed TryFrom<String> to TryFrom<&str>. Believe this is better
because it's quite possible to get &str from String than other way
round and in fact often we'll get &str from CLI parsing etc. But the
other way round is painful.

Right now passes all 'existing' tests (cargo test --all, except one
which likely fails because the right kernel image is not available to
test.)

TODO: Documentation with examples for using the Builder and Test cases
for the Builder.

@gabhijit
Copy link
Contributor Author

A prototype pull request for the Builder pattern for config request. This does not affect the config refactoring of #55.

@gabhijit gabhijit force-pushed the config-default-builder branch from 14c1c3b to 4653300 Compare January 18, 2021 12:19
Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

I personally like the approach, I have a few comments on the code level, but I'll add them once other people chime in to see if we should move forward with this.

@lauralt @aghecenco @alexandruag what do you say?

The only thing that I find a bit out of hand is that in the builder we need to provide a default for KernelConfig. This could be avoided with some duplication, but we need to see if it is really important.

@lauralt
Copy link
Collaborator

lauralt commented Jan 19, 2021

I personally like the approach, I have a few comments on the code level, but I'll add them once other people chime in to see if we should move forward with this.

@lauralt @aghecenco @alexandruag what do you say?

I like the proposal too.

The only thing that I find a bit out of hand is that in the builder we need to provide a default for KernelConfig. This could be avoided with some duplication, but we need to see if it is really important.

Would be nice if we can avoid the Default impl for KernelConfig, and use the cmdline and himem_start default values somewhere else, but if it becomes weird we can also open an issue for it and not block this PR with that (from what it looks like we are still required to pass the path arg and a value for it, am I right?).

src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/config/mod.rs Show resolved Hide resolved
src/vmm/src/config/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/config/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/config/mod.rs Show resolved Hide resolved
@gabhijit gabhijit force-pushed the config-default-builder branch from 4653300 to ce8a922 Compare January 20, 2021 10:07
src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/config/builder.rs Show resolved Hide resolved
src/api/src/lib.rs Outdated Show resolved Hide resolved
@gabhijit gabhijit force-pushed the config-default-builder branch from ce8a922 to 97421ee Compare January 22, 2021 16:03
@gabhijit gabhijit changed the title [WIP] Implementation of Builder pattern for Config Implementation of Builder pattern for Config Jan 22, 2021
@gabhijit gabhijit marked this pull request as ready for review January 22, 2021 16:08
@gabhijit gabhijit force-pushed the config-default-builder branch from 97421ee to 78638bf Compare January 22, 2021 16:38
src/vmm/src/config/mod.rs Show resolved Hide resolved
src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/lib.rs Outdated Show resolved Hide resolved
src/vmm/src/config/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/config/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/config/builder.rs Show resolved Hide resolved
`TryFrom` implementations for all the `*Config` structs were using
`String` as a parameter, instead of String, because likeley we'll be
having an `&str` already from the command line etc.

Signed-off-by: Abhijit Gadgil <[email protected]>
Implemented Builder for VMMConfig (WIP).

Removed derive(Default) from MemoryConfig, KernelConfig and
VcpuConfig and added sensible defaults (memory = 256M, vcpu=1) (derive
default will trivially initialize this to 0 and hence we are required to
take a memory coniguration as user input).

Brought these changes to api (CLI) to demonstrate a use.

Removed required from 'memory' and 'vcpu' config.

Signed-off-by: Abhijit Gadgil <[email protected]>
@gabhijit gabhijit force-pushed the config-default-builder branch 2 times, most recently from 34d2140 to f029d56 Compare January 26, 2021 00:51
Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

Just a couple of nits. The code looks good to me 👍

src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
src/vmm/src/config/builder.rs Outdated Show resolved Hide resolved
Added Documentation and unit tests for config `Builder`.

Signed-off-by: Abhijit Gadgil <[email protected]>
We were using the network config param as `network_config` inside the
`VMMConfig`, but the actual CLI parameter was called `net`. There were a
few places where this inconsistency was there. Now consistently using
`net_config` everywhere so that the CLI parem `--net` makes sense

Signed-off-by: Abhijit Gadgil <[email protected]>
@gabhijit gabhijit force-pushed the config-default-builder branch from f029d56 to 057ac47 Compare January 26, 2021 10:56
@andreeaflorescu andreeaflorescu merged commit 3240ec6 into rust-vmm:master Jan 26, 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.

3 participants