Skip to content

Changing as_str into as_cstr #118

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

Conversation

andreitraistaru
Copy link
Collaborator

Changing as_str() into as_cstr() in order to retrieve a null terminated string from the Cmdline struct.

Signed-off-by: Traistaru Andrei Cristian atc@amazon.com

Summary of the PR

Please summarize here why the changes in this PR are 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.
  • Any newly added unsafe code is properly documented.

@andreitraistaru
Copy link
Collaborator Author

I'll fix the comments and also the test that are currently failing. Also, I'll add a test to validate the null terminator.

@andreitraistaru andreitraistaru force-pushed the cmdline_as_nul_terminated_string branch 3 times, most recently from aaf0bf1 to a183e6e Compare September 19, 2022 21:59
@andreitraistaru
Copy link
Collaborator Author

Thanks for the review. I'm working on addressing the comments now. I'll ping you when done.

@andreitraistaru andreitraistaru requested review from lauralt and andreeaflorescu and removed request for andreeaflorescu and lauralt September 20, 2022 10:15
@andreitraistaru andreitraistaru force-pushed the cmdline_as_nul_terminated_string branch 2 times, most recently from 4edddce to 44c244c Compare September 20, 2022 14:18
@andreitraistaru andreitraistaru force-pushed the cmdline_as_nul_terminated_string branch 2 times, most recently from ad0ada5 to 19621d6 Compare September 21, 2022 08:49
Copy link
Contributor

@lauralt lauralt left a comment

Choose a reason for hiding this comment

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

You should also squash your commits.

@andreitraistaru andreitraistaru force-pushed the cmdline_as_nul_terminated_string branch from 9dc787c to b485560 Compare September 21, 2022 13:06
@andreitraistaru andreitraistaru requested review from lauralt and andreeaflorescu and removed request for lauralt September 21, 2022 13:14
lauralt
lauralt previously approved these changes Sep 21, 2022
Copy link
Contributor

@lauralt lauralt left a comment

Choose a reason for hiding this comment

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

👍

Changing as_str() into as_cstring() in order to retrieve a
CString (which is a null terminated string) from the Cmdline
struct.

We found a bug introduced by the following PR:
rust-vmm#72
This bug was caused by the fact that method load_cmdline() was
changed to receive a Cmdline instead of a CStr. That leads to
the call of the as_str() method from the Cmdline to get the
representation of the kernel command line. The method as_str()
from Cmdline returns a plain string from Rust that is not null
terminated by default.

In this commit, we kept the load_cmdline() method to receive
a Cmdline but converted the as_str() method into as_cstring()
that returns a null terminated string now.

Signed-off-by: Traistaru Andrei Cristian <atc@amazon.com>
@andreitraistaru andreitraistaru force-pushed the cmdline_as_nul_terminated_string branch from 9d7fe16 to ede3cbf Compare September 22, 2022 05:56
@andreitraistaru andreitraistaru requested review from lauralt and removed request for andreeaflorescu September 22, 2022 05:57
@andreeaflorescu andreeaflorescu merged commit 47a1984 into rust-vmm:main Sep 22, 2022
@lauralt lauralt mentioned this pull request Sep 23, 2022
3 tasks
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