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

Move non-essential code to utility crate(s) #20

Closed
alxiord opened this issue Mar 9, 2020 · 7 comments
Closed

Move non-essential code to utility crate(s) #20

alxiord opened this issue Mar 9, 2020 · 7 comments
Assignees

Comments

@alxiord
Copy link
Member

alxiord commented Mar 9, 2020

@alxiord alxiord mentioned this issue Mar 9, 2020
8 tasks
@alxiord
Copy link
Member Author

alxiord commented Mar 10, 2020

@alxiord
Copy link
Member Author

alxiord commented Mar 10, 2020

On second thought and following a discussion with @andreeaflorescu, there's room for improvement in cmdline; it can grow from a glorified stringifier to a command line builder with high-level abstractions, so that the vmm doesn't need to work with strings and instead asks cmdline to "build itself" based on the existing devices and configurations.

@andreeaflorescu
Copy link
Member

I would disagree with moving the utilities to vmm-sys-util unless those utilities are useful for other crates. I personally like the idea of locality which means that you define what you need to use as close as possible to where you use it. Otherwise we risk to have a super bloated vmm-sys-util for no good reason.

With that in mind, how about we move the struct_util to vmm-sys-util. That one has the potential to be useful for other crates out there. For the cmdline I would like to keep it in linux-loader and potentially extend it to be somewhat smarter that what we have now.

@alxiord
Copy link
Member Author

alxiord commented Mar 10, 2020

For the cmdline I would like to keep it in linux-loader and potentially extend it to be somewhat smarter that what we have now.

#25

@dianpopa
Copy link

I would disagree with moving the utilities to vmm-sys-util unless those utilities are useful for other crates. I personally like the idea of locality which means that you define what you need to use as close as possible to where you use it. Otherwise we risk to have a super bloated vmm-sys-util for no good reason.

With that in mind, how about we move the struct_util to vmm-sys-util. That one has the potential to be useful for other crates out there. For the cmdline I would like to keep it in linux-loader and potentially extend it to be somewhat smarter that what we have now.

Oh, yes I agree. My proposal was to remove cmdline module and load_cmdline function. The cmdline module is an uitilitary and load_cmdline writes the cmdline to memory. The kernel loader crate does not use the cmdline and the cmdline does not use anything from the crate, or?
IMO If there is any crate the cmdline is local to that is the vmm (which is the one that has access to arch and to the memory).

@andreeaflorescu
Copy link
Member

Oh, yes I agree. My proposal was to remove cmdline module and load_cmdline function. The cmdline module is an uitilitary and load_cmdline writes the cmdline to memory. The kernel loader crate does not use the cmdline and the cmdline does not use anything from the crate, or?

The linux loader crate purpose is to offer wrappers for loading kernels and other things related to it. The command line is one of the related things.

IMO If there is any crate the cmdline is local to that is the vmm (which is the one that has access to arch and to the memory).

With rust-vmm we want to maximize the code that is shared between VMMs. Code in the vmm itself is usually purpose built for a use case and in most of the cases we expect it to not be shared. Related to arch, please take a look at #15 to see one approach to handle this. Regarding the memory, the linux loader takes a dependency only on the trait GuestMemory.

@alxiord
Copy link
Member Author

alxiord commented Mar 11, 2020

Alternative to moving struct_util: remove it altogether 🎉
See https://github.com/rust-vmm/vmm-sys-util/issues/77#issuecomment-597511991

alxiord pushed a commit to alxiord/linux-loader that referenced this issue Mar 11, 2020
Fixes rust-vmm#20
Closes rust-vmm/vmm-sys-util#77

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
alxiord pushed a commit to alxiord/linux-loader that referenced this issue Mar 11, 2020
Fixes rust-vmm#20
Closes rust-vmm/vmm-sys-util#77

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
@alxiord alxiord self-assigned this Mar 12, 2020
alxiord pushed a commit to alxiord/linux-loader that referenced this issue Mar 13, 2020
Fixes rust-vmm#20

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
alxiord pushed a commit to alxiord/linux-loader that referenced this issue Mar 20, 2020
Fixes rust-vmm#20

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
alxiord pushed a commit to alxiord/linux-loader that referenced this issue Mar 20, 2020
Fixes rust-vmm#20

Signed-off-by: Alexandra Iordache <aghecen@amazon.com>
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

No branches or pull requests

3 participants