-
Notifications
You must be signed in to change notification settings - Fork 16
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 FdtWriter::property_cstring
#72
base: main
Are you sure you want to change the base?
Conversation
Since FdtWriter only accept &str, it does a validation to make sure it's NULL-terminated and copies the whole string in Cstring::new(). This is unnecessary in some circumstances. For example, for "bootargs" property, linux_loader can create CString [1]. However, library users need to convert it into String [2]. This patches adds another property helper `property_cstring`, which accepts a &CStr value directly. [1] https://docs.rs/linux-loader/latest/linux_loader/cmdline/struct.Cmdline.html#method.as_cstring [2] https://github.com/firecracker-microvm/firecracker/blob/3853362520b81efc8ce6559148d023379a5a4da4/src/vmm/src/arch/aarch64/fdt.rs#L240-L246 Signed-off-by: Seiya Nuta <[email protected]>
Signed-off-by: Seiya Nuta <[email protected]>
b7b00c1
to
fd645bc
Compare
I've skipped adding tests because the new method is covered through |
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.
I guess we could probably instead extend property_string()
to accept &CStr
or &str
and do the optimal thing (at compile time) using a trait
, but this approach is simple enough and looks fine to me.
@danielverkamp Good point I was wondering that too. The problem is convenient traits such as BTW, the review requires at least 2 approvers. Whom should I ask for another review? 🙏 |
Sorry, I missed this notification - I think it needs someone from https://github.com/rust-vmm/community/blob/main/MAINTAINERS.md#gatekeepers (I haven't been involved much lately, but it seems like there is supposed to be some automation for this, possibly not enabled for this repo though?) |
Thank you for pointing it out! I'll ask for another review in a community channel. |
Summary of the PR
Since
FdtWriter
only accept&str
, it does a validation to make sure it's NULL-terminated and copies the whole string inCstring::new()
.This is unnecessary in some circumstances. For example, for
"bootargs"
property, linux_loader can createCString
[1]. However, library users need to convert it intoString
[2].This patches adds another property helper
property_cstring
, which accepts a&CStr
value directly to eliminate the overhead.[1] https://docs.rs/linux-loader/latest/linux_loader/cmdline/struct.Cmdline.html#method.as_cstring
[2] https://github.com/firecracker-microvm/firecracker/blob/3853362520b81efc8ce6559148d023379a5a4da4/src/vmm/src/arch/aarch64/fdt.rs#L240-L246
Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.