-
Notifications
You must be signed in to change notification settings - Fork 52
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
enhancement: Enhance Enum types with string conversion capabilities #175
Conversation
Hi @utam0k @saschagrunert, I implement such conversion for enum type using macro |
src/runtime/linux.rs
Outdated
#[strum(to_string = "cgroup")] | ||
/// Cgroup Namespace for isolating cgroup hierarchies | ||
Cgroup = 0x02000000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to set this explicitly? I thought we just have to adapt the default value for Mount
and Network
and default to lower case transformation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's about this setting about them ?
#[strum(serialize_all = "lowercase")]
#[serde(rename_all = "snake_case")]
/// Available Linux namespaces.
pub enum LinuxNamespaceType {
#[strum(to_string = "mnt")]
/// Mount Namespace for isolating mount points
Mount = 0x00020000,
/// Cgroup Namespace for isolating cgroup hierarchies
Cgroup = 0x02000000,
/// Uts Namespace for isolating hostname and NIS domain name
Uts = 0x04000000,
/// Ipc Namespace for isolating System V, IPC, POSIX message queues
Ipc = 0x08000000,
/// User Namespace for isolating user and group ids
User = 0x10000000,
/// PID Namespace for isolating process ids
Pid = 0x20000000,
#[strum(to_string = "net")]
/// Network Namespace for isolating network devices, ports, stacks etc.
Network = 0x40000000,
/// Time Namespace for isolating the clocks
Time = 0x00000080,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, can you add tests so that we can verify that it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I have added related test cases for "net" and "mount/mnt". Please see the ns_type_enum_to_string()
and ns_type_string_to_enum()
For certain enum types, we need to convert them to their corresponding strings, or be able to map a given string to the corresponding enum type. However, many enum types currently lack these helper functions. With the help with strum macro, we can easily reach this goal. But we also take some special cases which are partially implemented into account. Fixes youki-dev#173 Signed-off-by: Alex Lyn <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, PTAL @utam0k
Hi @utam0k I need your help review this PR. PTAL! Thx |
@@ -1465,3 +1476,209 @@ impl Arbitrary for LinuxHugepageLimit { | |||
} | |||
} | |||
} | |||
|
|||
#[cfg(test)] | |||
mod tests { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
For certain enum types, we need to convert them to their corresponding strings, or be able to map a given string to the corresponding enum type. However, many enum types currently lack these helper functions. With the help with strum macro, we can easily reach this goal. But we also take some special cases which are partially implemented into account. For these, myabe we just implement
impl std::fmt::Display
for it.Fixes #173