Skip to content
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

add as_string() to the Cmdline crate #169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arigo-vos
Copy link

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:

  • All commits in this PR are signed (with git commit -s), and the commit
    message has max 60 characters for the summary and max 75 characters for each
    description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

@JonathanWoollett-Light
Copy link
Contributor

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
Copy link
Author

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");
    }
}

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.

Signed-off-by: Alvise Rigo <[email protected]>
Comment on lines +534 to +545
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())
Copy link
Member

Choose a reason for hiding this comment

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

This code ends up being almost the same as the as_cstring function. Is there a way to not duplicate the code?

Copy link
Author

Choose a reason for hiding this comment

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

Good point. What about this:

impl TryFrom<&Cmdline> for CString {
    type Error = Error;

    fn try_from(value: &Cmdline) -> result::Result<Self, Self::Error> {
        let as_string = <String as TryFrom<&Cmdline>>::try_from(value)?;
        Ok(CString::new(as_string).map_err(|_| Error::NullTerminator)?)
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants