rust-vmm / linux-loader

Linux kernel loader
Apache License 2.0
181 stars 55 forks source link

add as_string() to the Cmdline crate #169

Open arigo-vos opened 10 months ago

arigo-vos commented 10 months ago

In some cases, having a String representation of the Linux command line can be useful. This is the case of vmm-reference which otherwise would require a conversion dance between string types.

Summary of the PR

These changes are needed by vmm-reference which will be updated in another pull to use the a more recent version of this create. It turned out that without a way to directly cover to String, a dance of conversions was needed.

Requirements

Before submitting your PR, please make sure you addressed the following requirements:

JonathanWoollett-Light commented 10 months ago

impl From<CmdLine> for String would be the more idiomatic approach.

I remember having this discussion in the past, and I believe it was the case that as_string only exists as it predates the trait.

arigo-vos commented 10 months ago

impl From<CmdLine> for String would be the more idiomatic approach.

I remember having this discussion in the past, and I believe it was the case that as_string only exists as it predates the trait.

Hello,

thanks for the feedback! Yes, I totally agree. The reason why I did not go that way is to keep consistency with the rest of the code (i.e., as_cstring() which can also be refactored in this way). So I suggest to implement impl TryFrom<&CmdLine> for String:

/// Convert a Cmdline to String
///
/// # Examples
///
/// ```rust
/// # use linux_loader::cmdline::*;
/// let mut cl = Cmdline::new(20).unwrap();
/// cl.insert_str("foo").unwrap();
/// cl.insert_init_args("bar").unwrap();
/// assert_eq!(String::try_from(&cl).unwrap(), "foo -- bar");
/// ```
impl TryFrom<&Cmdline> for String {
    type Error = Error;

    fn try_from(value: &Cmdline) -> result::Result<Self, Self::Error> {
        if value.boot_args.is_empty() && value.init_args.is_empty() {
            Ok("".to_string())
        } else if value.boot_args.is_empty() {
            Err(Error::NoBootArgsInserted)
        } else if value.init_args.is_empty() {
            Ok(value.boot_args.to_string())
        } else {
            Ok(format!(
                "{}{}{}",
                value.boot_args, INIT_ARGS_SEPARATOR, value.init_args
            )
            .to_string())
        }
    }
}

And maybe this test:

    #[test]
    fn test_string_from_cmdline() {
        let mut cl = Cmdline::new(CMDLINE_MAX_SIZE).unwrap();

        assert_eq!(String::try_from(&cl).unwrap(), "");
        cl.insert_init_args("bar").unwrap();
        // This must fail as it is not allowed to have only init args
        let err = String::try_from(&cl).unwrap_err();
        assert_eq!(err, Error::NoBootArgsInserted);

        // However, inserting only bootargs is permitted
        cl = Cmdline::new(CMDLINE_MAX_SIZE).unwrap();
        cl.insert_str("foo").unwrap();
        assert_eq!(String::try_from(&cl).unwrap(), "foo");

        // Test also the concatenation of arguments
        cl.insert_init_args("bar").unwrap();
        assert_eq!(String::try_from(&cl).unwrap(), "foo -- bar");
    }
}