Process: namespaces: Change from Vec to HashMap #185
Conversation
|
LGTM, except for the small logic fix |
|
Cool can you please tell me what would you like me to change? Which logic? |
|
Oops, sorry about that. I forgot to click the "submit review" button |
src/process/namespaces.rs
Outdated
| @@ -39,8 +45,6 @@ impl Process { | |||
| /// See also the [Process::namespaces()] method | |||
| #[derive(Debug, Clone)] | |||
| pub struct Namespace { | |||
| /// Namespace type | |||
| pub ns_type: OsString, | |||
There was a problem hiding this comment.
Also, for better compatibility, I'm inclined to keep the ns_type field. For people who are currently using the Vec<_> interface, including the ns_type in the struct is more useful, at the expense of a small amount of memory duplication
src/process/namespaces.rs
Outdated
| namespaces | ||
| .insert( | ||
| ns_type.clone(), | ||
| Namespace { | ||
| ns_type, | ||
| path, | ||
| identifier: stat.st_ino, | ||
| device_id: stat.st_dev, | ||
| }, | ||
| ) | ||
| .map_or_else( | ||
| || Ok(()), | ||
| |n| { | ||
| Err(build_internal_error!(format!( | ||
| "NsType appears more than once {:?}", | ||
| n.ns_type | ||
| ))) | ||
| }, | ||
| )?; |
There was a problem hiding this comment.
Sorry to be a pain about this, but I think this can be written much more cleanly by just using a simple if statement:
| namespaces | |
| .insert( | |
| ns_type.clone(), | |
| Namespace { | |
| ns_type, | |
| path, | |
| identifier: stat.st_ino, | |
| device_id: stat.st_dev, | |
| }, | |
| ) | |
| .map_or_else( | |
| || Ok(()), | |
| |n| { | |
| Err(build_internal_error!(format!( | |
| "NsType appears more than once {:?}", | |
| n.ns_type | |
| ))) | |
| }, | |
| )?; | |
| if namespaces | |
| .insert( | |
| ns_type.clone(), | |
| Namespace { | |
| ns_type, | |
| path, | |
| identifier: stat.st_ino, | |
| device_id: stat.st_dev, | |
| }, | |
| ) | |
| .is_some() | |
| { | |
| return Err(build_internal_error!(format!("Duplicate namespace type {:?}", ns_type))); | |
| } |
There was a problem hiding this comment.
No problem, pushed a new revision, please let me know if there is any other change you would like to have.
This change breaks the API for process namespaces, but it provides a more efficent way of working with the namespaces of a process. Signed-off-by: Jon Doron <[email protected]>
|
Thanks! |
|
This change has been published as version |
This change breaks the API for process namespaces, but it provides
a more efficient way of working with the namespaces of a process.
Signed-off-by: Jon Doron [email protected]