Skip to content

Commit

Permalink
Remove panic by moving problem code out of constructor
Browse files Browse the repository at this point in the history
The root of the panic is due to us trying to serialize a
`Toml::Value::Table` to a Vec<u8> inside a constructor where we can't
handle the error cleanly

This commit pulls the TOML parsing out of Service::new() into
Cfg::to_exported() so we can properly handle the Result<> on toml parsing

Note: this does not yet handle the underlying issue which is that the
TOML serializer doesn't handle tables well.  In particular it requires
all keys with non-table values must be emitted first.  This is not the
default

See https://github.com/alexcrichton/toml-rs/blob/master/src/ser.rs#L7-L12

Signed-off-by: James Casey <james@chef.io>
  • Loading branch information
James Casey committed Jan 4, 2019
1 parent ece8636 commit aa1cbca
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 9 deletions.
8 changes: 2 additions & 6 deletions components/butterfly/src/rumor/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl Service {
package: &T,
service_group: ServiceGroup,
sys: SysInfo,
cfg: Option<&toml::value::Table>,
cfg: Option<Vec<u8>>,
) -> Self
where
T: Identifiable,
Expand All @@ -110,11 +110,7 @@ impl Service {
initialized: false,
pkg: package.to_string(),
sys: sys,
// TODO FN: Can we really expect this all the time, should we return a `Result<Self>`
// in this constructor?
cfg: cfg
.map(|v| toml::ser::to_vec(v).expect("Struct should serialize to bytes"))
.unwrap_or_default(),
cfg: cfg.unwrap_or_default(),
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions components/sup/src/manager/service/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ impl Cfg {
}

/// Returns a subset of the overall configuration which intersects with the given package's exports.
pub fn to_exported(&self, pkg: &Pkg) -> Result<toml::value::Table> {
pub fn to_exported(&self, pkg: &Pkg) -> Result<Vec<u8>> {
let mut map = toml::value::Table::default();
let cfg = toml::Value::try_from(&self).unwrap();
for (key, path) in pkg.exports.iter() {
Expand All @@ -253,7 +253,7 @@ impl Cfg {
map.insert(key.clone(), curr.clone());
}
}
Ok(map)
toml::ser::to_vec(&map).map_err(|e| sup_error!(Error::TomlEncode(e)))
}

fn load_toml_file<T1, T2>(dir: T1, file: T2) -> Result<Option<toml::value::Table>>
Expand Down
2 changes: 1 addition & 1 deletion components/sup/src/manager/service/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ impl Service {
&self.pkg.ident,
self.service_group.clone(),
self.sys.as_sys_info().clone(),
exported.as_ref(),
exported,
);
rumor.incarnation = incarnation;
rumor
Expand Down

0 comments on commit aa1cbca

Please sign in to comment.