Skip to content

Get machine config V2 #248

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api_server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ tokio-core = "=0.1.12"
tokio-uds = "=0.1.7"
tokio-io = "=0.1.5"

data_model = { path = "../data_model" }
fc_util = { path = "../fc_util" }
net_util = { path = "../net_util" }
sys_util = { path = "../sys_util" }
32 changes: 18 additions & 14 deletions api_server/src/http_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ use hyper::{self, Chunk, Headers, Method, StatusCode};
use serde_json;
use tokio_core::reactor::Handle;

use request::{self, ApiRequest, AsyncOutcome, AsyncRequestBody, ParsedRequest};
use data_model::vm::MachineConfiguration;
use request::{self, ApiRequest, AsyncOutcome, AsyncRequestBody, IntoParsedRequest, ParsedRequest};
use request::instance_info::InstanceInfo;
use super::{ActionMap, ActionMapValue};
use sys_util::EventFd;
Expand Down Expand Up @@ -182,13 +183,19 @@ fn parse_machine_config_req<'a>(
body: &Chunk,
) -> Result<'a, ParsedRequest> {
match path_tokens[1..].len() {
0 if method == Method::Get => Ok(ParsedRequest::Dummy),
0 if method == Method::Get => {
let empty_machine_config = MachineConfiguration {
vcpu_count: None,
mem_size_mib: None,
};
Ok(empty_machine_config
.into_parsed_request(method)
.map_err(|s| Error::Generic(StatusCode::BadRequest, s))?)
}

0 if method == Method::Put => Ok(serde_json::from_slice::<
request::MachineConfigurationBody,
>(body)
0 if method == Method::Put => Ok(serde_json::from_slice::<MachineConfiguration>(body)
.map_err(Error::SerdeJson)?
.into_parsed_request()
.into_parsed_request(method)
.map_err(|s| Error::Generic(StatusCode::BadRequest, s))?),
_ => Err(Error::InvalidPathMethod(path, method)),
}
Expand Down Expand Up @@ -500,8 +507,8 @@ mod tests {
use net_util::MacAddr;
use fc_util::LriHashMap;
use request::async::AsyncRequest;
use request::sync::{DeviceState, DriveDescription, DrivePermissions, MachineConfigurationBody,
NetworkInterfaceBody, SyncRequest, VsockJsonBody};
use request::sync::{DeviceState, DriveDescription, DrivePermissions, NetworkInterfaceBody,
SyncRequest, VsockJsonBody};

fn body_to_string(body: hyper::Body) -> String {
let ret = body.fold(Vec::new(), |mut acc, chunk| {
Expand Down Expand Up @@ -861,10 +868,7 @@ mod tests {
let body: Chunk = Chunk::from(json);

// GET
match parse_machine_config_req(&path_tokens, &path, Method::Get, &body) {
Ok(pr_dummy) => assert!(pr_dummy.eq(&ParsedRequest::Dummy)),
_ => assert!(false),
}
assert!(parse_machine_config_req(&path_tokens, &path, Method::Get, &body).is_ok());

assert!(
parse_machine_config_req(
Expand All @@ -876,12 +880,12 @@ mod tests {
);

// PUT
let mcb = MachineConfigurationBody {
let mcb = MachineConfiguration {
vcpu_count: Some(42),
mem_size_mib: Some(1025),
};

match mcb.into_parsed_request() {
match mcb.into_parsed_request(Method::Put) {
Ok(pr) => match parse_machine_config_req(&path_tokens, &path, Method::Put, &body) {
Ok(pr_mcb) => assert!(pr.eq(&pr_mcb)),
_ => assert!(false),
Expand Down
1 change: 1 addition & 0 deletions api_server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ extern crate serde_json;
extern crate tokio_core;
extern crate tokio_uds;

extern crate data_model;
extern crate fc_util;
extern crate net_util;
extern crate sys_util;
Expand Down
12 changes: 9 additions & 3 deletions api_server/src/request/mod.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
pub mod async;
pub mod sync;

use std::result;

use hyper::Method;
pub use self::async::{AsyncOutcome, AsyncOutcomeReceiver, AsyncOutcomeSender, AsyncRequest,
AsyncRequestBody};
pub use self::sync::{BootSourceBody, DriveDescription, MachineConfigurationBody,
NetworkInterfaceBody, SyncOutcomeReceiver, SyncOutcomeSender, SyncRequest,
VsockJsonBody};
pub use self::sync::{BootSourceBody, DriveDescription, NetworkInterfaceBody, SyncOutcomeReceiver,
SyncOutcomeSender, SyncRequest, VsockJsonBody};

pub mod instance_info;

Expand All @@ -26,3 +28,7 @@ pub enum ApiRequest {
Async(AsyncRequest),
Sync(SyncRequest),
}

pub trait IntoParsedRequest {
fn into_parsed_request(self, method: Method) -> result::Result<ParsedRequest, String>;
}
72 changes: 54 additions & 18 deletions api_server/src/request/sync/machine_configuration.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
use std::result;

use futures::sync::oneshot;
use hyper::{Response, StatusCode};
use hyper::{Method, Response, StatusCode};

use data_model::vm::MachineConfiguration;
use http_service::{empty_response, json_fault_message, json_response};
use request::{ParsedRequest, SyncRequest};
use request::{IntoParsedRequest, ParsedRequest, SyncRequest};
use request::sync::GenerateResponse;

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
pub struct MachineConfigurationBody {
#[serde(skip_serializing_if = "Option::is_none")]
pub vcpu_count: Option<u8>,
#[serde(skip_serializing_if = "Option::is_none")]
pub mem_size_mib: Option<usize>,
}

#[derive(Debug)]
pub enum PutMachineConfigurationError {
InvalidVcpuCount,
Expand Down Expand Up @@ -55,13 +48,41 @@ impl GenerateResponse for PutMachineConfigurationOutcome {
}
}

impl MachineConfigurationBody {
pub fn into_parsed_request(self) -> result::Result<ParsedRequest, String> {
impl GenerateResponse for MachineConfiguration {
fn generate_response(&self) -> Response {
let vcpu_count = match self.vcpu_count {
Some(v) => v.to_string(),
None => String::from("Uninitialized"),
};
let mem_size = match self.mem_size_mib {
Some(v) => v.to_string(),
None => String::from("Uninitialized"),
};

json_response(
StatusCode::Ok,
format!(
"{{ \"vcpu_count\": {:?}, \"mem_size_mib\": {:?} }}",
vcpu_count, mem_size
),
)
}
}

impl IntoParsedRequest for MachineConfiguration {
fn into_parsed_request(self, method: Method) -> result::Result<ParsedRequest, String> {
let (sender, receiver) = oneshot::channel();
Ok(ParsedRequest::Sync(
SyncRequest::PutMachineConfiguration(self, sender),
receiver,
))
match method {
Method::Get => Ok(ParsedRequest::Sync(
SyncRequest::GetMachineConfiguration(sender),
receiver,
)),
Method::Put => Ok(ParsedRequest::Sync(
SyncRequest::PutMachineConfiguration(self, sender),
receiver,
)),
_ => Ok(ParsedRequest::Dummy),
}
}
}

Expand Down Expand Up @@ -109,18 +130,33 @@ mod tests {

#[test]
fn test_into_parsed_request() {
let body = MachineConfigurationBody {
let body = MachineConfiguration {
vcpu_count: Some(8),
mem_size_mib: Some(1024),
};
let (sender, receiver) = oneshot::channel();
assert!(
body.clone()
.into_parsed_request()
.into_parsed_request(Method::Put)
.eq(&Ok(ParsedRequest::Sync(
SyncRequest::PutMachineConfiguration(body, sender),
receiver
)))
);
let uninitialized = MachineConfiguration {
vcpu_count: None,
mem_size_mib: None,
};
assert!(
uninitialized
.clone()
.into_parsed_request(Method::Get)
.is_ok()
);
assert!(
uninitialized
.into_parsed_request(Method::Patch)
.eq(&Ok(ParsedRequest::Dummy))
);
}
}
5 changes: 3 additions & 2 deletions api_server/src/request/sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use std::result;
use futures::sync::oneshot;
use hyper::{self, StatusCode};

use data_model::vm::MachineConfiguration;
use http_service::{empty_response, json_fault_message, json_response};
use net_util::TapError;

Expand All @@ -15,7 +16,6 @@ mod vsock;

pub use self::drive::{DriveDescription, DriveError, DrivePermissions, PutDriveOutcome};
pub use self::boot_source::{BootSourceBody, BootSourceType, LocalImage};
pub use self::machine_configuration::MachineConfigurationBody;
pub use self::net::NetworkInterfaceBody;
pub use self::vsock::VsockJsonBody;

Expand Down Expand Up @@ -53,9 +53,10 @@ pub enum DeviceState {
// This enum contains messages for the VMM which represent sync requests. They each contain various
// bits of information (ids, paths, etc.), together with an OutcomeSender, which is always present.
pub enum SyncRequest {
GetMachineConfiguration(SyncOutcomeSender),
PutBootSource(BootSourceBody, SyncOutcomeSender),
PutDrive(DriveDescription, SyncOutcomeSender),
PutMachineConfiguration(MachineConfigurationBody, SyncOutcomeSender),
PutMachineConfiguration(MachineConfiguration, SyncOutcomeSender),
PutNetworkInterface(NetworkInterfaceBody, SyncOutcomeSender),
PutVsock(VsockJsonBody, SyncOutcomeSender),
}
Expand Down
4 changes: 3 additions & 1 deletion data_model/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
[package]
name = "data_model"
version = "0.1.0"
authors = ["The Chromium OS Authors"]
authors = ["Amazon firecracker team <[email protected]>"]

[dependencies]
serde = "=1.0.27"
serde_derive = "=1.0.27"
52 changes: 4 additions & 48 deletions data_model/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,49 +1,5 @@
// Copyright 2017 The Chromium OS Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
extern crate serde;
#[macro_use]
extern crate serde_derive;

/// Types for which it is safe to initialize from raw data.
///
/// A type `T` is `DataInit` if and only if it can be initialized by reading its contents from a
/// byte array. This is generally true for all plain-old-data structs. It is notably not true for
/// any type that includes a reference.
///
/// Implementing this trait guarantees that it is safe to instantiate the struct with random data.
pub unsafe trait DataInit: Copy + Send + Sync {}

// All intrinsic types and arays of intrinsic types are DataInit. They are just numbers.
macro_rules! array_data_init {
($T:ty, $($N:expr)+) => {
$(
unsafe impl DataInit for [$T; $N] {}
)+
}
}
macro_rules! data_init_type {
($T:ty) => {
unsafe impl DataInit for $T {}
array_data_init! {
$T,
0 1 2 3 4 5 6 7 8 9
10 11 12 13 14 15 16 17 18 19
20 21 22 23 24 25 26 27 28 29
30 31 32
}
}
}
data_init_type!(u8);
data_init_type!(u16);
data_init_type!(u32);
data_init_type!(u64);
data_init_type!(usize);
data_init_type!(i8);
data_init_type!(i16);
data_init_type!(i32);
data_init_type!(i64);
data_init_type!(isize);

pub mod endian;
pub use endian::*;

pub mod volatile_memory;
pub use volatile_memory::*;
pub mod vm;
16 changes: 16 additions & 0 deletions data_model/src/vm/machine_config.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
pub struct MachineConfiguration {
#[serde(skip_serializing_if = "Option::is_none")]
pub vcpu_count: Option<u8>,
#[serde(skip_serializing_if = "Option::is_none")]
pub mem_size_mib: Option<usize>,
}

impl Default for MachineConfiguration {
fn default() -> Self {
MachineConfiguration {
vcpu_count: Some(1),
mem_size_mib: Some(128),
}
}
}
3 changes: 3 additions & 0 deletions data_model/src/vm/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
pub mod machine_config;

pub use vm::machine_config::MachineConfiguration;
2 changes: 1 addition & 1 deletion devices/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@ vhost_backend = { path = "../vhost_backend" }
logger = {path = "../logger"}

[dev-dependencies]
data_model = { path = "../data_model"}
memory_model = { path = "../memory_model"}
8 changes: 4 additions & 4 deletions devices/src/virtio/queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ impl Queue {

#[cfg(test)]
pub(crate) mod tests {
extern crate data_model;
extern crate memory_model;

use std::marker::PhantomData;
use std::mem;
Expand All @@ -338,7 +338,7 @@ pub(crate) mod tests {
// The DataInit trait is required to use mem.read_obj_from_addr and write_obj_at_addr.
impl<'a, T> SomeplaceInMemory<'a, T>
where
T: data_model::DataInit,
T: memory_model::DataInit,
{
fn new(location: GuestAddress, mem: &'a GuestMemory) -> Self {
SomeplaceInMemory {
Expand Down Expand Up @@ -431,7 +431,7 @@ pub(crate) mod tests {

impl<'a, T> VirtqRing<'a, T>
where
T: data_model::DataInit,
T: memory_model::DataInit,
{
fn new(start: GuestAddress, mem: &'a GuestMemory, qsize: u16, alignment: usize) -> Self {
assert_eq!(start.0 & (alignment - 1), 0);
Expand Down Expand Up @@ -474,7 +474,7 @@ pub(crate) mod tests {
pub len: u32,
}

unsafe impl data_model::DataInit for VirtqUsedElem {}
unsafe impl memory_model::DataInit for VirtqUsedElem {}

pub type VirtqAvail<'a> = VirtqRing<'a, u16>;
pub type VirtqUsed<'a> = VirtqRing<'a, VirtqUsedElem>;
Expand Down
6 changes: 6 additions & 0 deletions memory_model/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[package]
Copy link

Choose a reason for hiding this comment

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

I have a concern here that it might be confusing for open source developers to find a data_model crate in Firecracker and another one in crosvm, which Firecracker is loosely based upon, but which contains different stuff. It would be even more confusing if the contents of crosvm's data_model are moved in a differently named crate in Firecracker (like memory_model). I understand that memory_model is a good name for memory related structs, but I'd prefer to keep naming consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

While this makes sense, I don't agree we should bend over backwards to keep as close to crossvm as possible.
I guess we could avoid renaming data_model->memory_model if we find an appropriate name for the data model stuff we want to put in data_model 😆
But personally, I'm fine with the change proposed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The plan is to remove the memory_model crate all together. From that crate only the volatile_memory.rs and some definitions from mod.rs are actually used. The plan would be to moved those to a crate where they actually belong. Might be sys_util (as Diana suggested) or something else.

name = "memory_model"
version = "0.1.0"
authors = ["The Chromium OS Authors"]

[dependencies]
2 changes: 1 addition & 1 deletion data_model/src/endian.rs → memory_model/src/endian.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
//! # Examples
//!
//! ```
//! # use data_model::*;
//! # use memory_model::*;
//! let b: Be32 = From::from(3);
//! let l: Le32 = From::from(3);
//!
Expand Down
Loading